Uh oh!
There was an error while loading. Please reload this page.
HBASE-24877 Add option to avoid aborting RS process upon uncaught exc… - #2255
Conversation
…eptions happen on replication source
Apache-HBase
commented
Aug 13, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2020
🎊 +1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| Thread.currentThread().getName() + ".replicationSource," + this.queueId, | ||
| (t,e) -> { | ||
| uncaughtException(t, e); | ||
| retryStartup.set(true); |
There was a problem hiding this comment.
I hope if we encounter uncaughtException here, we want to retry the loop again.
If so, shall we also add startupOngoing.set(true); here explicitely? Just in case if it is set to false in initialize()?
There was a problem hiding this comment.
I hope if we encounter uncaughtException here, we want to retry the loop again.
Yes, we want to keep trying until we succeed.
If so, shall we also add startupOngoing.set(true); here explicitely? Just in case if it is set to false in initialize()?
The only cases startupOngoing would had been set to false in initialize is if it completes fine without any uncaught exception, so adding it here would be redundant.
There was a problem hiding this comment.
Yeah right, it is not set to false if it completes successfully, anyone touching the same code in future should realize this.
Although not a strong point but If you don't mind, maybe we can comment here indicating startupOngoing is expected to be true when we are here handling Exception.
There was a problem hiding this comment.
Addressed the nits and added some comments to the two flags.
Apache9
left a comment
There was a problem hiding this comment.
What's next if we ignore the exception? We will retry later? Or we will just go on without this replication source? Users will then find out that the cluster is fine but data has not been replicated out? I'm not sure if this is correct way, we fix an issue but introduce another hard to find issue?
Adding a flag can keep the old behavior but we give users an impression that the exception can be ignored? Still not sure if this is the correct way to fix this...
Mind explaining more on your real usage?
Thanks.
wchevreuil
commented
Aug 14, 2020
As you can see on
It's common practice to verify replication status after a maintenance.
It does not fail silently, errors will get logged, and it gives operators the chance to look after what's going wrong without a complete downtime of their source clusters.
We do use some custom replication endpoints that under certain unavailability of some target peer hosts ended up throwing uncaught exception and aborting the source RSes. Sure, there could be improvements on the custom code, and it was an internal infra issue, but with a flag like this, we wouldn't need to face a period of outage at the source. |
Apache9
commented
Aug 14, 2020
OK, thank you for your reply. Let me take a look at the code more carefully. |
Apache-HBase
commented
Aug 14, 2020
🎊 +1 overall
This message was automatically generated. |
| walReader, Thread.currentThread().getName() | ||
| + ".replicationSource.wal-reader." + walGroupId + "," + queueId, | ||
| this::uncaughtException); | ||
| (t,e) -> { |
There was a problem hiding this comment.
So here it is for wal reader. I think refreshSources and retry is an acceptable way. Then let's just test the abortOnError flag here? If it is true, we will call uncaughtException, otherwise we will try to refresh the replication source.
| Threads.setDaemonThreadRunning(this, | ||
| name + ".replicationSource.shipper" + walGroupId + "," + source.getQueueId(), handler); | ||
| name + ".replicationSource.shipper" + walGroupId + "," + source.getQueueId(), | ||
| (t,e) -> { |
There was a problem hiding this comment.
OK, the code is almost the same... Then I think we could move the logic into uncaughtException method? If abortOnError is true, we about, otherwise we will try to refresh the source.
There was a problem hiding this comment.
Moved it to uncaughtException. This is true when handling potential errors on both the reader and shipper threads, but uncaughtException is also used when handling issues from ReplicationSource.initialize method, which might blow before reader/shipper is even started. So needed to add extra check in uncaughtException to decide when to invoke refreshSources.
| Threads.setDaemonThreadRunning(initThread, | ||
| Thread.currentThread().getName() + ".replicationSource," + this.queueId, | ||
| this::uncaughtException); | ||
| this.retryStartup.set(true); |
There was a problem hiding this comment.
This flag is only used in this method? Let's use a local var instead of a class member field?
There was a problem hiding this comment.
Yeah, originally I was also referring it on uncaughtException, but it was not actually needed.
Uh oh!
There was an error while loading. Please reload this page.
wchevreuil
commented
Aug 24, 2020
Thanks for the suggestions, @Apache9, had a pushed a new commit addressing those, let me know on your thoughts. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
| // an initialization happening (but not finished yet), | ||
| // so that it doesn't try submit another initialize thread. | ||
| // NOTE: this should only be set to false at the end of initialize method, prior to return. | ||
| // private final AtomicBoolean startupOngoing = new AtomicBoolean(false); |
| if(abortOnError){ | ||
| server.abort("Unexpected exception in " + t.getName(), e); | ||
| } | ||
| if(manager!=null){ |
There was a problem hiding this comment.
Need to update the formatter config? Usually it should be 'if (manager != null) {'.
| this::uncaughtException); | ||
| //Flag that signalizes uncaught error happening while starting up the source | ||
| // and a retry should be attempted | ||
| AtomicBoolean retryStartup = new AtomicBoolean(false); |
There was a problem hiding this comment.
Why we need a AtomicBoolean here? It is only used locally, so a simple boolean is enough?
There was a problem hiding this comment.
It's been modified by the lambda expression on line #635, since lambdas expressions require variables to be final, I can't use a simple, local primitive boolean or wrapper.
There was a problem hiding this comment.
Then use MutableBoolean? We do not need to use atomic here.
There was a problem hiding this comment.
Sure, thanks for suggesting MutableBoolean, am not too familiar with apache commons lib.
| retryStartup.set(true); | ||
| }); | ||
| } | ||
| } while (!this.sourceRunning); |
There was a problem hiding this comment.
I had a second thought on this here, we can't simply re-use this boolean, because in case of failure, we risk reach this point before the exception handler has updated it to false. I'm bringing back the original startupOngoing in the next commit,
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2020
🎊 +1 overall
This message was automatically generated. |
wchevreuil
commented
Aug 25, 2020
Pushed a new commit addressing latest suggestion and checkstyle issues. |
Apache-HBase
commented
Aug 25, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 25, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 25, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 25, 2020
🎊 +1 overall
This message was automatically generated. |
wchevreuil
commented
Aug 26, 2020
retest build |
1 similar comment
wchevreuil
commented
Aug 26, 2020
retest build |
Apache9
commented
Aug 27, 2020
This does not work, you need to go to the jenkins page to manually start a build, or just do a rebase and force push here, if you want to trigger a new build. |
Apache-HBase
commented
Aug 28, 2020
🎊 +1 overall
This message was automatically generated. |
wchevreuil
commented
Aug 29, 2020
It looks like this is causing some of the UTs to timeout. Let me dig into it further. |
…mplete initialization but don't care about it
Apache-HBase
commented
Sep 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 4, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 4, 2020
💔 -1 overall
This message was automatically generated. |
wchevreuil
commented
Sep 4, 2020
Latest UT failure seems unrelated, have it passing locally. |
joshelser
left a comment
There was a problem hiding this comment.
Looks OK to me. Seems like Duo has given this a thorough investigation :)
…eptions happen on replication source