Change signature of Get Repositories Response #30333

hub-cap · 2018-05-02T12:20:58Z

The Get Repositories response object held a list of RepositoryMetaData
entries. This object does not have the from/toXContent methods that are
needed to expose this to the high level REST client. The
RepositoriesMetaData, however, does, and it also contains a list of
RepositoryMetaData objects within it. So rather than duplicate this
logic or move it (RepositoriesMetaData is a fragment object used by
cluster state), the object holding state in the Response was changed to
use the RepositoriesMetaData instead. This also cleans up the read/write
methods in the response, as they can now use the same read/write in
RepositoriesMetaData, which also were not present in the singular class.

The Get Repositories response object held a list of RepositoryMetaData entries. This object does not have the from/toXContent methods that are needed to expose this to the high level REST client. The RepositoriesMetaData, however, does, and it also contains a list of RepositoryMetaData objects within it. So rather than duplicate this logic or move it (RepositoriesMetaData is a fragment object used by cluster state), the object holding state in the Response was changed to use the RepositoriesMetaData instead. This also cleans up the read/write methods in the response, as they can now use the same read/write in RepositoriesMetaData, which also were not present in the singular class.

hub-cap · 2018-05-02T16:52:00Z

Pinging @elastic/es-core-infra

rjernst

I left a couple suggestions

rjernst · 2018-05-02T16:58:54Z

.../org/elasticsearch/action/admin/cluster/repositories/get/TransportGetRepositoriesAction.java

@@ -75,9 +75,9 @@ protected void masterOperation(final GetRepositoriesRequest request, ClusterStat
        RepositoriesMetaData repositories = metaData.custom(RepositoriesMetaData.TYPE);
        if (request.repositories().length == 0 || (request.repositories().length == 1 && "_all".equals(request.repositories()[0]))) {
            if (repositories != null) {
-                listener.onResponse(new GetRepositoriesResponse(repositories.repositories()));
+                listener.onResponse(new GetRepositoriesResponse(new RepositoriesMetaData(repositories.repositories())));


Why is a new RepositoriesMetaData created instead of using the existing?

wow that was silly. good catch.

rjernst · 2018-05-02T17:00:36Z

.../org/elasticsearch/action/admin/cluster/repositories/get/TransportGetRepositoriesAction.java

            } else {
-                listener.onResponse(new GetRepositoriesResponse(Collections.<RepositoryMetaData>emptyList()));
+                listener.onResponse(new GetRepositoriesResponse(new RepositoriesMetaData(Collections.<RepositoryMetaData>emptyList())));


Is the explicit generic needed? It should be inferred by the compiler?

rjernst · 2018-05-02T17:01:17Z

.../org/elasticsearch/action/admin/cluster/repositories/get/TransportGetRepositoriesAction.java

@@ -102,7 +102,7 @@ protected void masterOperation(final GetRepositoriesRequest request, ClusterStat
                    }
                    repositoryListBuilder.add(repositoryMetaData);
                }
-                listener.onResponse(new GetRepositoriesResponse(Collections.unmodifiableList(repositoryListBuilder)));
+                listener.onResponse(new GetRepositoriesResponse(new RepositoriesMetaData((repositoryListBuilder))));


nit: there is an extra set of parens

rjernst · 2018-05-02T17:04:52Z

server/src/main/java/org/elasticsearch/rest/action/cat/RestRepositoriesAction.java

@@ -79,7 +79,7 @@ protected Table getTableWithHeader(RestRequest request) {

    private Table buildTable(RestRequest req, GetRepositoriesResponse getRepositoriesResponse) {
        Table table = getTableWithHeader(req);
-        for (RepositoryMetaData repositoryMetaData : getRepositoriesResponse.repositories()) {
+        for (RepositoryMetaData repositoryMetaData : getRepositoriesResponse.repositories().repositories()) {


repositories().repositories() looks very confusing. Maybe the GetRepositoriesResponse.repositories() should continue to return the List, internally calling repositories.repositories()?

This was my only struggle. Everything else exposes the actual object (the constructor, etc) and so i wanted to keep it the same. My other thought was to actually use the Iterator that it was (see the removal of that in the review) and just remove both of the repositories().repositories(). its the only way this class is really used anyway. Thoughts @rjernst ?

Using the iterator sounds fine.

There were a bunch of other uses in tests to check the size / contents, as a list, so i decided to revert the repositories() return value. having access to the RepositoriesMetaData is not that useful as a consumer of this call anyway.

rjernst

LGTM, one last question.

rjernst · 2018-05-02T18:44:16Z

...in/java/org/elasticsearch/action/admin/cluster/repositories/get/GetRepositoriesResponse.java

@@ -52,43 +53,25 @@
     * @return list or repositories
     */
    public List<RepositoryMetaData> repositories() {


I thought the idea was to return Iterable<RepositoryMetaData> here?

no, the way the code was before this change was the class implements Iterable<RepositoryMetaData> , so i was considering using that, but it only solved 1 case, and there were still many cases where repositories().repositories().size() was used in tests, and some other cases where the iterator approach would work. Regardless of how it was implemented, we need to get the size of the list in some tests, so i figure it best to just return the list itself.

That being said, we should probably consider making the List inside the RepositoriesMetaData a unmodifiable List unless it has to be otherwise. I dont see any indication that it needs to be otherwise modifyable.

Please do make it unmodifiable!

Coming in a following PR

for posterity that other pr was #30361

…sponse

This commit makes the RepositoriesMetaData backing list no longer modifiable. Ref elastic#30333

javanna · 2018-05-03T12:33:41Z

Great change @hub-cap , thanks!

This commit makes the RepositoriesMetaData backing list no longer modifiable. Ref #30333

The Get Repositories response object held a list of RepositoryMetaData entries. This object does not have the from/toXContent methods that are needed to expose this to the high level REST client. The RepositoriesMetaData, however, does, and it also contains a list of RepositoryMetaData objects within it. So rather than duplicate this logic or move it (RepositoriesMetaData is a fragment object used by cluster state), the object holding state in the Response was changed to use the RepositoriesMetaData instead. This also cleans up the read/write methods in the response, as they can now use the same read/write in RepositoriesMetaData, which also were not present in the singular class.

This commit makes the RepositoriesMetaData backing list no longer modifiable. Ref #30333

* master: Set the new lucene version for 6.4.0 [ML][TEST] Clean up jobs in ModelPlotIT Upgrade to 7.4.0-snapshot-1ed95c097b (#30357) Watcher: Ensure trigger service pauses execution (#30363) [DOCS] Added coming qualifiers in changelog [DOCS] Commented out empty sections in the changelog to fix the doc build. (#30372) Security: reduce garbage during index resolution (#30180) Make RepositoriesMetaData contents unmodifiable (#30361) Change quad tree max levels to 29. Closes #21191 (#29663) 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) InternalEngineTests.testConcurrentOutOfOrderDocsOnReplica should use two documents (#30121) Change signature of Get Repositories Response (#30333) Tests: Use different watch ids per test in smoke test (#30331) [Docs] Add term query with normalizer example Adds Eclipse config for xpack licence headers (#30299) Watcher: Make start/stop cycle more predictable and synchronous (#30118) [test] add debug logging for packaging test [DOCS] Removed X-Pack Breaking Changes [DOCS] Fixes link to TLS LDAP info Update versions for start_trial after backport (#30218) Packaging: Set elasticsearch user to have non-existent homedir (#29007) [DOCS] Fixes broken links to bootstrap user (#30349) Fix NPE when CumulativeSum agg encounters null/empty bucket (#29641) Make licensing FIPS-140 compliant (#30251) [DOCS] Reorganizes authentication details in Stack Overview (#30280) Network: Remove http.enabled setting (#29601) Fix merging logic of Suggester Options (#29514) [DOCS] Adds LDAP realm configuration details (#30214) [DOCS] Adds native realm configuration details (#30215) ReplicationTracker.markAllocationIdAsInSync may hang if allocation is cancelled (#30316) [DOCS] Enables edit links for X-Pack pages (#30278) Packaging: Unmark systemd service file as a config file (#29004) SQL: Reduce number of ranges generated for comparisons (#30267) Tests: Simplify VersionUtils released version splitting (#30322) Cancelling a peer recovery on the source can leak a primary permit (#30318) Added changelog entry for deb prerelease version change (#30184) Convert server javadoc to html5 (#30279) Create default ES_TMPDIR on Windows (#30325) [Docs] Clarify `fuzzy_like_this` redirect (#30183) Post backport of #29658. Fix docs of the `_ignored` meta field. Remove MapperService#types(). (#29617) Remove useless version checks in REST tests. (#30165) Add a new `_ignored` meta field. (#29658) Move repository-azure fixture test to QA project (#30253) # Conflicts: # buildSrc/version.properties # server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

* origin/ccr: (166 commits) Introduce soft-deletes retention policy based on global checkpoint (elastic#30335) Enable MockHttpTransport in ShardChangsIT Remove old sha files from dated Lucene snapshot Update InternalEngine tests on ccr side for elastic#30121 Set the new lucene version for 6.4.0 [ML][TEST] Clean up jobs in ModelPlotIT Upgrade to 7.4.0-snapshot-1ed95c097b (elastic#30357) Watcher: Ensure trigger service pauses execution (elastic#30363) [DOCS] Added coming qualifiers in changelog [DOCS] Commented out empty sections in the changelog to fix the doc build. (elastic#30372) Security: reduce garbage during index resolution (elastic#30180) Make RepositoriesMetaData contents unmodifiable (elastic#30361) Change quad tree max levels to 29. Closes elastic#21191 (elastic#29663) Test: use trial license in qa tests with security [ML] Add integration test for model plots (elastic#30359) SQL: Fix bug caused by empty composites (elastic#30343) [ML] Account for gaps in data counts after job is reopened (elastic#30294) InternalEngineTests.testConcurrentOutOfOrderDocsOnReplica should use two documents (elastic#30121) Change signature of Get Repositories Response (elastic#30333) Tests: Use different watch ids per test in smoke test (elastic#30331) ...

* origin/ccr: (127 commits) Introduce soft-deletes retention policy based on global checkpoint (elastic#30335) Enable MockHttpTransport in ShardChangsIT Remove old sha files from dated Lucene snapshot Update InternalEngine tests on ccr side for elastic#30121 Set the new lucene version for 6.4.0 [ML][TEST] Clean up jobs in ModelPlotIT Upgrade to 7.4.0-snapshot-1ed95c097b (elastic#30357) Watcher: Ensure trigger service pauses execution (elastic#30363) [DOCS] Added coming qualifiers in changelog [DOCS] Commented out empty sections in the changelog to fix the doc build. (elastic#30372) Security: reduce garbage during index resolution (elastic#30180) Make RepositoriesMetaData contents unmodifiable (elastic#30361) Change quad tree max levels to 29. Closes elastic#21191 (elastic#29663) Test: use trial license in qa tests with security [ML] Add integration test for model plots (elastic#30359) SQL: Fix bug caused by empty composites (elastic#30343) [ML] Account for gaps in data counts after job is reopened (elastic#30294) InternalEngineTests.testConcurrentOutOfOrderDocsOnReplica should use two documents (elastic#30121) Change signature of Get Repositories Response (elastic#30333) Tests: Use different watch ids per test in smoke test (elastic#30331) ...

* origin/ccr: (166 commits) Introduce soft-deletes retention policy based on global checkpoint (elastic#30335) Enable MockHttpTransport in ShardChangsIT Remove old sha files from dated Lucene snapshot Update InternalEngine tests on ccr side for elastic#30121 Set the new lucene version for 6.4.0 [ML][TEST] Clean up jobs in ModelPlotIT Upgrade to 7.4.0-snapshot-1ed95c097b (elastic#30357) Watcher: Ensure trigger service pauses execution (elastic#30363) [DOCS] Added coming qualifiers in changelog [DOCS] Commented out empty sections in the changelog to fix the doc build. (elastic#30372) Security: reduce garbage during index resolution (elastic#30180) Make RepositoriesMetaData contents unmodifiable (elastic#30361) Change quad tree max levels to 29. Closes elastic#21191 (elastic#29663) Test: use trial license in qa tests with security [ML] Add integration test for model plots (elastic#30359) SQL: Fix bug caused by empty composites (elastic#30343) [ML] Account for gaps in data counts after job is reopened (elastic#30294) InternalEngineTests.testConcurrentOutOfOrderDocsOnReplica should use two documents (elastic#30121) Change signature of Get Repositories Response (elastic#30333) Tests: Use different watch ids per test in smoke test (elastic#30331) ...

* 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)

hub-cap added >non-issue v7.0.0 :Core/Java High Level REST Client >refactoring v6.4.0 and removed :Core/Java High Level REST Client labels May 2, 2018

Fix precommit

ee6ba37

hub-cap added :Core/Java High Level REST Client and removed :Core/Java High Level REST Client labels May 2, 2018

rjernst reviewed May 2, 2018

View reviewed changes

Cleanup signature of repositories()

9f268f9

hub-cap requested a review from rjernst May 2, 2018 18:26

rjernst approved these changes May 2, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into repository_fix_re…

caffca8

…sponse

hub-cap merged commit bdd43fa into elastic:master May 3, 2018

hub-cap added a commit to hub-cap/elasticsearch that referenced this pull request May 3, 2018

Make RepositoriesMetaData contents unmodifiable

ce08f6f

This commit makes the RepositoriesMetaData backing list no longer modifiable. Ref elastic#30333

hub-cap added the backport pending label May 3, 2018

hub-cap mentioned this pull request May 3, 2018

Make RepositoriesMetaData contents unmodifiable #30361

Merged

hub-cap added a commit that referenced this pull request May 3, 2018

Make RepositoriesMetaData contents unmodifiable (#30361)

5f8101a

This commit makes the RepositoriesMetaData backing list no longer modifiable. Ref #30333

hub-cap added a commit that referenced this pull request May 3, 2018

Make RepositoriesMetaData contents unmodifiable (#30361)

32dcaad

This commit makes the RepositoriesMetaData backing list no longer modifiable. Ref #30333

hub-cap removed the backport pending label May 3, 2018

hub-cap deleted the repository_fix_response branch May 3, 2018 20:55

jimczi 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

Change signature of Get Repositories Response #30333

Change signature of Get Repositories Response #30333

hub-cap commented May 2, 2018

hub-cap commented May 2, 2018

rjernst left a comment

rjernst May 2, 2018

hub-cap May 2, 2018

rjernst May 2, 2018

rjernst May 2, 2018

rjernst May 2, 2018

hub-cap May 2, 2018

rjernst May 2, 2018

hub-cap May 2, 2018

rjernst left a comment

rjernst May 2, 2018 •

edited

Loading

hub-cap May 2, 2018

rjernst May 2, 2018

hub-cap May 3, 2018

hub-cap May 3, 2018

javanna commented May 3, 2018

Change signature of Get Repositories Response #30333

Change signature of Get Repositories Response #30333

Conversation

hub-cap commented May 2, 2018

hub-cap commented May 2, 2018

rjernst 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

rjernst May 2, 2018 • edited Loading

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

javanna commented May 3, 2018

rjernst May 2, 2018 •

edited

Loading