Modify state of VerifyRepositoryResponse for bwc #30762

hub-cap · 2018-05-21T18:42:22Z

The VerifyRepositoryResponse class holds a DiscoveryNode[], but the
nodes themselves are not serialized to a REST API consumer. Since we do
not want to put all of a DiscoveryNode over the wire, be it REST or
Transport since its unused, this change introduces a BWC compatible
change in ser/deser of the Response. Anything 6.4 and above will
read/write a NodeView, and anything prior will read/write a
DiscoveryNode. Further changes to 7.0 will be introduced to remove the
BWC shim and only read/write NodeView, and hold a List as the
VerifyRepositoryResponse internal state.

The VerifyRepositoryResponse class holds a DiscoveryNode[], but the nodes themselves are not serialized to a REST API consumer. Since we do not want to put all of a DiscoveryNode over the wire, be it REST or Transport since its unused, this change introduces a BWC compatible change in ser/deser of the Response. Anything 6.4 and above will read/write a NodeView, and anything prior will read/write a DiscoveryNode. Further changes to 7.0 will be introduced to remove the BWC shim and only read/write NodeView, and hold a List<NodeView> as the VerifyRepositoryResponse internal state.

elasticmachine · 2018-05-21T18:42:25Z

Pinging @elastic/es-core-infra

hub-cap · 2018-05-21T18:42:59Z

as per our conversations around #30662 (comment)

nik9000 · 2018-05-21T19:17:22Z

...ava/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java

+        static {
+            ObjectParser<NodeView, Void> parser = new ObjectParser<>("nodes");
+            parser.declareString(NodeView::setName, new ParseField(Fields.NAME));
+            PARSER = (XContentParser p, Void v, String name )-> parser.parse(p, new NodeView(name), null);


Can you replace ame )-> with ame) ->?

You could probably also get away with not having the types here too if you want.

nik9000 · 2018-05-21T19:18:28Z

...ava/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java

+        }
+
+        @Override
+        public void writeTo(StreamOutput out) throws IOException {


Can you move this directly under the reading ctor? I like having them close together so you can see that they are the same more easily.

nik9000 · 2018-05-21T19:23:23Z

...ava/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java

+
+        /**
+         * Temporary method that allows turning a {@link NodeView} into a {@link DiscoveryNode}. This will never be used in practice, but
+         * will be used to represent a the former as the latter in the {@link VerifyRepositoryResponse}. Effectively this will be used to


I'm not sure I'd say it is never used in practice because it is used in 6.x. I'm certainly +1 on leaving a big chunk of javadoc explaining when and why we call the method.

…o_sig

hub-cap · 2018-05-21T20:08:00Z

@nik9000 comments addressed, <3

nik9000 · 2018-05-21T20:11:47Z

Thanks! LGTM.

hub-cap · 2018-05-21T20:50:06Z

I also have a local CI full check running against 6.x to make sure this does not cause issue there.

jasontedor

Looks good, I left a few comments.

jasontedor · 2018-05-22T01:01:13Z

...ava/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java

+    public static class NodeView implements Writeable, ToXContentObject {
+        private static final ObjectParser.NamedObjectParser<NodeView, Void> PARSER;
+        static {
+            ObjectParser<NodeView, Void> parser = new ObjectParser<>("nodes");


I think it's a little odd that we call this parser here and it's not the same type as PARSER. A standard construct in class initializers is something like:

private static final Map<String, String> MAP; static { final Map<String, String> map = new HashMap<>(); map.put("foo", "bar"); MAP = Collections.unmodifiableMap(map); }

So we use the same name (in lowercase) to build up the immutable object that we are initializing. Since we are not using that pattern here, and parser and PARSER are of different types, I think that they should have different names.

jasontedor · 2018-05-22T01:01:56Z

...ava/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java

+        }
+
+        public NodeView(StreamInput in) throws IOException {
+            this(in.readString());


Why not this(in.readString(), in.readString());?

heh cuz i added the 2 string constructor after i added the StreamInput one, good catch :)

jasontedor · 2018-05-22T01:04:19Z

...ava/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java

+        public String getNodeId() { return nodeId; }
+
+        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+            builder.startObject(nodeId);


Lisa needs braces!

heh, i had actually added this locally after our last convo about it, but forgot to pull it out of a stash when i redid this as a separate PR :rip:

jasontedor · 2018-05-22T01:05:12Z

...ava/org/elasticsearch/action/admin/cluster/repositories/verify/VerifyRepositoryResponse.java

-        nodes = new DiscoveryNode[in.readVInt()];
-        for (int i=0; i<nodes.length; i++){
-            nodes[i] = new DiscoveryNode(in);
+        if (in.getVersion().onOrAfter(Version.V_6_4_0)) {


Typically when we make a change like this in master we would set the version to 7.0.0 as otherwise it will be impossible to have a green CI build here since 6.x (6.4) does not have this code yet. Then when you backport you will flip the version to 6.4.0 and push a commit to master to change the version to 6.4.0.

oh i wonder if this is why my bwc tests are failing!

Almost surely. 😇

…o_sig

The BWC version was previously at 7.0, because the 6.x backport had not yet landed. Now that it has landed, this commit replaces the BWC compat with the real version, 6.4.0. Relates elastic#30762

* master: QA: Add xpack tests to rolling upgrade (#30795) Modify state of VerifyRepositoryResponse for bwc (#30762) Reduce CLI scripts to one-liners on Windows (#30772) Simplify number of shards setting (#30783) Replace Request#setHeaders with addHeader (#30588) [TEST] remove endless wait in RestClientTests (#30776) [Docs] Fix script-fields snippet execution (#30693) Upgrade to Lucene-7.4.0-snapshot-cc2ee23050 (#30778) [DOCS] Add SAML configuration information (#30548) [DOCS] Remove X-Pack references from SQL CLI (#30694) Make http pipelining support mandatory (#30695) [Docs] Fix typo in circuit breaker docs (#29659) [Feature] Adding a char_group tokenizer (#24186) [Docs] Fix broken cross link in documentation Test: wait for netty threads in a JUnit ClassRule (#30763) Increase the maximum number of filters that may be in the cache. (#30655) [Security] Include an empty json object in an json array when FLS filters out all fields (#30709) [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk Add more yaml tests for get alias API (#29513) Ignore empty completion input (#30713) [DOCS] fixed incorrect default [ML] Filter undefined job groups from update calendar actions (#30757) Fix docs failure on language analyzers (#30722) [Docs] Fix inconsistencies in snapshot/restore doc (#30480) Enable installing plugins from snapshots.elastic.co (#30765) Remove fedora 26, add 28 (#30683) Accept Gradle build scan agreement (#30645) Remove logging from elasticsearch-nio jar (#30761) Add Delete Repository High Level REST API (#30666)

The VerifyRepositoryResponse class holds a DiscoveryNode[], but the nodes themselves are not serialized to a REST API consumer. Since we do not want to put all of a DiscoveryNode over the wire, be it REST or Transport since its unused, this change introduces a BWC compatible change in ser/deser of the Response. Anything 6.4 and above will read/write a NodeView, and anything prior will read/write a DiscoveryNode. Further changes to 7.0 will be introduced to remove the BWC shim and only read/write NodeView, and hold a List<NodeView> as the VerifyRepositoryResponse internal state.

* es/6.x: Move score script context from SearchScript to its own class (#30816) Fix bad version check writing Repository nodes (#30846) Modify state of VerifyRepositoryResponse for bwc (#30762) QA: Fix tribe tests when running default zip Use remote client in TransportFieldCapsAction (#30838) Mute IndexMasterFailoverIT.testMasterFailoverDuringIndexingWithMappingChanges Ensure that ip_range aggregations always return bucket keys. (#30701) Limit user to single concurrent auth per realm (#30794) Security: fix dynamic mapping updates with aliases (#30787) [Tests] Move templated _rank_eval tests (#30679) Move Watcher versioning setting to meta field (#30832) Restore "Add more yaml tests for get alias API " (#30814) Send client headers from TransportClient (#30803) [Docs] Explain incomplete dates in range queries (#30689) Move persistent task registrations to core (#30755) Decouple ClusterStateTaskListener & ClusterApplier (#30809) Packaging: Ensure upgrade_is_oss flag file is always deleted (#30732) Force stable file modes for built packages (#30823)

The BWC version was previously at 7.0, because the 6.x backport had not yet landed. Now that it has landed, this commit replaces the BWC compat with the real version, 6.4.0. Relates #30762

hub-cap added >enhancement :Core/Infra/Transport API Transport client API v7.0.0 v6.4.0 labels May 21, 2018

hub-cap requested review from nik9000 and jasontedor May 21, 2018 18:42

hub-cap mentioned this pull request May 21, 2018

Add Verify Repository High Level REST API #30662

Closed

nik9000 approved these changes May 21, 2018

View reviewed changes

hub-cap added 2 commits May 21, 2018 15:04

Fixups as per review

7761c26

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

9b0967d

…o_sig

Fix the assignment on nodes

6790509

jasontedor reviewed May 22, 2018

View reviewed changes

hub-cap added 4 commits May 21, 2018 20:51

Changes as per review

75c2b73

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

2d59e05

…o_sig

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

6d7dd78

…o_sig

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

383d761

…o_sig

hub-cap merged commit a8cea90 into elastic:master May 22, 2018

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

hub-cap mentioned this pull request May 22, 2018

Change BWC version for VerifyRepositoryResponse #30796

Merged

hub-cap removed the backport pending label May 25, 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

Modify state of VerifyRepositoryResponse for bwc #30762

Modify state of VerifyRepositoryResponse for bwc #30762

hub-cap commented May 21, 2018

elasticmachine commented May 21, 2018

hub-cap commented May 21, 2018

nik9000 May 21, 2018

nik9000 May 21, 2018

nik9000 May 21, 2018

nik9000 May 21, 2018

hub-cap commented May 21, 2018

nik9000 commented May 21, 2018

hub-cap commented May 21, 2018

jasontedor left a comment

jasontedor May 22, 2018 •

edited

Loading

hub-cap May 22, 2018

jasontedor May 22, 2018

hub-cap May 22, 2018

jasontedor May 22, 2018

hub-cap May 22, 2018

jasontedor May 22, 2018

hub-cap May 22, 2018

jasontedor May 22, 2018

Modify state of VerifyRepositoryResponse for bwc #30762

Modify state of VerifyRepositoryResponse for bwc #30762

Conversation

hub-cap commented May 21, 2018

elasticmachine commented May 21, 2018

hub-cap commented May 21, 2018

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

hub-cap commented May 21, 2018

nik9000 commented May 21, 2018

hub-cap commented May 21, 2018

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor May 22, 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

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

jasontedor May 22, 2018 •

edited

Loading