Add seq no powered optimistic locking support to the index and delete transport actions #36619
Conversation
… transport actions
bleskes
added
>enhancement
:Distributed/CRUD
|
Pinging @elastic/es-distributed |
| @@ -77,6 +78,8 @@ | |||
| private static final ParseField RETRY_ON_CONFLICT = new ParseField("retry_on_conflict"); | |||
| private static final ParseField PIPELINE = new ParseField("pipeline"); | |||
| private static final ParseField SOURCE = new ParseField("_source"); | |||
| private static final ParseField IF_SEQ_NO_MATCH = new ParseField("if_seq_no_match"); | |||
There was a problem hiding this comment.
really? :D that name is very confusing. apparently expected_seq_no is not what you want do. bummer. I am definitely -1 on this one sorry. Same goes for any cas variants. I think these names are really not good. What about "replace" : { "seq_no" : x, "primary_term" : y}?
There was a problem hiding this comment.
:) this is hard. The problem with json structure is that these need to go in a url parameter.
| @@ -417,16 +427,16 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null | |||
| if ("index".equals(action)) { | |||
| if (opType == null) { | |||
| internalAdd(new IndexRequest(index, type, id).routing(routing).version(version).versionType(versionType) | |||
| .setPipeline(pipeline) | |||
| .setPipeline(pipeline).ifMatch(ifSeqNoMatch, ifPrimaryTermMatch) | |||
There was a problem hiding this comment.
ifMatch no sorry -1 :) that's even worse than cas
| @@ -215,6 +250,13 @@ public void readFrom(StreamInput in) throws IOException { | |||
| } | |||
| version = in.readLong(); | |||
| versionType = VersionType.fromValue(in.readByte()); | |||
| if (in.getVersion().onOrAfter(Version.V_7_0_0)) { | |||
| ifSeqNoMatch = in.readZLong(); | |||
There was a problem hiding this comment.
can it be negative at this point? if not can we assert?
There was a problem hiding this comment.
this can be -2 (unassigned). I wonder if we should call the validate method on these once deserialized and under assertion? I think that will give us a much better testing coverage?
There was a problem hiding this comment.
I left my suggestions, I will leave it to whoever makes the call on the naming. I will leave my reasoning here and that's it.
I think we use a CAS like approach internally. That approach expects a expected_primary_term and expected_seq_no. That's what I would name the rest params. the java API should be setExpected(long seqId, long primaryTerm) If it's not set you we don't expect anything. I tried to think of this as how would I write docs and explain it and from that perspecitve expected is the obvious solution. I personally feel the solutions with cas in the name or if are terrible and I would not want to go with. I understand that this is taste and if folks feel that is the way to go so be it.
|
run the gradle build tests 2 |
… transport actions (#36619) This commit add support for using sequence numbers to power [optimistic concurrency control](http://en.wikipedia.org/wiki/Optimistic_concurrency_control) in the delete and index transport actions and requests. A follow up will come with adding sequence numbers to the update and get results. Relates #36148 Relates #10708
This PR add support for using sequence numbers to power optimistic concurrency control in the delete and index transport actions and requests. A follow up will come with adding sequence numbers to the update and get results.
Relates #36148
Relates #10708