Skip to content

HDDS-11350. NullPointerException thrown on checking container balancer status #7134

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.

Sign up for GitHub

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

Merged
siddhantsangwan merged 2 commits into from
Sep 4, 2024
Merged

HDDS-11350. NullPointerException thrown on checking container balancer status #7134

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba46197
HDDS-11350. NullPointerException thrown on checking container balance…
siddhantsangwan Aug 29, 2024
34cb6fe
initialise findTargetStrategy in the constructor instead of null-check
siddhantsangwan Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions ...cm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,16 @@ public List<ContainerBalancerTaskIterationStatusInfo> getCurrentIterationsStatis
.max()
.orElse(0);

// the balancing thread could go to sleep without having initialized findTargetStrategy, so we check for null here
Map<DatanodeDetails, Long> sizeEnteringNodes = Collections.emptyMap();
if (findTargetStrategy != null) {

@juncevich juncevich Aug 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is non-obvious, that findTarget(Source)Strategy can be null. Maybe it will be better to use some default realisation with empty map, if org.apache.hadoop.hdds.scm.container.balancer.ContainerBalancerTask#initializeIteration was not executed?

@siddhantsangwan siddhantsangwan Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean here. Are you suggesting initialising findTargetStrategy in the constructor of ContainerBalancerTask (like findSourceStrategy)? That should work as well.

@juncevich juncevich Sep 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Yes, you are right. That's what I mean)

sizeEnteringNodes = findTargetStrategy.getSizeEnteringNodes();
}
Map<DatanodeDetails, Long> sizeLeavingNodes = Collections.emptyMap();

@juncevich juncevich Aug 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it will be better to create empty map only if findSourceStrategy == null?

    if (findSourceStrategy == null) {
      sizeLeavingNodes = Collections.emptyMap();
    } else {
      sizeLeavingNodes = findSourceStrategy.getSizeLeavingNodes();
    }

@siddhantsangwan siddhantsangwan Sep 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - we can come back to this after we've resolved your first review comment above.

if (findSourceStrategy != null) {
sizeLeavingNodes = findSourceStrategy.getSizeLeavingNodes();
}

ContainerBalancerTaskIterationStatusInfo currentIterationStatistic = new ContainerBalancerTaskIterationStatusInfo(
lastIterationNumber + 1,
null,
Expand All @@ -350,8 +360,7 @@ public List<ContainerBalancerTaskIterationStatusInfo> getCurrentIterationsStatis
metrics.getNumContainerMovesCompletedInLatestIteration(),
metrics.getNumContainerMovesFailedInLatestIteration(),
metrics.getNumContainerMovesTimeoutInLatestIteration(),
findTargetStrategy.getSizeEnteringNodes()
.entrySet()
sizeEnteringNodes.entrySet()
.stream()
.filter(Objects::nonNull)
.filter(datanodeDetailsLongEntry -> datanodeDetailsLongEntry.getValue() > 0)
Expand All @@ -360,8 +369,7 @@ public List<ContainerBalancerTaskIterationStatusInfo> getCurrentIterationsStatis
entry -> entry.getValue() / OzoneConsts.GB
)
),
findSourceStrategy.getSizeLeavingNodes()
.entrySet()
sizeLeavingNodes.entrySet()
.stream()
.filter(Objects::nonNull)
.filter(datanodeDetailsLongEntry -> datanodeDetailsLongEntry.getValue() > 0)
Expand Down
2 changes: 1 addition & 1 deletion ...dds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/MockedSCM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public MockedSCM(@Nonnull TestableCluster testableCluster) {
}
}

private void init(@Nonnull ContainerBalancerConfiguration balancerConfig, @Nonnull OzoneConfiguration ozoneCfg) {
void init(@Nonnull ContainerBalancerConfiguration balancerConfig, @Nonnull OzoneConfiguration ozoneCfg) {
ozoneCfg.setFromObject(balancerConfig);
try {
doMock(balancerConfig, ozoneCfg);
Expand Down
28 changes: 28 additions & 0 deletions ...t/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerStatusInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
package org.apache.hadoop.hdds.scm.container.balancer;

import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.scm.server.StorageContainerManager;
import org.apache.hadoop.ozone.OzoneConsts;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import java.util.List;
Expand Down Expand Up @@ -55,4 +57,30 @@ void testGetIterationStatistics() {
});

}

/**
* @see <a href="https://issues.apache.org/jira/browse/HDDS-11350">HDDS-11350</a>
*/
@Test
void testGetCurrentIterationsStatisticDoesNotThrowNullPointerExceptionWhenBalancingThreadIsSleeping() {
MockedSCM mockedScm = new MockedSCM(new TestableCluster(10, OzoneConsts.GB));
OzoneConfiguration ozoneConfig = new OzoneConfiguration();
ContainerBalancerConfiguration config = ozoneConfig.getObject(ContainerBalancerConfiguration.class);

config.setIterations(2);
// the following config makes the balancing thread go to sleep while waiting for DU to be triggered in DNs and
// updated storage reports to arrive via DN heartbeats - of course, this is a unit test and NodeManager, DNs etc.
// are all mocked
config.setTriggerDuEnable(true);
mockedScm.init(config, ozoneConfig);

// run ContainerBalancerTask in a new thread and have the current thread call getCurrentIterationsStatistic
StorageContainerManager scm = mockedScm.getStorageContainerManager();
ContainerBalancer cb = new ContainerBalancer(scm);
ContainerBalancerTask task = new ContainerBalancerTask(scm, 0, cb, cb.getMetrics(), config, false);
Thread thread = new Thread(task);
thread.setDaemon(true);
thread.start();
Assertions.assertDoesNotThrow(task::getCurrentIterationsStatistic);
}
}