Use standard semantics for retried auto-id requests #47311
Conversation
|
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
Thanks @ywelsch for working on this. My primary comment here is if we should treat op_type=create with auto-id identically to op_type=index with auto-id? More details in comments.
| @@ -267,9 +267,6 @@ public void testVersionMapAfterAutoIDDocument() throws IOException { | |||
| try (Engine.Searcher searcher = engine.acquireSearcher("test")) { | |||
| assertEquals(2, searcher.getIndexReader().numDocs()); | |||
| } | |||
| if (operation.origin() == PRIMARY) { | |||
There was a problem hiding this comment.
Not sure I follow why this is not still valid to assert?
There was a problem hiding this comment.
This was an artifact of an earlier iteration where I had changed things up more fundamentally. I've reinstated this check again in 76066ee.
| } else { | ||
| plan = IndexingStrategy.optimizedAppendOnly(1L); | ||
| } | ||
| final boolean canOptimizeAddDocument = canOptimizeAddDocument(index); |
There was a problem hiding this comment.
I think this means that a double delivery of an auto-id with op_type=create will fail with a version conflict? I know this is similar to regular if_seqno update, but I would prefer to not do that in the auto-id case, since we know that it is safe? Especially because this makes it simpler to later advocate to change POST index/_doc to imply op_type=create.
We can still do the code simplification, we should just avoid doing the isVersionConflictForWrites check when canOptimizeAddDocument=true.
Another version of it is to skip the write if the doc is already there? Would be kind of nicer towards someone doing update-by-query concurrently with this (the updated-counts would be slightly more correct, since we no longer risk a retry overwriting something that was already updated). However, I think we then risk returning a non-fsync'ed result and we should probably defer this to a follow-up if we want to do this.
There was a problem hiding this comment.
yes, this will mean that double-delivery of an indexing operation with auto-id and optype create will fail with a version conflict, which is similar to an indexing operation without auto-id but with optype create. I agree that we should try to handle internal retries better (in both situations, with the auto-id case perhaps being simpler), but I also do not want to continue being lenient when it comes to overriding newer docs due to retries, in particular if the optype is create (even more so if we associate an index privilege with this).
My plan was to explore something along the lines of your last paragraph as a follow-up, but I do not see this as a blocker to release the given change here.
| } else { | ||
| plan = IndexingStrategy.optimizedAppendOnly(1L); | ||
| } | ||
| final boolean canOptimizeAddDocument = canOptimizeAddDocument(index); |
Adds support for handling auto-id requests with optype CREATE. Also simplifies the code handling this by using the standard indexing path when dealing with possible retry conflicts. Relates elastic#47169
Adds support for handling auto-id requests with optype CREATE. Also simplifies the code handling this by using the standard indexing path when dealing with possible retry conflicts. Relates #47169
Adds support for handling auto-id requests with optype CREATE. Also simplifies the code handling this by using the standard indexing path when dealing with possible retry conflicts.
Relates #47169