Skip to content
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

Add primary term to doc write response #24171

Merged
merged 19 commits into from
Apr 19, 2017

Conversation

jasontedor
Copy link
Member

This commit adds the primary term to the doc write response.

Relates #10708

This commit adds the primary term to the doc write response.
}
}
]
}
--------------------------------------------------
// TESTRESPONSE[s/"took": 30/"took": $body.took/ s/"index_uuid": .../"index_uuid": $body.items.3.update.error.index_uuid/ s/"_seq_no" : 0/"_seq_no" : $body.items.0.index._seq_no/ s/"_seq_no" : 1/"_seq_no" : $body.items.1.delete._seq_no/ s/"_seq_no" : 2/"_seq_no" : $body.items.2.create._seq_no/ s/"_seq_no" : 3/"_seq_no" : $body.items.3.update._seq_no/]
// TESTRESPONSE[s/"took": 30/"took": $body.took/ s/"index_uuid": .../"index_uuid": $body.items.3.update.error.index_uuid/ s/"_seq_no" : 0/"_seq_no" : $body.items.0.index._seq_no/ s/"_primary_term" : 1/"_primary_term" : $body.items.0.index._primary_term/ s/"_seq_no" : 1/"_seq_no" : $body.items.1.delete._seq_no/ s/"_primary_term" : 2/"_primary_term" : $body.items.1.delete._primary_term/ s/"_seq_no" : 2/"_seq_no" : $body.items.2.create._seq_no/ s/"_primary_term" : 3/"_primary_term" : $body.items.2.create._primary_term/ s/"_seq_no" : 3/"_seq_no" : $body.items.3.update._seq_no/ s/"_primary_term" : 4/"_primary_term" : $body.items.3.update._primary_term/]
Copy link
Member

Choose a reason for hiding this comment

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

You can stick these onto separate lines if you like.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed 4c6b35c.

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

Looks good - I left one comment about the Engine related classes.

protected Result(Operation.TYPE operationType, long version, long seqNo) {
this(operationType, null, version, seqNo);
protected Result(Operation.TYPE operationType, long version, long seqNo, long primaryTerm) {
this(operationType, null, version, seqNo, primaryTerm);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on sure about this one - the semantics we currently have is that Result represents everything that the engine decides. The primary term is part of the operation - I'm not sure we need this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you mean d4bbfcd?

}
}
]
}
--------------------------------------------------
// TESTRESPONSE[s/"took": 30/"took": $body.took/ s/"index_uuid": .../"index_uuid": $body.items.3.update.error.index_uuid/ s/"_seq_no" : 0/"_seq_no" : $body.items.0.index._seq_no/ s/"_seq_no" : 1/"_seq_no" : $body.items.1.delete._seq_no/ s/"_seq_no" : 2/"_seq_no" : $body.items.2.create._seq_no/ s/"_seq_no" : 3/"_seq_no" : $body.items.3.update._seq_no/]
// TESTRESPONSE[s/"took": 30/"took": $body.took/]
Copy link
Contributor

Choose a reason for hiding this comment

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

good to know this is possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I didn't know it was until @nik9000 made the suggestion and so I went and read the source to understand the syntax behind it.

* master:
  Add BucketMetricValue interface (elastic#24188)
  Enable index-time sorting (elastic#24055)
  Clarify elasticsearch user uid:gid mapping in Docker docs
  Update field-names-field.asciidoc (elastic#24178)
  ElectMasterService.hasEnoughMasterNodes should return false if no masters were found
  Remove Ubuntu 12.04 (elastic#24161)
  [Test] Add unit tests for InternalHDRPercentilesTests (elastic#24157)
  Replicate write failures (elastic#23314)
  Rename variable in translog simple commit test
  Strengthen translog commit with open view test
  Stronger check in translog prepare and commit test
  Fix translog prepare commit and commit test
  ingest-node.asciidoc - Clarify json processor (elastic#21876)
  Painless: more testing for script_stack (elastic#24168)
return new DeleteResult(ex, plan.versionOfDeletion, plan.versionOfDeletion,
plan.currentlyDeleted == false);
return new DeleteResult(
ex, plan.versionOfDeletion, plan.seqNoOfDeletion, plan.currentlyDeleted == false);
Copy link
Member Author

Choose a reason for hiding this comment

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

@bleskes By the way, I think this was a bug, maybe there's a test case missing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@areek I think that the tests in #23314 don't cover the delete case (but only indexing). Can you look at this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bleskes thanks for the ping, I am working on adding tests for the delete case

Copy link
Contributor

Choose a reason for hiding this comment

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

I added tests for this 077a6c3

@jasontedor jasontedor merged commit 4796557 into elastic:master Apr 19, 2017
@jasontedor jasontedor deleted the doc-write-response-primary-term branch April 19, 2017 18:44
@jasontedor
Copy link
Member Author

Thanks @nik9000 and @bleskes.

@clintongormley clintongormley added :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Sequence IDs labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >enhancement v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants