You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/04/19 08:33:46 UTC
[GitHub] [ozone] symious opened a new pull request, #3318: HDDS-6602. Incorrect logic in PipelineProvider.pickNodesNotUsed
symious opened a new pull request, #3318:
URL: https://github.com/apache/ozone/pull/3318
## What changes were proposed in this pull request?
In PipelineProvider, pickNodesNotUsed(CONFIG, long, long) filter the results from pickNodesNotUsed(CONFIG) based on metadata/data size.
But in pickNodesNotUsed(CONFIG), the result size is limited by the ReplicationConfig's required node, which means many other usable nodes will not be seen by pickNodesNotUsed(CONFIG, long, long).
This ticket is to solve this issue.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6602
## How was this patch tested?
unit test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3318: HDDS-6602. Incorrect logic in PipelineProvider.pickNodesNotUsed
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #3318:
URL: https://github.com/apache/ozone/pull/3318#issuecomment-1121825841
The patch LGTM, +1.
Thanks @symious for fix this, and @errose28 for the code review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3318: HDDS-6602. Incorrect logic in PipelineProvider.pickNodesNotUsed
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3318:
URL: https://github.com/apache/ozone/pull/3318#issuecomment-1102293995
@adoroszlai @errose28 Could you help to review?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3318: HDDS-6602. Incorrect logic in PipelineProvider.pickNodesNotUsed
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3318:
URL: https://github.com/apache/ozone/pull/3318#issuecomment-1105927178
@errose28 Thanks for the review.
> Can you clarify how the test changes are related to the bug?
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 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #3318: HDDS-6602. Incorrect logic in PipelineProvider.pickNodesNotUsed
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] symious commented on pull request #3318: HDDS-6602. Incorrect logic in PipelineProvider.pickNodesNotUsed
Posted by GitBox <gi...@apache.org>.
symious commented on PR #3318:
URL: https://github.com/apache/ozone/pull/3318#issuecomment-1121829200
@ChenSammi @errose28 Thank you for the review.
> 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.
Noted with thanks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #3318: HDDS-6602. Incorrect logic in PipelineProvider.pickNodesNotUsed
Posted by GitBox <gi...@apache.org>.
ChenSammi merged PR #3318:
URL: https://github.com/apache/ozone/pull/3318
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org