Share common readFrom/writeTo code in AcknowledgeResponse #30983

cbuescher · 2018-05-30T22:07:07Z

The majority of Responses inheriting from AcknowledgeResponse implement
the readFrom and writeTo serialization method in the same way. Moving this
as a default into AcknowledgeResponse and letting the few exceptions that
need a slightly different implementation handle this themselves saves a lot
of duplication.
I don't know if there is/was other reasons about not doing this in the past, if
so I'd like to learn about them.

elasticmachine · 2018-05-30T22:07:08Z

Pinging @elastic/es-core-infra

jasontedor

LGTM. I left a question that can be answered here and resolved if needed in a follow-up.

jasontedor · 2018-05-31T05:11:14Z

server/src/main/java/org/elasticsearch/action/support/master/AcknowledgedResponse.java

+    @Override
+    public void readFrom(StreamInput in) throws IOException {
+        super.readFrom(in);
+        readAcknowledged(in);


I am on mobile so I can not look this up. Is there a reason these read/write methods need to be visible to sub-classes now with this change?

Good point. Unfortunately there are (only) six classes extending AcknowledgedResponse that use those directly at the moment to read/write the acknowledged flag. I just looked at the details and found a problem with bwc with this change because they also use super() and interleave reading/writing the acknowledged flag in s weird way. I'm down to three cases where it might make sense to change the order of serialization in a bwc way to make this change work.

Conflicts: server/src/main/java/org/elasticsearch/action/admin/cluster/reroute/ClusterRerouteResponse.java

cbuescher · 2018-06-01T08:32:24Z

I added another commit that fixes some bwc issues for wire compatibility and changes the serialization on master so the acknowledged flag gets written earlier so we can use the super class read/write logic here as well.
@javanna since you just touched one of these classes last, would you mind taking another look at that last commit?

javanna

LGTM but we may need some more tests for the bw comp layer? I assume that we already have tests for serialization of all the classes touched, still specific tests for this bw comp layer would be good to have.

javanna · 2018-06-01T12:42:44Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponse.java

+            rolledOver = in.readBoolean();
+            readShardsAcknowledged(in);
+        } else {
+            oldIndex = in.readString();


here we are lucky that super reads only the acknowledged flag and we get away with effectively not calling super, it's a tricky one ;)

I would probably make a method with this shared code block

I'm not sure I understand which code block to share. Do you mean the part in super() that writes the boolean flag or the common read parts of the two branches here? In the later case this isn't completely possible since in the "old" serialization order the acknowledged field gets read as second-to-last field. Maybe I didn't understand this correctly.

arg I hadn't seen the acknowledged flag, nevermind

javanna · 2018-06-01T12:44:08Z

server/src/main/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponse.java

+            out.writeBoolean(dryRun);
+            out.writeBoolean(rolledOver);
+            out.writeBoolean(acknowledged);
+            writeShardsAcknowledged(out);


same here, factor out this shared code block into a method?

Conflicts: server/src/main/java/org/elasticsearch/action/ingest/PutPipelineResponse.java server/src/main/java/org/elasticsearch/action/ingest/WritePipelineResponse.java

cbuescher · 2018-06-01T14:41:42Z

@javanna thanks, I added bwc tests for RolloverResponse and ClusterUpdateSettingsResponse. For ClusterRerouteResponse the work needed would be quiet considerable since the test class doesn't really support any of the wireformat and serialization testing so far, and involves quiet complicated setup (contains ClusterState which e.g. doesn't implement equals/hashCode etc...). Adding any kind of serialization testing that makes sense would take quiet a bit of work, so I hope you don't mind if we don't do this in this PR. I count on integration testing in mixed cluster environments to catch any potential issues.

cbuescher · 2018-06-04T10:24:38Z

test this please

cbuescher · 2018-06-04T13:24:21Z

Backport PR: #31071

* master: Remove usage of explicit type in docs (elastic#29667) Share common readFrom/writeTo code in AcknowledgeResponse (elastic#30983) Adapt bwc versions after backporting elastic#31045 to 6.x Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (elastic#30989) Remove wrong link in index phrases doc Move pipeline APIs to ingest namespace (elastic#31027) [DOCS] Fixes accounting setting names (elastic#30863) [DOCS] Rewords _field_names documentation (elastic#31029) Index phrases (elastic#30450)

* master: Match phrase queries against non-indexed fields should throw an exception (#31060) In the internal highlighter APIs, use the field type as opposed to the mapper. (#31039) [DOCS] Removes duplicated authentication pages Enable customizing REST tests blacklist (#31074) Make sure KeywordFieldMapper#clone preserves split_queries_on_whitespace. (#31049) [DOCS] Moves machine learning overview to stack-docs [ML] Add secondary sort to ML events (#31063) [Rollup] Specialize validation exception for easier management (#30339) Adapt bwc versions after backporting #31045 to 6.3 Remove usage of explicit type in docs (#29667) Share common readFrom/writeTo code in AcknowledgeResponse (#30983) Adapt bwc versions after backporting #31045 to 6.x Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (#30989) Remove wrong link in index phrases doc Move pipeline APIs to ingest namespace (#31027) [DOCS] Fixes accounting setting names (#30863) [DOCS] Rewords _field_names documentation (#31029) Index phrases (#30450) Remove leftover debugging from PTCMDT Fix PTCMDT#testMinVersionSerialization Make Persistent Tasks implementations version and feature aware (#31045)

Relates #30983

) The majority of Responses inheriting from AcknowledgeResponse implement the readFrom and writeTo serialization method in the same way. Moving this as a default into AcknowledgeResponse and letting the few exceptions that need a slightly different implementation handle this themselves saves a lot of duplication.

The majority of Responses inheriting from AcknowledgeResponse implement the readFrom and writeTo serialization method in the same way. Moving this as a default into AcknowledgeResponse and letting the few exceptions that need a slightly different implementation handle this themselves saves a lot of duplication.

* 6.x: Share common readFrom/writeTo code in AcknowledgeResponse (#30983) [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient Fix rest test skip version Fix docs build. Add a doc value format to binary fields. (#30860) Only auto-update license signature if all nodes ready (#30859) Add BlobContainer.writeBlobAtomic() (#30902) Move caching of the size of a directory to `StoreDirectory`. (#30581) Clarify docs about boolean operator precedence. (#30808) Docs: remove notes on sparsity. (#30905) Improve documentation of dynamic mappings. (#30952) Decouple MultiValueMode. (#31075) Docs: Clarify constraints on scripted similarities. (#31076)

Relates #30983

* master: Removing erroneous repeat Adapt bwc versions after backporting #30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (#30978) Fix docs build. Only auto-update license signature if all nodes ready (#30859) Add BlobContainer.writeBlobAtomic() (#30902) Add a doc value format to binary fields. (#30860)

* elastic/master: Add cors support to NioHttpServerTransport (elastic#30827) [DOCS] Fixes security example (elastic#31082) Allow terms query in _rollup_search (elastic#30973) Removing erroneous repeat Adapt bwc versions after backporting elastic#30983 to 6.4

* ccr: [DOCS] Creates rest-api folder in docs [Rollup] Disallow index patterns that match the rollup index (elastic#30491) Replace exact numDocs by soft-del count in SegmentInfo (elastic#31086) Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (elastic#31073) Add cors support to NioHttpServerTransport (elastic#30827) [DOCS] Fixes security example (elastic#31082) Allow terms query in _rollup_search (elastic#30973) Removing erroneous repeat Adapt bwc versions after backporting elastic#30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (elastic#30978) Fix docs build. Only auto-update license signature if all nodes ready (elastic#30859) Add BlobContainer.writeBlobAtomic() (elastic#30902) Add a doc value format to binary fields. (elastic#30860)

cbuescher added >non-issue review :Core/Infra/Core Core issues without another label v7.0.0 v6.4.0 labels May 30, 2018

jasontedor approved these changes May 31, 2018

View reviewed changes

cbuescher added >refactoring and removed >non-issue labels May 31, 2018

Christoph Büscher added 2 commits May 31, 2018 14:44

Share common readFrom/writeTo code in AcknowledgeResponse

7a4c4bd

Fix serialization for classes overwriting readFrom/writeTo

6fd0d69

cbuescher force-pushed the cleanup-AckResponse branch from eb53a00 to 6fd0d69 Compare May 31, 2018 12:45

Merge branch 'master' into cleanup-AckResponse

8df71c4

Conflicts: server/src/main/java/org/elasticsearch/action/admin/cluster/reroute/ClusterRerouteResponse.java

cbuescher requested a review from javanna June 1, 2018 08:32

javanna approved these changes Jun 1, 2018

View reviewed changes

Christoph Büscher added 2 commits June 1, 2018 15:43

Merge branch 'master' into cleanup-AckResponse

dbc94b4

Conflicts: server/src/main/java/org/elasticsearch/action/ingest/PutPipelineResponse.java server/src/main/java/org/elasticsearch/action/ingest/WritePipelineResponse.java

Adding old version serialization tests

a9c943f

Christoph Büscher added 2 commits June 4, 2018 10:12

Merge branch 'master' into cleanup-AckResponse

27cf8be

Merge branch 'master' into cleanup-AckResponse

a0fdf8b

cbuescher merged commit 11b11f6 into elastic:master Jun 4, 2018

cbuescher mentioned this pull request Jun 4, 2018

Share common readFrom/writeTo code in AcknowledgeResponse 6x #31071

Merged

cbuescher added the backport pending label Jun 4, 2018

dnhatn added a commit that referenced this pull request Jun 4, 2018

Adapt changes in AcknowledgeResponse

87abb49

Relates #30983

JoeNguyen93 mentioned this pull request Jun 5, 2018

Share common readFrom/writeTo code in AcknowledgeResponse softagram/elasticsearch#1

Closed

cbuescher pushed a commit that referenced this pull request Jun 5, 2018

Adapt bwc versions after backporting #30983 to 6.4

81172c0

dnhatn added a commit that referenced this pull request Jun 5, 2018

Adapt changes in AcknowledgeResponse

8578caf

Relates #30983

cbuescher removed the backport pending label Jun 5, 2018

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

Share common readFrom/writeTo code in AcknowledgeResponse #30983

Share common readFrom/writeTo code in AcknowledgeResponse #30983

cbuescher commented May 30, 2018

elasticmachine commented May 30, 2018

jasontedor left a comment

jasontedor May 31, 2018

cbuescher May 31, 2018

cbuescher commented Jun 1, 2018

javanna left a comment

javanna Jun 1, 2018

cbuescher Jun 1, 2018

javanna Jun 1, 2018

javanna Jun 1, 2018

cbuescher commented Jun 1, 2018

cbuescher commented Jun 4, 2018

cbuescher commented Jun 4, 2018

Share common readFrom/writeTo code in AcknowledgeResponse #30983

Share common readFrom/writeTo code in AcknowledgeResponse #30983

Conversation

cbuescher commented May 30, 2018

elasticmachine commented May 30, 2018

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor May 31, 2018

Choose a reason for hiding this comment

cbuescher May 31, 2018

Choose a reason for hiding this comment

cbuescher commented Jun 1, 2018

javanna left a comment

Choose a reason for hiding this comment

javanna Jun 1, 2018

Choose a reason for hiding this comment

cbuescher Jun 1, 2018

Choose a reason for hiding this comment

javanna Jun 1, 2018

Choose a reason for hiding this comment

javanna Jun 1, 2018

Choose a reason for hiding this comment

cbuescher commented Jun 1, 2018

cbuescher commented Jun 4, 2018

cbuescher commented Jun 4, 2018