-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-8182. Add volume and container choosing policy #4408
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
|
@ChenSammi Could you help to review the PR? |
| DefaultContainerChoosingPolicy.class, | ||
| ContainerChoosingPolicy.class).newInstance(); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); |
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.
Please log error meg here.
|
|
||
| @Override | ||
| public ContainerData chooseContainer(OzoneContainer ozoneContainer, | ||
| HddsVolume hddsVolume, Set<Long> inProgressContainerIDs) { |
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 think we should turn this API into a batch API.
ozoneContainer.getController().getContainers(hddsVolume) will be a quite expensive operation once the container per volume reach a high number. Try to mostly leverage the result in one chooseContainer call, batch allocation or cache the container list of this volume for a while.
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.
Added a new parameter "targetSize" for batch retrieve of containers here.
|
|
||
| // Can not generate DiskBalancerTask if we have less than 2 results | ||
| if (volumes.size() <= 1) { | ||
| return null; |
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.
Please log a debug level msg for this case.
| return diskBalancerConfiguration; | ||
| } | ||
| } No newline at end of file | ||
| } |
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.
Please revert this unnecessary change in this and the following two files.
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.
The checkstyle will prompt the following warnings:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DiskBalancerStatus.java
1: File does not end with a newline.
| LoggerFactory.getLogger(DiskBalancerConfiguration.class); | ||
|
|
||
| public static final String HDDS_DATANODE_DISK_BALANCER_VOLUME_CHOOSING_POLICY | ||
| = "hdds.datanode.disk.balancer.volume.choosing.policy"; |
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.
Please use the @config annotation for these two properties.
|
LGTM, +1. |
What changes were proposed in this pull request?
This ticket is to add ContainerChoosingPolicy and VolumeChoosingPolicy for DiskBalancer Service.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8182
How was this patch tested?
unit test.