Remove IndexShard dependency from Repository #42213

s1monw · 2019-05-20T07:48:46Z

In order to simplify repository testing especially for BlobStoreRepository
it's important to remove the dependency on IndexShard and reduce it to
Store and MapperService (in the snapshot case). This significantly reduces
the dependency footprint for Repository and allows unit-testing without starting
nodes or instantiate entire shard instances. This change deprecates the old
method signatures and adds a unit-test for FileRepository to show the advantage
of this change.
In addition, the unit-testing surfaced a bug where the internal file names that
are private to the repository were used in the recovery stats instead of the
target file names which makes it impossible to relate to the actual lucene files
in the recovery stats.

In order to simplify repository testing especially for BlobStoreRepository it's important to remove the dependency on IndexShard and reduce it to Store and MapperService (in the snapshot case). This significantly reduces the dependcy footprint for Repository and allows unittesting without starting nodes or instantiate entire shard instances. This change deprecates the old method signatures and adds a unittest for FileRepository to show the advantage of this change. In addition, the unittesting surfaced a bug where the internal file names that are private to the repository were used in the recovery stats instead of the target file names which makes it impossible to relate to the actual lucene files in the recovery stats.

original-brownbear · 2019-05-20T08:58:40Z

server/src/main/java/org/elasticsearch/repositories/Repository.java

+     * @param indexId             id for the index being snapshotted
+     * @param snapshotIndexCommit commit point
+     * @param snapshotStatus      snapshot status
+     * @deprecated use {@link #snapshotShard(Store, MapperService, SnapshotId, IndexId, IndexCommit, IndexShardSnapshotStatus)} instead


I wonder if we even need this deprecation (instead of just outright changing the interface)? We will need to make changes to this interface anyway to fix #41581 so maybe it's not worth being careful here when we have to break 3rd party implementations (not sure if there even are any) to fix functionality anyway?

Why not drop the remaining deprecations as well 4a1bd35 ? :)

well I can drop it in a followup but I want to backport this change to 7.x and there the deprecation is mandatory IMO

Interesting :) I don't wanna take this one off-topic but I have to ask:

If we don't want to break the Repository interface in 7.x that makes fixing #41581 in 7.x effectively impossible. We simply need to change what initialize snapshot and finalize snapshot do to get that one right.
So if we really can't break the contract for that one, we'd have to add new versions of initialize and finalize and keep the deprecated ones with broken behavior around?
(note that we also made a number of breaking changes to BlobStoreRepository itself lately and are planning to make even bigger ones in #42189. Those would break any 3rd party implementation of BlobStoreRepository as well. Should we tread more carefully there than planned?

I personally see this being a different reason. If you need to break an interface for the sake of fixing a bug or a correctness issue we should and can break it in a minor release. If we just factor stuff out to make it more testable I think being nice to a user is a wise decision. I will for sure remove the deprecated methods from master once it's backported.

elasticmachine · 2019-05-20T12:35:32Z

Pinging @elastic/es-distributed

original-brownbear

@s1monw Looks good, just a few random points.

Except for: we ran into a test failure here https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-2/13713/testReport/junit/org.elasticsearch.xpack.test.rest/XPackRestIT/test__p0_snapshot_10_basic_Create_a_source_only_snapshot_and_then_restore_it_/ that looks potentially related. I tried reproducing it with this branch but couldn't though.

java.lang.AssertionError: Failure at [snapshot/10_basic:70]: test_index.shards.0.index.files.recovered didn't match expected value:
test_index.shards.0.index.files.recovered: expected [5] but was [1]

	at __randomizedtesting.SeedInfo.seed([220AD778DD757CB1:AA5EE8A273891149]:0)
	at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.executeSection(ESClientYamlSuiteTestCase.java:393)
	at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.test(ESClientYamlSuiteTestCase.java:370)
	at jdk.internal.reflect.GeneratedMethodAccessor12.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1750)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:938)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:974)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:988)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:49)
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
	at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48)
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:817)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:468)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:947)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:832)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:883)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:894)
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:41)
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)
	at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:54)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
	at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.AssertionError: test_index.shards.0.index.files.recovered didn't match expected value:
test_index.shards.0.index.files.recovered: expected [5] but was [1]

REPRODUCE WITH: ./gradlew :x-pack:plugin:integTestRunner --tests "org.elasticsearch.xpack.test.rest.XPackRestIT.test {p0=snapshot/10_basic/Create a source only snapshot and then restore it}" -Dtests.seed=220AD778DD757CB1 -Dtests.security.manager=true -Dtests.locale=mfe-MU -Dtests.timezone=Pacific/Rarotonga -Dcompiler.java=12 -Druntime.java=11 -Dtests.rest.blacklist=getting_started/10_monitor_cluster_health/*

maybe you have an idea? It doesn't look like your changes caused this but maybe I'm missing something, just figured I'd raise it :)

x-pack/plugin/core/src/main/java/org/elasticsearch/snapshots/SourceOnlySnapshotRepository.java

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java

original-brownbear · 2019-05-20T15:49:58Z

server/src/main/java/org/elasticsearch/repositories/Repository.java

+     * @param indexId             id for the index being snapshotted
+     * @param snapshotIndexCommit commit point
+     * @param snapshotStatus      snapshot status
+     * @deprecated use {@link #snapshotShard(Store, MapperService, SnapshotId, IndexId, IndexCommit, IndexShardSnapshotStatus)} instead


Why not drop the remaining deprecations as well 4a1bd35 ? :)

s1monw · 2019-05-20T19:38:16Z

@original-brownbear I addressed your comments and fixed the test.

original-brownbear · 2019-05-20T19:57:34Z

Thanks @s1monw LGTM :) Any guidance on the off-topic question would be much appreciated still.

rjernst

LGTM too. I also don't see why we wouldn't break the ctor in 7.x, given only plugins would be affected, but I don't have any problem being nice to plugin developers with a deprecation period.

* Remove IndexShard dependency from Repository In order to simplify repository testing especially for BlobStoreRepository it's important to remove the dependency on IndexShard and reduce it to Store and MapperService (in the snapshot case). This significantly reduces the dependcy footprint for Repository and allows unittesting without starting nodes or instantiate entire shard instances. This change deprecates the old method signatures and adds a unittest for FileRepository to show the advantage of this change. In addition, the unittesting surfaced a bug where the internal file names that are private to the repository were used in the recovery stats instead of the target file names which makes it impossible to relate to the actual lucene files in the recovery stats. * don't delegate deprecated methods * apply comments * test

We deprecated `restoreShard` and `snapshotShard` in elastic#42213 This change removes the deprecated methods and their usage and adds a note in the migration docs.

We deprecated `restoreShard` and `snapshotShard` in #42213 This change removes the deprecated methods and their usage and adds a note in the migration docs.

* Remove IndexShard dependency from Repository In order to simplify repository testing especially for BlobStoreRepository it's important to remove the dependency on IndexShard and reduce it to Store and MapperService (in the snapshot case). This significantly reduces the dependcy footprint for Repository and allows unittesting without starting nodes or instantiate entire shard instances. This change deprecates the old method signatures and adds a unittest for FileRepository to show the advantage of this change. In addition, the unittesting surfaced a bug where the internal file names that are private to the repository were used in the recovery stats instead of the target file names which makes it impossible to relate to the actual lucene files in the recovery stats. * don't delegate deprecated methods * apply comments * test

We deprecated `restoreShard` and `snapshotShard` in elastic#42213 This change removes the deprecated methods and their usage and adds a note in the migration docs.

s1monw added 2 commits May 20, 2019 09:41

Merge branch 'master' into cleanup_snapshot_restore

e18f460

s1monw requested review from rjernst, Tim-Brooks and original-brownbear May 20, 2019 07:48

original-brownbear reviewed May 20, 2019

View reviewed changes

original-brownbear added the :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label May 20, 2019

don't delegate deprecated methods

dddebb4

original-brownbear requested changes May 20, 2019

View reviewed changes

s1monw added 3 commits May 20, 2019 21:25

apply comments

1db57b1

test

d6963cd

Merge branch 'master' into cleanup_snapshot_restore

bb2492c

original-brownbear approved these changes May 20, 2019

View reviewed changes

rjernst approved these changes May 20, 2019

View reviewed changes

s1monw added 4 commits May 21, 2019 07:43

Merge branch 'master' into cleanup_snapshot_restore

32daaa6

Merge branch 'master' into cleanup_snapshot_restore

e5caafe

Merge branch 'master' into cleanup_snapshot_restore

e6862cb

Merge branch 'master' into cleanup_snapshot_restore

de5d6c1

s1monw merged commit d228442 into elastic:master May 22, 2019

s1monw deleted the cleanup_snapshot_restore branch May 22, 2019 09:54

s1monw added >breaking-java >enhancement v7.2.0 v8.0.0 and removed >breaking-java labels May 22, 2019

s1monw mentioned this pull request May 22, 2019

Remove deprecated Repository methods #42359

Merged

s1monw added a commit that referenced this pull request May 23, 2019

Remove deprecated Repository methods (#42359)

cb40222

We deprecated `restoreShard` and `snapshotShard` in #42213 This change removes the deprecated methods and their usage and adds a note in the migration docs.

original-brownbear mentioned this pull request Jun 5, 2019

FsRepositoryTests#testSnapshotAndRestore reproducibly fails #42905

Closed

original-brownbear mentioned this pull request Jul 5, 2019

More Efficient Ordering of Shard Upload Execution #42791

Merged

original-brownbear mentioned this pull request Feb 20, 2020

Better Incrementality for Snapshots of Unchanged Shards #52182

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove IndexShard dependency from Repository #42213

Remove IndexShard dependency from Repository #42213

s1monw commented May 20, 2019

original-brownbear May 20, 2019 •

edited

Loading

original-brownbear May 20, 2019

s1monw May 20, 2019

original-brownbear May 20, 2019

s1monw May 21, 2019

elasticmachine commented May 20, 2019

original-brownbear left a comment

original-brownbear May 20, 2019

s1monw commented May 20, 2019

original-brownbear commented May 20, 2019

rjernst left a comment

Remove IndexShard dependency from Repository #42213

Remove IndexShard dependency from Repository #42213

Conversation

s1monw commented May 20, 2019

original-brownbear May 20, 2019 • edited Loading

Choose a reason for hiding this comment

original-brownbear May 20, 2019

Choose a reason for hiding this comment

s1monw May 20, 2019

Choose a reason for hiding this comment

original-brownbear May 20, 2019

Choose a reason for hiding this comment

s1monw May 21, 2019

Choose a reason for hiding this comment

elasticmachine commented May 20, 2019

original-brownbear left a comment

Choose a reason for hiding this comment

original-brownbear May 20, 2019

Choose a reason for hiding this comment

s1monw commented May 20, 2019

original-brownbear commented May 20, 2019

rjernst left a comment

Choose a reason for hiding this comment

original-brownbear May 20, 2019 •

edited

Loading