Don't load global state when only restoring indices #29239

tlrx · 2018-03-25T14:52:38Z

Restoring a snapshot (or getting the status of finished snapshots) currently always load the global state metadata file from the repository even if it not required. This slows down the restore process (or listing statuses process) and can also be an issue if the global state cannot be deserialized (because it has unknown customs for example).

This commit splits the Repository.getSnapshotMetadata() method into two distincts methods: getGlobalMetadata() and getIndexMetadata() that are now called on purpose. It also adds a test to control which/when metadata files are accessed. I'm not really happy of the test but I didn't come up with a better one. It also renames few variables and reorganizes the order of some instructions.

Closes #28934

Restoring a snapshot, or getting the status of finished snapshots, currently always load the global state metadata file from the repository even if it not required. This slows down the restore process (or listing statuses process) and can also be an issue if the global state cannot be deserialized (because it has unknown customs for example). This commit splits the Repository.getSnapshotMetadata() method into two distincts methods: getGlobalMetadata() and getIndexMetadata() that are now called on purpose. Closes elastic#28934

elasticmachine · 2018-03-25T14:52:39Z

Pinging @elastic/es-distributed

ywelsch

Change looks good. What about deleting snapshots not requiring to load global state (as mentioned in #28934)? Will you do that as a follow-up?
Regarding tests, an alternative (or addition to the existing) test is to corrupt the global metadata and then check that you can still restore indices (see testListCorruptedSnapshot for inspiration). Similarly, corrupt one index and check that you can still restore another index in same snapshot.

ywelsch · 2018-03-26T08:36:43Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

+            }
+
+            final List<IndexId> indexIdsInSnapshot = repositoryData.resolveIndices(indicesInSnapshot);
+            if (indexIdsInSnapshot != null) {


should never be null?

Right, it should never be null.

ywelsch · 2018-03-26T08:51:42Z

server/src/test/java/org/elasticsearch/snapshots/MetadataLoadingDuringSnapshotRestoreIT.java

+        }
+    }
+
+    private void assertIndexMetadataLoads(final String snapshot, final String index, final Integer times) {


use an int instead of an integer and use 0 to denote that indexmetadata was never loaded

I wonder why I did this. Thanks

ywelsch · 2018-03-26T08:54:22Z

server/src/test/java/org/elasticsearch/snapshots/MetadataLoadingDuringSnapshotRestoreIT.java

+
+/**
+ * This class tests that snapshot's global metadata and index metadata are loaded from the
+ * {@link Repository} on purpose


maybe instead: tests whether global and index metadata are only loaded from the repository when needed.

tlrx · 2018-03-26T12:41:31Z

Thanks @ywelsch for the review, I updated the code with your feedback.

What about deleting snapshots not requiring to load global state (as mentioned in #28934)? Will you do that as a follow-up?

Yes, I'd like to look at this in detail in another pull request.

Regarding tests, an alternative (or addition to the existing) test is to corrupt the global metadata and then check that you can still restore indices (see testListCorruptedSnapshot for inspiration). Similarly, corrupt one index and check that you can still restore another index in same snapshot.

That's a great suggestion, I added this two tests.

ywelsch

LGTM

ywelsch · 2018-03-26T15:52:31Z

server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java

+    public void testRestoreSnapshotWithCorruptedIndexMetadata() throws Exception {
+        final Client client = client();
+        final Path repo = randomRepoPath();
+        final int nbIndices = randomIntBetween(2, 7);


maybe 7 indices with 50 docs is a bit too much (= slow test), let's reduce randomness to 3 indices, each max 2 shards, and 10 docs.

imotov

LGTM2

tlrx · 2018-03-28T08:24:47Z

Thanks @ywelsch and @imotov

Restoring a snapshot, or getting the status of finished snapshots, currently always load the global state metadata file from the repository even if it not required. This slows down the restore process (or listing statuses process) and can also be an issue if the global state cannot be deserialized (because it has unknown customs for example). This commit splits the Repository.getSnapshotMetadata() method into two distincts methods: getGlobalMetadata() and getIndexMetadata() that are now called only when needed.

tlrx · 2018-05-03T11:15:20Z

This change has been backported to 6.x in 9a065af

* 6.x: Stop forking javac (#30462) Fix tribe tests Docs: Use task_id in examples of tasks (#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (#30442) Packaging: Set elasticsearch user to have non-existent homedir (#29007) [Docs] Fix typo in cardinality-aggregation.asciidoc (#30434) Avoid NPE in `more_like_this` when field has zero tokens (#30365) Build: Switch to building javadoc with html5 (#30440) Add a quick tour of the project to CONTRIBUTING (#30187) Add stricter geohash parsing (#30376) Reindex: Use request flavored methods (#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (#30432) Auto-expand replicas when adding or removing nodes (#30423) Silence IndexUpgradeIT test failures. (#30430) Fix line length violation in cache tests Add failing test for core cache deadlock [DOCS] convert forcemerge snippet Update forcemerge.asciidoc (#30113) Added zentity to the list of API extension plugins (#29143) Fix the search request default operation behavior doc (#29302) (#29405) Watcher: Mark watcher as started only after loading watches (#30403) Correct wording in log message (#30336) Do not fail snapshot when deleting a missing snapshotted file (#30332) AwaitsFix testCreateShrinkIndexToN DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (#30400) [Docs] Add snippets for POS stop tags default value Remove entry inadvertently picked into changelog Move respect accept header on no handler to 6.3.1 Respect accept header on no handler (#30383) [Test] Add analysis-nori plugin to the vagrant tests [Rollup] Validate timezone in range queries (#30338) [Docs] Fix bad link [Docs] Fix end of section in the korean plugin docs add the Korean nori plugin to the change logs Expose the Lucene Korean analyzer module in a plugin (#30397) Docs: remove transport_client from CCS role example (#30263) Test: remove cluster permission from CCS user (#30262) Watcher: Remove unneeded index deletion in tests fix docs branch version fix lucene snapshot version Upgrade to 7.4.0-snapshot-1ed95c097b (#30357) [ML][TEST] Clean up jobs in ModelPlotIT Watcher: Ensure trigger service pauses execution (#30363) [DOCS] Fixes ordering of changelog sections [DOCS] Commented out empty sections in the changelog to fix the doc build. (#30372) Make RepositoriesMetaData contents unmodifiable (#30361) Change signature of Get Repositories Response (#30333) 6.x Backport: Terms query validate bug (#30319) InternalEngineTests.testConcurrentOutOfOrderDocsOnReplica should use two documents (#30121) Security: reduce garbage during index resolution (#30180) Test: use trial license in qa tests with security [ML] Add integration test for model plots (#30359) SQL: Fix bug caused by empty composites (#30343) [ML] Account for gaps in data counts after job is reopened (#30294) [ML] Refactor DataStreamDiagnostics to use array (#30129) Make licensing FIPS-140 compliant (#30251) Do not load global state when deleting a snapshot (#29278) Don't load global state when only restoring indices (#29239) Tests: Use different watch ids per test in smoke test (#30331) Watcher: Make start/stop cycle more predictable and synchronous (#30118) [Docs] Add term query with normalizer example Adds Eclipse config for xpack licence headers (#30299) Fix message content in users tool (#30293) [DOCS] Removed X-Pack breaking changes page [DOCS] Added security breaking change [DOCS] Fixes link to TLS LDAP info [DOCS] Merges X-Pack release notes into changelog (#30350) [DOCS] Fixes broken links to bootstrap user (#30349) [Docs] Remove errant changelog line Fix NPE when CumulativeSum agg encounters null/empty bucket (#29641) [DOCS] Reorganizes authentication details in Stack Overview (#30280) Tests: Simplify VersionUtils released version splitting (#30322) Fix merging logic of Suggester Options (#29514) ReplicationTracker.markAllocationIdAsInSync may hang if allocation is cancelled (#30316) [DOCS] Adds LDAP realm configuration details (#30214) [DOCS] Adds native realm configuration details (#30215) Disable SSL on testing old BWC nodes (#30337) [DOCS] Enables edit links for X-Pack pages Cancelling a peer recovery on the source can leak a primary permit (#30318) SQL: Reduce number of ranges generated for comparisons (#30267) [DOCS] Adds links to changelog sections Convert server javadoc to html5 (#30279) REST Client: Add Request object flavored methods (#29623) Create default ES_TMPDIR on Windows (#30325) [Docs] Clarify `fuzzy_like_this` redirect (#30183) Fix docs of the `_ignored` meta field. Add a new `_ignored` meta field. (#29658) Move repository-azure fixture test to QA project (#30253)

tlrx added review :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 labels Mar 25, 2018

tlrx requested review from imotov and ywelsch March 25, 2018 14:52

ywelsch reviewed Mar 26, 2018

View reviewed changes

tlrx added 3 commits March 26, 2018 12:33

Apply feedback

ddae0d7

Add another test

063dec5

Add another test with corrupted index metadata

3099553

ywelsch approved these changes Mar 26, 2018

View reviewed changes

Reduce number of indices/shards/docs

496c6d8

imotov approved these changes Mar 27, 2018

View reviewed changes

tlrx merged commit 36f8531 into elastic:master Mar 28, 2018

tlrx deleted the do-not-load-always-global-state branch March 28, 2018 07:36

tlrx added the v6.4.0 label May 3, 2018

lcawl added the >enhancement label Aug 20, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't load global state when only restoring indices #29239

Don't load global state when only restoring indices #29239

tlrx commented Mar 25, 2018 •

edited

Loading

elasticmachine commented Mar 25, 2018

ywelsch left a comment

ywelsch Mar 26, 2018

tlrx Mar 26, 2018

ywelsch Mar 26, 2018

tlrx Mar 26, 2018

ywelsch Mar 26, 2018

tlrx commented Mar 26, 2018

ywelsch left a comment

ywelsch Mar 26, 2018

tlrx Mar 27, 2018

imotov left a comment

tlrx commented Mar 28, 2018

tlrx commented May 3, 2018

Don't load global state when only restoring indices #29239

Don't load global state when only restoring indices #29239

Conversation

tlrx commented Mar 25, 2018 • edited Loading

elasticmachine commented Mar 25, 2018

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch Mar 26, 2018

Choose a reason for hiding this comment

tlrx Mar 26, 2018

Choose a reason for hiding this comment

ywelsch Mar 26, 2018

Choose a reason for hiding this comment

tlrx Mar 26, 2018

Choose a reason for hiding this comment

ywelsch Mar 26, 2018

Choose a reason for hiding this comment

tlrx commented Mar 26, 2018

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch Mar 26, 2018

Choose a reason for hiding this comment

tlrx Mar 27, 2018

Choose a reason for hiding this comment

imotov left a comment

Choose a reason for hiding this comment

tlrx commented Mar 28, 2018

tlrx commented May 3, 2018

tlrx commented Mar 25, 2018 •

edited

Loading