Add Sequence Numbers and enforce Primary Terms #14651
Conversation
| /** | ||
| * A based class for the response of a write operation that involves are a single doc | ||
| */ | ||
| public abstract class DocWriteResponse extends ReplicationResponse implements ToXContent { |
There was a problem hiding this comment.
can this somehow implement Writeable here or rather at the parent and make all the members final?
There was a problem hiding this comment.
I was trying to keep the same patterns as before to keep the change small. Will give it a shot.
There was a problem hiding this comment.
I had a look and this impossible without changing our network infra as well. This inherits from TransportResponse which implements Streamable, i.e., the readFrom has different semantics and returns void.
|
looks great I left some comments |
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * A based class for the response of a write operation that involves are a single doc |
There was a problem hiding this comment.
Nits: "based class" -> "base class" and "involves are a" -> "involves a"
bleskes
force-pushed
the
seq_no_counter
branch
from
eaf0866
to
ea638ab
Compare
|
@jasontedor can you take another look? |
bleskes
force-pushed
the
seq_no_counter
branch
from
ea638ab
to
7179e2a
Compare
| this.version = version; | ||
| } | ||
|
|
||
| public DocWriteResponse() { |
There was a problem hiding this comment.
Should this have a comment that it's for serialization?
There was a problem hiding this comment.
will do. Also made it protected.
clintongormley
changed the title
| } | ||
|
|
||
| /** | ||
| * Returns the sequence number assigned for this change. Returns -1L if the operation wasn't |
There was a problem hiding this comment.
Do you think it would be clearer if the -1L that are now scattered in a few places (e.g., UpdateResponse, among a few others) were changed to refer to some constant field?
|
|
||
| try { | ||
| indexShard.incrementOperationCounter(primaryTerm - 1); | ||
| fail("you can not increment the operation counter with an older primary term"); |
|
@bleskes I have a few nits, and some concerns about the |
bleskes
force-pushed
the
seq_no_counter
branch
from
7179e2a
to
e3bc32a
Compare
|
@jasontedor +1 to the -1 :) . pushed another round. Also rebased on latest master so we can sue the writeZLong thing. |
|
LGTM. |
Adds a counter to each write operation on a shard. This sequence numbers is indexed into lucene using doc values, for now (we will probably require indexing to support range searchers in the future). On top of this, primary term semantics are enforced and shards will refuse write operation coming from an older primary. Other notes: - The add SequenceServiceNumber is just a skeleton and will be replaced with much heavier one, once we have all the building blocks (i.e., checkpoints). - I completely ignored recovery - for this we will need checkpoints as well. - A new based class is introduced for all single doc write operations. This is handy to unify common logic (like toXContent). - For now, we don't use seq# as versioning. We could in the future. Relates to #10708 Closes #14651
|
Merged into |
Primary terms is a way to make sure that operations replicated from stale primary are rejected by shards following a newly elected primary. Original PRs adding this to the seq# feature branch elastic#14062 , elastic#14651 . Unlike those PR, here we take a different approach (based on newer code in master) where the primary terms are stored in the meta data only (and not in `ShardRouting` objects). Relates to elastic#17038 Closes elastic#17044
clintongormley
added
:Engine
:Distributed/Engine
This PR (against the feature/seq_no branch) adds a counter to each write operation on a shard. This sequence numbers is indexed into lucene using doc values, for now (we will probably require indexing to support range searchers in the future).
On top of this, primary term semantics are enforced and shards will refuse write operation coming from an older primary.
Other notes:
Relates to #10708