fix repository update with the same settings but different type by vladimirdolzhenko · Pull Request #31458 · elastic/elasticsearch

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

        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


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

		Repository repository2 = repositoriesService.repository(repositoryName);

Conversation

vladimirdolzhenko commented Jun 20, 2018

Uh oh!

elasticmachine commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirdolzhenko commented Jun 20, 2018

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirdolzhenko commented Jun 20, 2018

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirdolzhenko commented Jun 21, 2018

Uh oh!

vladimirdolzhenko commented Jun 21, 2018

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirdolzhenko commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants