-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-14384. Cleanup PipelineProvider and the related classes. #9608
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
ptlrs
left a comment
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 for the PR @szetszwo.
| private final long blockSize; | ||
| private final long minRatisVolumeSizeBytes; | ||
| private final long containerSize; |
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.
In this PR we are now using a final filter instead of instantiating a new one.
Will the Ozone configurations mapping to these properties ever be modified dynamically?
If so, the instantiated filter won't get the updated values for these configurations.
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.
@ptlrs , thanks for reviewing this!
There are final fields such as numPipelinesPerMetadataVolume in SCMNodeManager. It is consistent to make NonWritableNodeFilter final.
For reconf, SCM currently only supports reconf admins; see below. So, the filter cannot be modified dynamically.
Lines 401 to 407 in cbea8af
| reconfigurationHandler = | |
| new ReconfigurationHandler("SCM", conf, this::checkAdminAccess) | |
| .register(OZONE_ADMINISTRATORS, this::reconfOzoneAdmins) | |
| .register(OZONE_READONLY_ADMINISTRATORS, | |
| this::reconfOzoneReadOnlyAdmins); | |
| reconfigurationHandler.setReconfigurationCompleteCallback(reconfigurationHandler.defaultLoggingCallback()); |
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 for the explanation @szetszwo.
|
@ptlrs , @adoroszlai , thanks a lot for reviewing this! |
What changes were proposed in this pull request?
What is the link to the Apache JIRA
HDDS-14384
How was this patch tested?
By existing tests.