Skip to content

ShardSearchFailure#readFrom to set index and shardId #33161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 27, 2018

Conversation

javanna
Copy link
Member

@javanna javanna commented Aug 27, 2018

As part of recent changes made to ShardOperationFailedException we introduced index and shardId members to the base class, but the subclasses are entirely responsible for the serialization of such fields. In the case of ShardSearchFailure, we have an additional SearchShardTarget instance member which also holds the index and the shardId, hence they get serialized as part of SearchShardTarget itself. When de-serializing a ShardSearchFailure though, we need to remember to also set the parent class index and shardId fields otherwise they get lost

Relates to #32640

As part of recent changes made to `ShardOperationFailedException` we introduced `index` and `shardId` members to the base class, but the subclasses are entirely responsible for the serialization of such fields. In the case of `ShardSearchFailure`, we have an additional `SearchShardTarget` instance member which also holds the index and the shardId, hence they get serialized as part of `SearchShardTarget` itself. When de-serializing a `ShardSearchFailure` though, we need to remember to also set the parent class `index` and `shardId` fields otherwise they get lost

Relates to elastic#32640
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna
Copy link
Member Author

javanna commented Aug 27, 2018

I labelled this non-issue as it fixes a bug that was never released, due to changes pushed to master and 6.x.

@javanna javanna requested a review from jimczi August 27, 2018 10:12
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch !

@@ -101,6 +100,8 @@ public static ShardSearchFailure readShardSearchFailure(StreamInput in) throws I
public void readFrom(StreamInput in) throws IOException {
if (in.readBoolean()) {
shardTarget = new SearchShardTarget(in);
super.index = shardTarget.getFullyQualifiedIndexName();
super.shardId = shardTarget.getShardId().getId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove super ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

@javanna javanna merged commit ed0571e into elastic:master Aug 27, 2018
dnhatn added a commit that referenced this pull request Aug 28, 2018
* master:
  [Rollup] Better error message when trying to set non-rollup index (#32965)
  HLRC: Use Optional in validation logic (#33104)
  Remove unused User class from protocol (#33137)
  ingest: Introduce the dissect processor (#32884)
  [Docs] Add link to es-kotlin-wrapper-client (#32618)
  [Docs] Remove repeating words (#33087)
  Minor spelling and grammar fix (#32931)
  Remove support for deprecated params._agg/_aggs for scripted metric aggregations (#32979)
  Watcher: Simplify finding next date in cron schedule (#33015)
  Run Third party audit with forbidden APIs CLI  (part3/3) (#33052)
  Fix plugin build test on Windows (#33078)
  HLRC+MINOR: Remove Unused Private Method (#33165)
  Remove old unused test script files (#32970)
  Build analysis-icu client JAR (#33184)
  Ensure to generate identical NoOp for the same failure (#33141)
  ShardSearchFailure#readFrom to set index and shardId (#33161)
javanna added a commit that referenced this pull request Sep 18, 2018
As part of recent changes made to `ShardOperationFailedException` we introduced `index` and `shardId` members to the base class, but the subclasses are entirely responsible for the serialization of such fields. In the case of `ShardSearchFailure`, we have an additional `SearchShardTarget` instance member which also holds the index and the shardId, hence they get serialized as part of `SearchShardTarget` itself. When de-serializing a `ShardSearchFailure` though, we need to remember to also set the parent class `index` and `shardId` fields otherwise they get lost

Relates to #32640
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants