-
Notifications
You must be signed in to change notification settings - Fork 503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-11410. Refactoring more tests from TestContainerBalancerTask #7156
base: master
Are you sure you want to change the base?
Conversation
@Tejaskriya, take a look please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR seems good to me, just a small comment below.
if (nodeCount < DATANODE_COUNT_LIMIT_FOR_SMALL_CLUSTER) { | ||
config.setMaxDatanodesPercentageToInvolvePerIteration(100); | ||
} | ||
config.setThreshold(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the threshold has been removed here and in few more tests below, but in the newly introduced ContainerBalancerConfigBuilder, we aren't setting it. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tejaskriya , thank for a review. I forgot about threshold thanks, I'll fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ready
/pending set threshold in tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking this issue as un-mergeable as requested.
Please use /ready
comment when it's resolved.
Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)
set threshold in tests
…terationResultTimeout test
@adoroszlai , you mean explicitly write |
@Montura no, I only meant that based on this conversation something needs to be done about setting threshold. If you have addressed that, please reply |
/ready |
Blocking review request is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks for the patch @Montura
Looks like this increases test run time from:
to:
So 2x for The following test cases take more than 1 second:
|
There are 3 possible solutions:
|
Can you please explore how much |
- initializeIterationShouldUpdateUnBalancedNodesWhenThresholdChanges (reduce iterations 50 -> 10 - checkIterationResultTimeout (increase maxEnteringSize, reduce timeouts)
UPD:
|
In PR for HDDS-9889 we discussed with Siddhant Sangwan that tests form
org.apache.hadoop.hdds.scm.container.balancer.TestContainerBalancerTask
could be refactored usingMockedSCM
class (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.TestContainerBalancerTask
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11410
How was this patch tested?
Use standalone tests