ChenSammi commented on PR #3318: URL: https://github.com/apache/ozone/pull/3318#issuecomment-1121827728
> Sure. In `TestRatisPipelineProvider.testCreatePipelinesWhenNotEnoughSpace()`, the datanodes' details are created in `init(1, largeContainerConf);`, which can be trace back to `MockNodeManager.populateNodeMetric(...)`, so the datanodes are as follows: > > ``` > new NodeData(10L * OzoneConsts.TB, OzoneConsts.GB), > new NodeData(64L * OzoneConsts.TB, 100 * OzoneConsts.GB), > new NodeData(128L * OzoneConsts.TB, 256 * OzoneConsts.GB), > new NodeData(40L * OzoneConsts.TB, OzoneConsts.TB), > new NodeData(256L * OzoneConsts.TB, 200 * OzoneConsts.TB), > new NodeData(20L * OzoneConsts.TB, 10 * OzoneConsts.GB), > new NodeData(32L * OzoneConsts.TB, 16 * OzoneConsts.TB), > new NodeData(OzoneConsts.TB, 900 * OzoneConsts.GB), > new NodeData(OzoneConsts.TB, 900 * OzoneConsts.GB, NodeData.STALE), > new NodeData(OzoneConsts.TB, 200L * OzoneConsts.GB, NodeData.STALE), > new NodeData(OzoneConsts.TB, 200L * OzoneConsts.GB, NodeData.DEAD) > ``` > > In original implementation, the result of `pickNodesNotUsed()` only includes the first datanode, what the remaining capacity is `10TB - 1GB`, which doesn't fulfill the size requirement of `100TB`, so the original implemtation would throw exception. > > In new implementation, the result of `pickAllNodesNotUsed()` will include the first 8 datanodes, then `provider` will choose from the all 8 datanodes that fulfills the size requirement of `100TB`, and the result will be the third datanode, so exceptions won't be thrown. > > So in order to throw the exception to keep the original assert, we need to change the size to a larger value that all 8 datanodes can't fulfill. And the change will also verify the correctness of the new implementation. This explanation is very clear. However, I would suggest to add a new straightforward test case in case of this in future. That will help the reviewer understand the code change more easily. Then the code review will be more efficient. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
