fix repository update with the same settings but different type #31458

vladimirdolzhenko · 2018-06-20T11:01:27Z

No description provided.

elasticmachine · 2018-06-20T11:01:28Z

Pinging @elastic/es-distributed

tlrx · 2018-06-20T11:09:55Z

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

@@ -349,7 +349,7 @@ private boolean registerRepository(RepositoryMetaData repositoryMetaData) throws
        Repository previous = repositories.get(repositoryMetaData.name());
        if (previous != null) {
            RepositoryMetaData previousMetadata = previous.getMetadata();
-            if (!previousMetadata.type().equals(repositoryMetaData.type()) && previousMetadata.settings().equals(repositoryMetaData.settings())) {
+            if (previousMetadata.type().equals(repositoryMetaData.type()) && previousMetadata.settings().equals(repositoryMetaData.settings())) {


Maybe we can just use RepositoryMetaData.equals() here?

tlrx · 2018-06-20T11:10:35Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java

+        return Collections.singletonList(TestFSRepositoryPlugin.class);
+    }
+
+    public static class TestFSRepositoryPlugin extends Plugin implements RepositoryPlugin {


You can use MockRepository.Plugin instead of adding a new type of repository

tlrx

I left some comments

tlrx · 2018-06-20T11:11:37Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java

+
+        Client client = client();
+        Settings settings = ((InternalTestCluster) cluster()).getDefaultSettings();
+        Path location = randomRepoPath(settings);


Why not using randomPath() directly?

tlrx · 2018-06-20T11:14:12Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java

+        assertThat(getRepositoriesResponse2.repositories(), hasSize(1));
+        RepositoryMetaData repositoryMetaData2 = getRepositoriesResponse2.repositories().get(0);
+
+        assertThat(repositoryMetaData2.type(), equalTo(FAKE_FS_TYPE));


I'm wondering if we should also check the Repository instance itself using internalCluster().getInstance(RepositoriesService.class).repository(name) and check if the instance is the same or has changed.

agree - make sense to add that kind of check

…est case

… randomization

vladimirdolzhenko · 2018-06-20T14:12:42Z

@tlrx thanks for your comments, could you pls have another look ?

tlrx

I left more minor comments, thanks @vladimirdolzhenko for fixing this!

tlrx · 2018-06-20T14:32:20Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+                .setType(FsRepository.TYPE)
+                .setSettings(repoSettings)
+                .get();
+        assertThat(putRepositoryResponse1.isAcknowledged(), equalTo(true));


You can use assertAcked(client.admin().cluster().preparePutRepository(repositoryName)
.setType(FsRepository.TYPE)
.setSettings(repoSettings))

tlrx · 2018-06-20T14:34:00Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+
+        // update repository
+
+        boolean diffType = randomBoolean();


Maybe we could make this more readable with:
final String updatedRepositoryType = randomBoolean() ? "mock" : FsRepository.TYPE

?

tlrx · 2018-06-20T14:36:27Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+                .setType(diffType ? "mock" : FsRepository.TYPE)
+                .setSettings(repoSettings)
+                .get();
+        assertThat(putRepositoryResponse2.isAcknowledged(), equalTo(true));


You can use assertAcked()

tlrx · 2018-06-20T14:39:00Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+        assertThat(repositoryMetaData2.type(), equalTo(diffType ? "mock" : FsRepository.TYPE));
+
+        Repository repository2 = repositoriesService.repository(repositoryName);
+        assertThat(repository2, instanceOf(diffType ? MockRepository.class : FsRepository.class));


I don't think we need to check the implementation class, instance should be enough.

tlrx · 2018-06-20T14:39:29Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+
+        assertThat(repositoryMetaData2.type(), equalTo(diffType ? "mock" : FsRepository.TYPE));
+
+        Repository repository2 = repositoriesService.repository(repositoryName);


Can you use more explicit name? (Also for getRepositoriesResponse1/2).

vladimirdolzhenko · 2018-06-20T18:36:42Z

test this please

tlrx

Another bunch of minor comments, sorry I didn't catch everything the first times.

tlrx · 2018-06-21T07:10:40Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+
+        Client client = client();
+        InternalTestCluster cluster = (InternalTestCluster) cluster();
+        RepositoriesService repositoriesService = cluster.getInstance(RepositoriesService.class);


Can you make this final? Also, I think you should use getDataOrMasterNodeInstances as the repository is only registered on master eligible and data nodes. The method getInstance() retrieves the instance from a random node and if it's not a data or master one it will not find the repository.

tlrx · 2018-06-21T07:11:40Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+        InternalTestCluster cluster = (InternalTestCluster) cluster();
+        RepositoriesService repositoriesService = cluster.getInstance(RepositoriesService.class);
+        Settings settings = cluster.getDefaultSettings();
+        Path location = randomRepoPath();


If it's not used afterwards, maybe we don't need a ref to the location. We could simply use .put("location", randomRepoPath())

tlrx · 2018-06-21T07:11:51Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+                .setSettings(repoSettings)
+                .get());
+
+        GetRepositoriesResponse originalRetRepositoriesResponse =


Nit: typo in originalRetRepositoriesResponse

tlrx · 2018-06-21T07:12:06Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+    }
+
+    public void testUpdateRepository() {
+        String repositoryName = "test-repo";


Can you make it final?

tlrx · 2018-06-21T07:12:30Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+        // update repository
+
+        boolean updated = randomBoolean();
+        String updatedRepositoryType = updated ? "mock" : FsRepository.TYPE;


updated and updatedRepositoryType can be final too

vladimirdolzhenko · 2018-06-21T12:45:48Z

test this please

vladimirdolzhenko · 2018-06-21T14:43:04Z

test this please

…date_diff_type

tlrx

LGTM - I left a more comments that I'd like to be addressed before merging but I don't think that it will require another review

tlrx · 2018-06-22T13:12:53Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+            cluster.getDataOrMasterNodeInstances(RepositoriesService.class).iterator().next();
+        final Settings settings = cluster.getDefaultSettings();
+
+        final Settings.Builder repoSettings = Settings.builder().put(settings).put("location", randomRepoPath());


You don't need to pass the default cluster settings here but location is enough for a FS repository

tlrx · 2018-06-22T13:14:27Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+        final Repository originalRepository = repositoriesService.repository(repositoryName);
+        assertThat(originalRepository, instanceOf(FsRepository.class));
+
+        // update repository


I think you can remove this comment

tlrx · 2018-06-22T13:16:07Z

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceIT.java

+    }
+
+    public void testUpdateRepository() throws Exception {
+        waitNoPendingTasksOnAll();


Thinking about it twice, I don't think that this is necessary. I think you can remove it, run the test multiple times andsee if it fails or not.

vladimirdolzhenko · 2018-06-22T13:27:18Z

thanks @tlrx for your comments

* master: Add get field mappings to High Level REST API Client (#31423) [DOCS] Updates Watcher examples for code testing (#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (#31474) [DOCS] Move monitoring to docs folder (#31477) Core: Combine doExecute methods in TransportAction (#31517) IndexShard should not return null stats (#31528) fix repository update with the same settings but different type (#31458) Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527) Node selector per client rather than per request (#31471) Core: Combine messageRecieved methods in TransportRequestHandler (#31519) Upgrade to Lucene 7.4.0. (#31529) [ML] Add ML filter update API (#31437) Allow multiple unicast host providers (#31509) Avoid deprecation warning when running the ML datafeed extractor. (#31463) REST high-level client: add simulate pipeline API (#31158) Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507) [PkiRealm] Invalidate cache on role mappings change (#31510) [Security] Check auth scheme case insensitively (#31490) In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514) [DOCS] Fix REST tests in SQL docs [DOCS] Add code snippet testing in more ML APIs (#31339) Core: Remove ThreadPool from base TransportAction (#31492) [DOCS] Remove fixed file from build.gradle Rename createNewTranslog to fileBasedRecovery (#31508) Test: Skip assertion on windows [DOCS] Creates field and document level security overview (#30937) [DOCS] Significantly improve SQL docs [DOCS] Move migration APIs to docs (#31473) Core: Convert TransportAction.execute uses to client calls (#31487) Return transport addresses from UnicastHostsProvider (#31426) Ensure local addresses aren't null (#31440) Remove unused generic type for client execute method (#31444) Introduce http and tcp server channels (#31446)

* elastic/master: (92 commits) Reduce number of raw types warnings (elastic#31523) Migrate scripted metric aggregation scripts to ScriptContext design (elastic#30111) turn GetFieldMappingsResponse to ToXContentObject (elastic#31544) Close xcontent parsers (partial) (elastic#31513) Ingest Attachment: Upgrade Tika to 1.18 (elastic#31252) TEST: Correct the assertion arguments order (elastic#31540) Add get field mappings to High Level REST API Client (elastic#31423) [DOCS] Updates Watcher examples for code testing (elastic#31152) TEST: Add bwc recovery tests with synced-flush index [DOCS] Move sql to docs (elastic#31474) [DOCS] Move monitoring to docs folder (elastic#31477) Core: Combine doExecute methods in TransportAction (elastic#31517) IndexShard should not return null stats (elastic#31528) fix repository update with the same settings but different type (elastic#31458) Fix Mockito trying to mock IOException that isn't thrown by method (elastic#31433) (elastic#31527) Node selector per client rather than per request (elastic#31471) Core: Combine messageRecieved methods in TransportRequestHandler (elastic#31519) Upgrade to Lucene 7.4.0. (elastic#31529) [ML] Add ML filter update API (elastic#31437) Allow multiple unicast host providers (elastic#31509) ...

fix repository update with the same settings but different type

b20fa77

vladimirdolzhenko added >bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 v6.4.0 labels Jun 20, 2018

vladimirdolzhenko assigned tlrx Jun 20, 2018

vladimirdolzhenko assigned vladimirdolzhenko and unassigned tlrx Jun 20, 2018

vladimirdolzhenko requested a review from tlrx June 20, 2018 11:01

fix comment

fc5e94d

tlrx reviewed Jun 20, 2018

View reviewed changes

Vladimir Dolzhenko added 2 commits June 20, 2018 13:31

use equals instead of check each property of repo metadata; improve t…

3f606bd

…est case

integration test has to be IT; added check for the instance; add some…

c3dd0f6

… randomization

vladimirdolzhenko requested a review from tlrx June 20, 2018 14:12

tlrx reviewed Jun 20, 2018

View reviewed changes

minor test case enhancements

a857d2b

tlrx reviewed Jun 21, 2018

View reviewed changes

make variables final; explicit start a single master node

8f7eb41

Merge remote-tracking branch 'remotes/origin/master' into fix_repo_up…

bd44fe4

…date_diff_type

vladimirdolzhenko requested a review from tlrx June 21, 2018 17:19

do not stuck with a single master case

f7d1a30

tlrx approved these changes Jun 22, 2018

View reviewed changes

final polish

4fd7576

vladimirdolzhenko merged commit 7313a98 into elastic:master Jun 22, 2018

vladimirdolzhenko deleted the fix_repo_update_diff_type branch June 22, 2018 15:44

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

fix repository update with the same settings but different type #31458

fix repository update with the same settings but different type #31458

vladimirdolzhenko commented Jun 20, 2018

elasticmachine commented Jun 20, 2018

tlrx Jun 20, 2018

vladimirdolzhenko Jun 20, 2018

tlrx Jun 20, 2018

tlrx left a comment

tlrx Jun 20, 2018

tlrx Jun 20, 2018

vladimirdolzhenko Jun 20, 2018

vladimirdolzhenko commented Jun 20, 2018

tlrx left a comment

tlrx Jun 20, 2018

tlrx Jun 20, 2018

tlrx Jun 20, 2018

tlrx Jun 20, 2018

tlrx Jun 20, 2018

vladimirdolzhenko commented Jun 20, 2018

tlrx left a comment

tlrx Jun 21, 2018

tlrx Jun 21, 2018

tlrx Jun 21, 2018

tlrx Jun 21, 2018

tlrx Jun 21, 2018

vladimirdolzhenko commented Jun 21, 2018

vladimirdolzhenko commented Jun 21, 2018

tlrx left a comment

tlrx Jun 22, 2018

tlrx Jun 22, 2018

tlrx Jun 22, 2018

vladimirdolzhenko commented Jun 22, 2018


		assertThat(repositoryMetaData2.type(), equalTo(diffType ? "mock" : FsRepository.TYPE));

		Repository repository2 = repositoriesService.repository(repositoryName);

fix repository update with the same settings but different type #31458

fix repository update with the same settings but different type #31458

Conversation

vladimirdolzhenko commented Jun 20, 2018

elasticmachine commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx 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

vladimirdolzhenko commented Jun 20, 2018

tlrx 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

vladimirdolzhenko commented Jun 20, 2018

tlrx 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

vladimirdolzhenko commented Jun 21, 2018

vladimirdolzhenko commented Jun 21, 2018

tlrx 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

vladimirdolzhenko commented Jun 22, 2018