-
Notifications
You must be signed in to change notification settings - Fork 424
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
TEZ-4501: Fix TestLocalMode timeouts #300
Conversation
can you please take a look at this @rbalamohan , @bikassaha ? simple fix in AsyncDispatcher |
This comment was marked as outdated.
This comment was marked as outdated.
// this is needed because in serviceStop we're looping to drain the rest of the events and checking !drained | ||
// in that case, an interruption here would lead to an empty queue with drained=false, leading to | ||
// a useless loop with a timeout (this can even cause unit test timeouts) | ||
drained = eventQueue.isEmpty(); | ||
LOG.info( | ||
"Setting drained to {} due to interruption while putting a new event, stopped: {}, blockNewEvents: {}", | ||
drained, stopped, blockNewEvents); |
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.
why in serviceStop we don't directly check eventQueue.isEmpty
?
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.
eh, right, let me try that :)
NOTE: even if it works, I'll have to check the special meaning of having a separate boolean
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 tried to look out for reasons for separate boolean, but didn't find anything reasonable, maybe the coding style then...
💔 -1 overall
This message was automatically generated. |
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.
LGTM
No description provided.