-
Notifications
You must be signed in to change notification settings - Fork 626
HDDS-6070. ContainerBalancerConfig doesn't read config from ozone-site.xml #2893
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
Changes from 6 commits
036f07a
0a88420
4552162
f74f473
2b0a78c
17a1a1c
19a0b94
009dc80
cb662c8
d0d0615
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,6 @@ | |
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| /** | ||
|
|
@@ -73,15 +72,15 @@ public final class ContainerBalancerConfiguration { | |
| private long maxSizeToMovePerIteration = 30 * OzoneConsts.GB; | ||
|
|
||
| @Config(key = "size.entering.target.max", type = ConfigType.SIZE, | ||
| defaultValue = "", tags = {ConfigTag.BALANCER}, description = "The " + | ||
| defaultValue = "6GB", tags = {ConfigTag.BALANCER}, description = "The " + | ||
| "maximum size that can enter a target datanode in each " + | ||
| "iteration while balancing. This is the sum of data from multiple " + | ||
| "sources. The default value is greater than the configured" + | ||
| " (or default) ozone.scm.container.size by 1GB.") | ||
| private long maxSizeEnteringTarget; | ||
|
|
||
| @Config(key = "size.leaving.source.max", type = ConfigType.SIZE, | ||
| defaultValue = "", tags = {ConfigTag.BALANCER}, description = "The " + | ||
| defaultValue = "6GB", tags = {ConfigTag.BALANCER}, description = "The " + | ||
| "maximum size that can leave a source datanode in each " + | ||
| "iteration while balancing. This is the sum of data moving to multiple " + | ||
| "targets. The default value is greater than the configured" + | ||
|
|
@@ -99,13 +98,13 @@ public final class ContainerBalancerConfiguration { | |
| private String excludeContainers = ""; | ||
|
|
||
| @Config(key = "move.timeout", type = ConfigType.TIME, defaultValue = "30m", | ||
| timeUnit = TimeUnit.MINUTES, tags = {ConfigTag.BALANCER}, description = | ||
| tags = {ConfigTag.BALANCER}, description = | ||
| "The amount of time in minutes to allow a single container to move " + | ||
| "from source to target.") | ||
| private long moveTimeout = Duration.ofMinutes(30).toMillis(); | ||
|
|
||
| @Config(key = "balancing.iteration.interval", type = ConfigType.TIME, | ||
| defaultValue = "1h", timeUnit = TimeUnit.MINUTES, tags = { | ||
| defaultValue = "1h", tags = { | ||
| ConfigTag.BALANCER}, description = "The interval period between each " + | ||
| "iteration of Container Balancer.") | ||
| private long balancingInterval; | ||
|
|
@@ -127,11 +126,11 @@ public final class ContainerBalancerConfiguration { | |
| private DUFactory.Conf duConf; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this isn't getting initialised now, we can remove this and its usages in the |
||
|
|
||
| /** | ||
| * Create configuration with default values. | ||
| * Modify configuration with default values. | ||
| * | ||
| * @param config Ozone configuration | ||
| */ | ||
| public ContainerBalancerConfiguration(OzoneConfiguration config) { | ||
| public void initialize(OzoneConfiguration config) { | ||
| Preconditions.checkNotNull(config, | ||
| "OzoneConfiguration should not be null."); | ||
| this.ozoneConfiguration = config; | ||
|
|
@@ -140,7 +139,7 @@ public ContainerBalancerConfiguration(OzoneConfiguration config) { | |
| // greater than container size | ||
| long size = (long) ozoneConfiguration.getStorageSize( | ||
| ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE, | ||
| ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.GB) + | ||
| ScmConfigKeys.OZONE_SCM_CONTAINER_SIZE_DEFAULT, StorageUnit.BYTES) + | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @siddhantsangwan Another issue is the StorageUnit here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was just going to raise a jira for this particular bug when I saw that you've pushed an update. Thanks!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems the other configs, like "move.timeout" and "balancing.interval", also have the same issue. |
||
| OzoneConsts.GB; | ||
| maxSizeEnteringTarget = size; | ||
| maxSizeLeavingSource = size; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -89,6 +89,7 @@ public class TestContainerBalancer { | |||
| private Map<DatanodeUsageInfo, Set<ContainerID>> datanodeToContainersMap = | ||||
| new HashMap<>(); | ||||
| private static final ThreadLocalRandom RANDOM = ThreadLocalRandom.current(); | ||||
| private static final double DELTA = 1e-15; | ||||
|
|
||||
| @Rule | ||||
| public TemporaryFolder tempFolder = new TemporaryFolder(); | ||||
|
|
@@ -102,7 +103,9 @@ public void setup() throws SCMException, NodeNotFoundException { | |||
| containerManager = Mockito.mock(ContainerManager.class); | ||||
| replicationManager = Mockito.mock(ReplicationManager.class); | ||||
|
|
||||
| balancerConfiguration = new ContainerBalancerConfiguration(conf); | ||||
| balancerConfiguration = | ||||
| conf.getObject(ContainerBalancerConfiguration.class); | ||||
| balancerConfiguration.initialize(conf); | ||||
| balancerConfiguration.setThreshold(0.1); | ||||
| balancerConfiguration.setIdleIteration(1); | ||||
| balancerConfiguration.setMaxDatanodesRatioToInvolvePerIteration(1.0d); | ||||
|
|
@@ -486,8 +489,11 @@ public void balancerShouldObeyMaxSizeEnteringTargetLimit() { | |||
| Assert.assertTrue(containerBalancer.getSourceToTargetMap().isEmpty()); | ||||
|
|
||||
| // some containers should be selected when using default values | ||||
| containerBalancer.start( | ||||
| new ContainerBalancerConfiguration(new OzoneConfiguration())); | ||||
| OzoneConfiguration ozoneConfiguration = new OzoneConfiguration(); | ||||
| ContainerBalancerConfiguration cbc = ozoneConfiguration. | ||||
| getObject(ContainerBalancerConfiguration.class); | ||||
| cbc.initialize(ozoneConfiguration); | ||||
| containerBalancer.start(cbc); | ||||
|
|
||||
| // waiting for balance completed. | ||||
| // TODO: this is a temporary implementation for now | ||||
|
|
@@ -589,6 +595,24 @@ public void balancerShouldFollowExcludeAndIncludeDatanodesConfigurations() { | |||
| } | ||||
| } | ||||
|
|
||||
| @Test | ||||
| public void testContainerBalancerConfiguration() { | ||||
| OzoneConfiguration ozoneConfiguration = new OzoneConfiguration(); | ||||
| ozoneConfiguration.set("ozone.scm.container.size", "5GB"); | ||||
| ozoneConfiguration.setDouble( | ||||
| "hdds.container.balancer.utilization.threshold", 0.01); | ||||
|
|
||||
| ContainerBalancerConfiguration cbConf = | ||||
| ozoneConfiguration.getObject(ContainerBalancerConfiguration.class); | ||||
| cbConf.initialize(ozoneConfiguration); | ||||
| Assert.assertEquals(cbConf.getThreshold(), 0.01d, DELTA); | ||||
|
|
||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we remove DELTA and use assertTrue(Doublu.compare(a,b) == 0)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have removed DELTA and use a similar comparison as Line 76 in 8b4d4a9
, please have a check. |
||||
| Assert.assertEquals(cbConf.getMaxSizeLeavingSource(), | ||||
| 6 * 1024 * 1024 * 1024L); | ||||
|
|
||||
| Assert.assertEquals(cbConf.getMoveTimeout().toMillis(), 30 * 60 * 1000); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Determines unBalanced nodes, that is, over and under utilized nodes, | ||||
| * according to the generated utilization values for nodes and the threshold. | ||||
|
|
||||
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.
may be we can add a static build function to
ContainerBalancerConfigurationclass to build and initialize a ContainerBalancerConfiguration instance, for example: