Use PiT for outdated document search #98797

mshustov · 2021-04-29T17:15:05Z

Summary

Part of #90279
PR adds PiT usage when searching for outdated docs. I removed refresh: wait_for to reduce reindex overhead. Instead, Kibana does manual refresh after all the docs were transformed (for both index migration and outdated documents migration).

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

mshustov · 2021-05-03T15:56:46Z

src/core/server/saved_objects/migrationsv2/actions/index.ts

-   * index for backup purposes, but won't be available in the upgraded index.
-   */
-  unusedTypesQuery: Option.Option<estypes.QueryContainer>,
+  query: estypes.QueryContainer | undefined,


It's always defined, so I removed the Option container

Should we update the readWithPit description?

It's always defined

Why the | undefined then? For testing purposes?

Was the comment meant for reindex, L586, where you changed from Option.Option<estypes.QueryContainer> to estypes.QueryContainer?

Why the | undefined then? For testing purposes?

Was the comment meant for reindex, L586, where you changed from Option.Option<estypes.QueryContainer> to estypes.QueryContainer?

yes for both

mshustov · 2021-05-03T15:59:21Z

src/core/server/saved_objects/migrationsv2/types.ts


-export type FatalState = BaseState & {


it improves TS performance

mshustov · 2021-05-03T17:33:59Z

src/core/server/saved_objects/migrationsv2/actions/index.ts

@@ -997,6 +1008,8 @@ interface SearchForOutdatedDocumentsOptions {
 * Search for outdated saved object documents with the provided query. Will
 * return one batch of documents. Searching should be repeated until no more
 * outdated documents can be found.
+ *
+ * Used for testing only


we can move it to testing tools in a follow-up

mshustov · 2021-05-03T17:35:40Z

src/core/server/saved_objects/migrationsv2/next.ts

+      Actions.readWithPit(
+        client,
+        state.pitId,
+        state.outdatedDocumentsQuery,


it doesn't use unusedTypesQuery because target index already contains filtered SO types

TinaHeiligers

Code review only: Implementation looks solid and the logic flow makes sense.
I left a few comments and questions but these shouldn't hold up the PR.
LGTM

TinaHeiligers · 2021-05-03T18:24:41Z

src/core/server/saved_objects/migrationsv2/actions/index.ts

-   * index for backup purposes, but won't be available in the upgraded index.
-   */
-  unusedTypesQuery: Option.Option<estypes.QueryContainer>,
+  query: estypes.QueryContainer | undefined,


Should we update the readWithPit description?

src/core/server/saved_objects/migrationsv2/next.ts

src/core/server/saved_objects/migrationsv2/model.ts

pgayvallet

LGTM

pgayvallet · 2021-05-04T06:52:13Z

src/core/server/saved_objects/migrationsv2/actions/index.ts

-   * index for backup purposes, but won't be available in the upgraded index.
-   */
-  unusedTypesQuery: Option.Option<estypes.QueryContainer>,
+  query: estypes.QueryContainer | undefined,


It's always defined

Why the | undefined then? For testing purposes?

Was the comment meant for reindex, L586, where you changed from Option.Option<estypes.QueryContainer> to estypes.QueryContainer?

pgayvallet · 2021-05-04T06:59:14Z

src/core/server/saved_objects/migrationsv2/integration_tests/actions.test.ts

+      const readWithPitTask = readWithPit(
+        client,
+        pitResponse.right.pitId,
+        {
+          match: { title: { query: 'doc' } },
+        },
+        1000,
+        undefined,
+        true
+      );


We may want to switch to an object argument the next time we touch this code, argument list is hard to follow without seing the signature (undefined, true). For consistency, we may even want to do that for all actions

#97222 (comment)

src/core/server/saved_objects/migrationsv2/integration_tests/actions.test.ts

pgayvallet · 2021-05-04T07:03:57Z

src/core/server/saved_objects/migrationsv2/integration_tests/actions.test.ts

+          expect.not.objectContaining({
+            _seq_no: expect.any(Number),
+            _primary_term: expect.any(Number),
+          }),
+        ])


NIT: Test will pass if _seq_no or _primary_term exist but it not a number. I would rather test for property existence here. OTOH, by the ES spec, these properties are numbers, so it's probably fine as it is.

pgayvallet · 2021-05-04T07:06:59Z

src/core/server/saved_objects/migrationsv2/integration_tests/outdated_docs.test.ts

+          // contains migrated index with 8.0 aliases to skip migration, but run outdated doc search
+          dataArchive: Path.join(__dirname, 'archives', '8.0.0_migrated_with_outdated_docs.zip'),


with 8.0 aliases

We'll probably need to disable this test in the 7.x branch, as I think it will fail because the alias points to an higher version of Kibana?

But I don't see any other option to test such scenario on master.

discussed offline: it's okay to skip the test on 7.x branch to reduce the maintenance cost of different fixtures for 8 and 7.x branches

kibanamachine · 2021-05-04T09:53:41Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #124066 failed dccc125
💚 Build #123887 succeeded c09e098
💔 Build #123809 failed 4bfc454
💚 Build #123782 succeeded 57c1d8e
💚 Build #123479 succeeded acf8f63

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* use PiT for OUTDATED_DOCUEMENT_SEARCH step * update tests * fix typo * fix so migrations unit tests * TEMP: use wait_for when transformin docs * add a step to refresh target index if transformed outdated docs * add unit tests * refresh before searching outdated docs * add integration test for outdated docs migration * add a step to refresh target index if transformed outdated docs * make query required * address comments

kibanamachine · 2021-05-04T09:56:24Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* Use PiT for outdated document search (#98797) * use PiT for OUTDATED_DOCUEMENT_SEARCH step * update tests * fix typo * fix so migrations unit tests * TEMP: use wait_for when transformin docs * add a step to refresh target index if transformed outdated docs * add unit tests * refresh before searching outdated docs * add integration test for outdated docs migration * add a step to refresh target index if transformed outdated docs * make query required * address comments * skip test Co-authored-by: Mikhail Shustov <restrry@gmail.com>

mshustov added 3 commits April 29, 2021 19:12

use PiT for OUTDATED_DOCUEMENT_SEARCH step

9e8a4a7

update tests

c2894d0

fix typo

d3d889b

mshustov added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.14.0 project:ResilientSavedObjectMigrations Reduce Kibana upgrade failures by making saved object migrations more resilient labels Apr 29, 2021

mshustov added 9 commits April 30, 2021 09:28

fix so migrations unit tests

3787f06

Merge branch 'master' into migration-outdated-docs-search-pit

eecc175

TEMP: use wait_for when transformin docs

acf8f63

Merge branch 'master' into migration-outdated-docs-search-pit

9292992

add a step to refresh target index if transformed outdated docs

57c1d8e

add unit tests

13b17df

refresh before searching outdated docs

4bfc454

add integration test for outdated docs migration

e6ce633

add a step to refresh target index if transformed outdated docs

66438a1

mshustov commented May 3, 2021

View reviewed changes

make query required

c09e098

mshustov commented May 3, 2021

View reviewed changes

mshustov marked this pull request as ready for review May 3, 2021 17:36

mshustov requested a review from a team as a code owner May 3, 2021 17:36

mshustov requested review from TinaHeiligers and pgayvallet May 3, 2021 17:36

TinaHeiligers approved these changes May 3, 2021

View reviewed changes

mshustov added 2 commits May 4, 2021 08:47

Merge branch 'master' into migration-outdated-docs-search-pit

46a5ce8

address comments

6d70450

pgayvallet approved these changes May 4, 2021

View reviewed changes

address comments

dccc125

mshustov enabled auto-merge (squash) May 4, 2021 07:45

mshustov added the auto-backport Deprecated - use backport:version if exact versions are needed label May 4, 2021

fix test

19306c7

mshustov merged commit c815e4c into elastic:master May 4, 2021

mshustov deleted the migration-outdated-docs-search-pit branch May 4, 2021 09:53

kibanamachine mentioned this pull request May 4, 2021

[7.x] Use PiT for outdated document search (#98797) #99176

Merged

TinaHeiligers mentioned this pull request May 4, 2021

[Saved object migrations] Collect all documents that fail to transform before stopping the migration #96986

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PiT for outdated document search #98797

Use PiT for outdated document search #98797

mshustov commented Apr 29, 2021 •

edited

Loading

mshustov May 3, 2021

TinaHeiligers May 3, 2021

pgayvallet May 4, 2021

mshustov May 4, 2021

mshustov May 3, 2021

mshustov May 3, 2021 •

edited

Loading

mshustov May 3, 2021

TinaHeiligers left a comment

TinaHeiligers May 3, 2021

pgayvallet left a comment

pgayvallet May 4, 2021

pgayvallet May 4, 2021

mshustov May 4, 2021

pgayvallet May 4, 2021

pgayvallet May 4, 2021

mshustov May 4, 2021

kibanamachine commented May 4, 2021

kibanamachine commented May 4, 2021

		// contains migrated index with 8.0 aliases to skip migration, but run outdated doc search
		dataArchive: Path.join(__dirname, 'archives', '8.0.0_migrated_with_outdated_docs.zip'),

Use PiT for outdated document search #98797

Use PiT for outdated document search #98797

Conversation

mshustov commented Apr 29, 2021 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented May 4, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented May 4, 2021

💚 Backport successful

mshustov commented Apr 29, 2021 •

edited

Loading

mshustov May 3, 2021 •

edited

Loading