-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
YARN-11188. Only files belong to the first file controller are removed even if multiple log aggregation file controllers are configured #4468
Conversation
💔 -1 overall
This message was automatically generated. |
606a453
to
fcd1aad
Compare
💔 -1 overall
This message was automatically generated. |
🎊 +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.
Thank you for the fix @szilard-nemeth. It looks good to me but I had one concern that should be addressed in my opinion.
for (LogDeletionTask task : tasks) { | ||
if (task != null && task.getRMClient() != null) { | ||
RPC.stopProxy(task.getRMClient()); | ||
} |
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.
There is only one common RMClient is created, however, you stop this RMClient three times, which could be problematic, or at best redundant. I presume the client itself is thread safe, thus safe to share between tasks, but it should only be stopped once.
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.
Thanks @9uapaw ,
Nice catch.
Added a safety net for this: The tests are now verifying that the RM client is only closed once.
Please check the latest commit.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks for the fix @szilard-nemeth. Committed to trunk. |
…gationFilesBuilder
Description of PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?