[CCR] Improve error when operations are missing #35179
Conversation
martijnvg
added
>non-issue
:Distributed Indexing/CCR
|
Pinging @elastic/es-distributed |
| @@ -84,4 +87,32 @@ public void testGetOperationsBasedOnGlobalSequenceId() throws Exception { | |||
| assertThat(operation.id(), equalTo("5")); | |||
| } | |||
|
|
|||
| public void testMissingOperations() { | |||
| protected void doExecute(Task task, Request request, ActionListener<Response> listener) { | ||
| ActionListener<Response> wrappedListener = ActionListener.wrap(listener::onResponse, e -> { | ||
| Throwable cause = ExceptionsHelper.unwrapCause(e); | ||
| if (cause instanceof IllegalStateException && cause.getMessage().contains("Not all operations between from_seqno [")) { |
There was a problem hiding this comment.
I am fine with this change but I prefer to narrow the scope. How about catching and converting the exception with the existing try in ShardChangesAction#getOperations? I am thinking to have a dedicated exception so that we can access "fromSeqNo" and "toSeqNo" but maybe overkilled. WDYT?
There was a problem hiding this comment.
I am thinking to have a dedicated exception so that we can access "fromSeqNo" and "toSeqNo" but maybe overkilled.
We should not introduce that dedicated exception I think.
There was a problem hiding this comment.
I am fine with this change but I prefer to narrow the scope. How about catching and converting the exception with the existing try in ShardChangesAction#getOperations
I think converting the exception there is premature. If a replica shard copy is queried, it may not have all the operations, but another shard may have it, so the shard changes api can retry it on another shard copy. Then the exception convertion was not needed, since the shard changes api was able to serve the request after all.
When we intercept the exception at the place were the exception conversion is now, we know that the shard changes has attempted the read the range of operations on all shard copies, but still wasn't able to satisfy the request.
I am thinking to have a dedicated exception so that we can access "fromSeqNo" and "toSeqNo" but maybe overkilled. WDYT?
I was considering this, because the way we detect this kind of error is weak (instance of IllegalStateException AND the exception message containing a specific message). What about throwing an EngineException with two headers (from and to seqno) in LuceneChangesSnapshot#rangeCheck(...)?
There was a problem hiding this comment.
Thanks for the explanation. Let's do it here. Since the error has an inner cause which provides from_seq_no and to_seq_no information, I think we are good.
There was a problem hiding this comment.
+100 to any way that doesn't rely on message strings. Thanks for iterating on this to get there!
x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardChangesAction.java
Outdated
Show resolved
Hide resolved
Improve error when operations are missing
Improve error when operations are missing
No description provided.