-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-28850 Only return from ReplicationSink.replicationEntries while… #6263
Conversation
… all background tasks are finished
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trade off is this may change the performance of replicateEntries. Before, the replicateEntries call will fail fast at the first failure. After, the replicateEntries will block for the entire time it takes to confirm all local edits in the batch are applied or failed.
I have a testbed where I reproduced SEGV crashes for HBASE-28584. Let me apply this patch there and try it out.
This comment has been minimized.
This comment has been minimized.
if (error == null) { | ||
error = ioe; | ||
} else { | ||
error.addSuppressed(ioe); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging the exception before it is suppressed might be helpful? (unless suppressed exceptions are all logged with original exception, which i don't recall if it happens)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static void main(String[] args) {
Exception error = new Exception("root");
error.addSuppressed(new IOException("suppressed1"));
error.addSuppressed(new IOException("suppressed2"));
error.printStackTrace();
}
The output
java.lang.Exception: root
at org.apache.hadoop.hbase.regionserver.regionreplication.RegionReplicationSink.main(RegionReplicationSink.java:461)
Suppressed: java.io.IOException: suppressed1
at org.apache.hadoop.hbase.regionserver.regionreplication.RegionReplicationSink.main(RegionReplicationSink.java:462)
Suppressed: java.io.IOException: suppressed2
at org.apache.hadoop.hbase.regionserver.regionreplication.RegionReplicationSink.main(RegionReplicationSink.java:463)
So I think it is fine to not log it here?
Just a minor comment above, +1 otherwise. Had an offline chat with @apurtell also reg this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good. I rehydrated the testbed where I was able to reproduce SEGVs and everything has been stable and performing normally after 250M rows replicated on the way up to 1B. Not expecting any issues.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
… all background tasks are finished (apache#6263) Signed-off-by: Andrew Purtell <apurtell@apache.org> (cherry picked from commit 52082bc)
… all background tasks are finished