sodonnel commented on PR #4645: URL: https://github.com/apache/ozone/pull/4645#issuecomment-1533159333
Change looks good. The only two suggests I have: 1. I wonder if we should make the "scaling factor" which we increase the limit and thread pool by configurable with a default of 2? If we make it configurable, should it be a decimal rather than an integer so we can scale by 1.5, 2.5 etc? I guess the config would need to apply on both the DN and RM, so I am not 100% sure where we should define it. It would be a shame to need two configs as they could get out of sync. 2. Might be good to also add a test based on `testSendThrottledReplicateContainerCommand` in the `TestReplicationManager` class to validate the decommissioning host is picked as a target when all nodes are over the original limit. This is kind of covered in the excluded nodes test, but excludes nodes are only updated when the new command pushes it over the limit. This test would ensure decommissioning nodes are still picked if they are over the original limit but under the extended limit. -- 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]
