HBASE-24603: Make Zookeeper sync() call synchronous #1945
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
previously approved these changes
Jun 22, 2020
| this.recoverableZooKeeper.sync(path, (i, s, o) -> latch.countDown(), null); | ||
| try { | ||
| if (!latch.await(zkSyncTimeout, TimeUnit.MILLISECONDS)) { | ||
| LOG.error("sync() operation to ZK timed out. Configured timeout: {}ms. This usually points " |
There was a problem hiding this comment.
IMHO error is fine but better to keep it warn. We anyways have warn for InterruptedException also.
There was a problem hiding this comment.
Any particular reason this has to be WARN? This can potentially be a correctness issue at this point and hence I chose the higher log level.
There was a problem hiding this comment.
WARN implies to me as an operator that something bad happened that is noteworthy but not an emergency. ERROR implies a serious problem that probably should cause someone to be paged.
sync is going to occasionally time out. We should be handling such without correctness problems. ERROR logging doesn't substitute for handling the timeout properly. So, assuming we are handling timeouts correctly, WARN level logging is most appropriate.
There was a problem hiding this comment.
Fair point. Changed.
hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java
Show resolved
Hide resolved
|
Test failures are relevant, I just ran |
apurtell
requested changes
Jun 22, 2020
hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java
Show resolved
Hide resolved
| this.recoverableZooKeeper.sync(path, (i, s, o) -> latch.countDown(), null); | ||
| try { | ||
| if (!latch.await(zkSyncTimeout, TimeUnit.MILLISECONDS)) { | ||
| LOG.error("sync() operation to ZK timed out. Configured timeout: {}ms. This usually points " |
|
💔 -1 overall
This message was automatically generated. |
1 similar comment
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
commented
Jun 25, 2020
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Jun 25, 2020
hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java
Show resolved
Hide resolved
hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java
Outdated
Show resolved
Hide resolved
hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java
Show resolved
Hide resolved
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread.
apurtell
reviewed
Jun 25, 2020
hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java
Show resolved
Hide resolved
apurtell
reviewed
Jun 25, 2020
hbase-zookeeper/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKWatcher.java
Show resolved
Hide resolved
apurtell
approved these changes
Jun 25, 2020
|
Approved, assuming a clean precommit prior to merge. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
added a commit
to bharathv/hbase
that referenced
this pull request
Jun 25, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f)
|
🎊 +1 overall
This message was automatically generated. |
bharathv
added a commit
to bharathv/hbase
that referenced
this pull request
Jun 25, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f) (cherry picked from commit 2379a25)
bharathv
added a commit
to bharathv/hbase
that referenced
this pull request
Jun 25, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f) (cherry picked from commit 2379a25)
bharathv
added a commit
that referenced
this pull request
Jun 25, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f)
bharathv
added a commit
that referenced
this pull request
Jun 25, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f)
bharathv
added a commit
that referenced
this pull request
Jun 26, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f) (cherry picked from commit 2379a25)
bharathv
added a commit
that referenced
this pull request
Jul 10, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f)
bharathv
added a commit
that referenced
this pull request
Jul 10, 2020
Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f)
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…ache#1975) Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f)
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
…ache#1975) Author: Andrew Purtell Reason: Improvement Ref: CDPD-15964 Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> Change-Id: Iea2a2e7ae9d64d5e5349fe7c4ba7eda92da74a65 (cherry picked from commit 9e06a48)
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…ache#1975) Writing a test for this is tricky. There is enough coverage for functional tests. Only concern is performance, but there is enough logging for it to detect timed out/badly performing sync calls. Additionally, this patch decouples the ZK event processing into it's own thread rather than doing it in the EventThread's context. That avoids deadlocks and stalls of the event thread. Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 84e246f) (cherry picked from commit 100d794) Change-Id: Iea2a2e7ae9d64d5e5349fe7c4ba7eda92da74a65
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Writing a test for this is tricky. There is enough coverage for
functional tests. Only concern is performance, but there is enough
logging for it to detect timed out/badly performing sync calls.