HDDS-5797. Support setting Datanode Reserved Space in MiniOzoneCluster.#2695
Conversation
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks for working on this @siddhantsangwan.
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.OptionalInt; | ||
| import java.util.UUID; | ||
| import java.util.concurrent.TimeoutException; |
There was a problem hiding this comment.
Nit: please avoid moving around imports unnecessarily.
| datanodeReservedSpace.ifPresent( | ||
| s -> dnConf.set(ScmConfigKeys.HDDS_DATANODE_DIR_DU_RESERVED, | ||
| dir + ":" + s)); |
There was a problem hiding this comment.
Since the same config property is set for each volume, reserved space would only be set for the last one. It should be set once, outside the loop, similar to HDDS_DATANODE_DIR_KEY.
Can you please add an assertion in TestMiniOzoneCluster#testMultipleDataDirs to verify behavior?
There was a problem hiding this comment.
For the unit test: Is there a better way than adding a reservedInBytes getter to VolumeInfo for testing, and then asserting its value for each volume?
There was a problem hiding this comment.
Is there a better way than adding a
reservedInBytesgetter toVolumeInfofor testing, and then asserting its value for each volume?
Sounds good enough to me.
| protected Optional<String> omId = Optional.empty(); | ||
|
|
||
| protected Boolean randomContainerPort = true; | ||
| protected Optional<String> datanodeReservedSpace = Optional.of("0B"); |
There was a problem hiding this comment.
I think a better default value would be:
| protected Optional<String> datanodeReservedSpace = Optional.of("0B"); | |
| protected Optional<String> datanodeReservedSpace = Optional.empty(); |
|
@adoroszlai Thanks for the review. I've updated the PR. |
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @siddhantsangwan for updating the patch.
JacksonYao287
left a comment
JacksonYao287
left a comment
There was a problem hiding this comment.
thanks @siddhantsangwan for this work, the changes looks good to me , +1!
|
Thanks @siddhantsangwan for the patch, @JacksonYao287 for the review. |
3 participants
What changes were proposed in this pull request?
Support setting
org.apache.hadoop.hdds.scm.ScmConfigKeys#HDDS_DATANODE_DIR_DU_RESERVEDfor each volume in each Datanode inMiniOzoneCluster(sets the same reserved space for each volume).What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5797
How was this patch tested?
Passed
TestMiniOzoneCluster