-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
add 'stopped' check and handling to HttpLoadQueuePeon load and drop segment methods #5555
Conversation
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.
Some minor comments.
@@ -261,6 +260,7 @@ public void onSuccess(InputStream result) | |||
public void onFailure(Throwable t) | |||
{ | |||
try { | |||
responseHandler.description = t.getMessage(); |
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.
t.toString()
is more informative than t.getMessage()
, since typically it includes the type of the exception but getMessage does not.
@@ -398,6 +403,15 @@ public void loadSegment(DataSegment segment, LoadPeonCallback callback) | |||
public void dropSegment(DataSegment segment, LoadPeonCallback callback) | |||
{ | |||
synchronized (lock) { | |||
if (stopped) { | |||
log.warn( | |||
"Server[%s] failed to drop segment[%s] because load queue peon is stopped.", |
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.
Instead of "failed to drop", "cannot drop" is a better log message.
@@ -382,6 +377,16 @@ public void stop() | |||
public void loadSegment(DataSegment segment, LoadPeonCallback callback) | |||
{ | |||
synchronized (lock) { | |||
if (stopped) { | |||
log.warn( | |||
"Server[%s] failed to load segment[%s] because load queue peon is stopped.", |
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.
Instead of "failed to load", "cannot load" is a better log message.
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 after fixing the log message.
@@ -405,7 +405,7 @@ public void dropSegment(DataSegment segment, LoadPeonCallback callback) | |||
synchronized (lock) { | |||
if (stopped) { | |||
log.warn( | |||
"Server[%s] failed to drop segment[%s] because load queue peon is stopped.", | |||
"Server[%s] cannot to drop segment[%s] because load queue peon is stopped.", |
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.
“cannot to” should be “cannot”
…egment methods (apache#5555) * add stopped check and handling to HttpLoadQueuePeon load and drop segment methods * fix unrelated timeout :( * revert unintended change * PR feedback: change logging * fix dumb
…egment methods (apache#5555) * add stopped check and handling to HttpLoadQueuePeon load and drop segment methods * fix unrelated timeout :( * revert unintended change * PR feedback: change logging * fix dumb
…egment methods (apache#5555) (apache#5960) * add stopped check and handling to HttpLoadQueuePeon load and drop segment methods * fix unrelated timeout :( * revert unintended change * PR feedback: change logging * fix dumb
Fixes #5542 by checking if
HttpLoadQueuePeon
is stopped before loading or dropping a segment, and adds a test to ensure callbacks execute.