Uh oh!
There was an error while loading. Please reload this page.
HDDS-10917. Refactoring more tests from TestContainerBalancerTask - #6734
Conversation
…TestContainerBalancerDatanodeNodeLimit using MockedSCM
Montura
commented
May 31, 2024
@siddhantsangwan, take a look please |
Montura
commented
Jun 27, 2024
@siddhantsangwan , take a look please |
adoroszlai
commented
Jul 15, 2024
@afilpp@sarvekshayr@sumitagrawl@Tejaskriya can you please help review this? |
Tejaskriya
left a comment
There was a problem hiding this comment.
Thanks for the patch @Montura. Are you also planning to move the other tests in TestContainerBalancerTask to TestContainerBalancerDatanodeNodeLimit with new PRs?
| int nodeCount = mockedSCM.getCluster().getNodeCount(); | ||
| if (nodeCount < DATANODE_COUNT_LIMIT_FOR_SMALL_CLUSTER) { | ||
| config.setMaxDatanodesPercentageToInvolvePerIteration(100); | ||
| } | ||
| config.setIterations(1); | ||
| config.setThreshold(10); | ||
| config.setMaxSizeToMovePerIteration(50 * STORAGE_UNIT); | ||
| config.setMaxSizeEnteringTarget(50 * STORAGE_UNIT); |
There was a problem hiding this comment.
This seems to be a part of most of the tests in this class, causing a lot of code duplication. Can we move this to the setup? Any test that required different values, it can be set again in the test.
There was a problem hiding this comment.
Thanks for reviewing.
Actually, in the first version I hid this config settings in the class. But @siddhantsangwantold that is better to have config setup in each test. At least for lines 341-343.
I agree with you that all this config setup shouldn't be duplicated. I'm ready to fix this in next PR with next refactored tests.
What do you think?
There was a problem hiding this comment.
IMO we can move the default configs to the setup, any specific ones can be added in each test. In many other tests also we follow the same pattern
Montura
commented
Jul 25, 2024
Sure, I have plans to finish it in one or two PRs (@siddhantsangwan has told first time that too many lines per one PR). I'd like to finish in one PR of course. |
Tejaskriya
commented
Jul 26, 2024
Splitting it into many PRs will make it easy for reviews, it makes sense. |
Montura
commented
Jul 31, 2024
@adoroszlai , could you merge please? Or I have to do something else? |
sarvekshayr
left a comment
There was a problem hiding this comment.
Thanks @Montura for working on this patch. The changes LGTM.
adoroszlai
commented
Aug 1, 2024
Thanks @Montura for the patch, @sarvekshayr, @Tejaskriya for the review. |
In PR for HDDS-9889 we discussed with Siddhant Sangwan that tests form
org.apache.hadoop.hdds.scm.container.balancer.TestContainerBalancerTaskcould be refactored usingMockedSCMclass (introduced in HDDS-9889)Some work has been already done in:
What changes were proposed in this pull request?
org.apache.hadoop.hdds.scm.container.balancer.TestContainerBalancerTaskWhat is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10917
How was this patch tested?
Use standalone tests