-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-26012 Improve logging and dequeue logic in DelayQueue #3397
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
Conversation
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| return queue.take(); | ||
| E element = queue.poll(10, TimeUnit.SECONDS); | ||
| if (element == null && queue.size() > 0) { | ||
| LOG.error("DelayQueue is not empty when timed waiting elapsed. If this is repeated for" |
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.
This maybe too aggressive? Why choose 10 seconds as timeout here?
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.
Since the default value of hbase.procedure.remote.dispatcher.delay.msec is just 150, I thought 10s might be enough. But I am open to keeping it higher. What do you think would be better value? Maybe 25/30s or 60s?
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.
Also, this condition might be true in some case, but only if this condition continues to be true for the same queue consistently, we might have a problem. That's why I am keeping log message like this:
DelayQueue is not empty when timed waiting elapsed. If this is repeated for same queue consistently, it means no element is getting expired from DelayQueue and it might freeze the system.
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.
I do not think we should output a warn message if it is not a problem. As I said above, is it possible to move the warn log to upper layer? The DelayedUtil seems a general name, not only for dispatcher.
3dc749e to
fc4f089
Compare
|
@Apache9 Addressed your comments, could you please take a look again? Thanks |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| public static <E extends Delayed> E takeWithoutInterrupt(final DelayQueue<E> queue) { | ||
| try { | ||
| return queue.take(); | ||
| return queue.poll(15, TimeUnit.SECONDS); |
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.
Just change the method to poll and let the upper layer pass in the timeout parameters?
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.
Although it sounds good, I believe so far we have only 2 callers and both can pass same value for now.
fc4f089 to
4b5af7a
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Duo Zhang <[email protected]>
Signed-off-by: Duo Zhang <[email protected]>
Signed-off-by: Duo Zhang <[email protected]>
Signed-off-by: Duo Zhang <[email protected]>
No description provided.