tasanuma commented on a change in pull request #3679:
URL: https://github.com/apache/hadoop/pull/3679#discussion_r764568831
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/balancer/Dispatcher.java
##########
@@ -848,16 +871,35 @@ private long getBlockList() throws IOException {
synchronized (block) {
block.clearLocations();
+ if (blkLocs instanceof StripedBlockWithLocations) {
+ // EC block may adjust indices before, avoid repeated adjustments
+ ((DBlockStriped) block).setIndices(
+ ((StripedBlockWithLocations) blkLocs).getIndices());
+ }
+
// update locations
+ List<Integer> adjustList = new ArrayList<>();
final String[] datanodeUuids = blkLocs.getDatanodeUuids();
final StorageType[] storageTypes = blkLocs.getStorageTypes();
for (int i = 0; i < datanodeUuids.length; i++) {
final StorageGroup g = storageGroupMap.get(
datanodeUuids[i], storageTypes[i]);
if (g != null) { // not unknown
block.addLocation(g);
+ } else if (blkLocs instanceof StripedBlockWithLocations) {
+ // some datanode may not in storageGroupMap due to
decommission operation
+ // or balancer cli with "-exclude" parameter
+ adjustList.add(i);
}
}
+
+ if (!adjustList.isEmpty()) {
+ // block.locations mismatch with block.indices
+ // adjust indices to get correct internalBlock for Datanode in
#getInternalBlock
+ ((DBlockStriped) block).adjustIndices(adjustList);
+ Preconditions.checkArgument(((DBlockStriped)
block).indices.length
Review comment:
As `Preconditions.checkArgument()` can throw `IllegalArgumentException`,
`getBlockList()` should declares it, and `dispatchBlocks()` should catch the
exception.
```diff
- private long getBlockList() throws IOException {
+ private long getBlockList() throws IOException,
IllegalArgumentException {
```
```diff
try {
final long received = getBlockList();
if (received == 0) {
return;
}
blocksToReceive -= received;
continue;
- } catch (IOException e) {
+ } catch (IOException|IllegalArgumentException e) {
LOG.warn("Exception while getting reportedBlock list", e);
return;
}
```
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/balancer/TestBalancer.java
##########
@@ -1615,6 +1636,107 @@ public void testBalancerWithStripedFile() throws
Exception {
NameNodeConnector.setWrite2IdFile(false);
}
+ @Test
Review comment:
Could you move `testBalancerWithExcludeListWithStripedFile()` and
`doTestBalancerWithExcludeListWithStripedFile ()` after
`doTestBalancerWithStripedFile`?
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/balancer/TestBalancer.java
##########
@@ -1615,6 +1619,92 @@ public void testBalancerWithStripedFile() throws
Exception {
NameNodeConnector.setWrite2IdFile(false);
}
+ @Test
+ public void testBalancerWithExcludeListWithStripedFile() throws Exception {
+ Configuration conf = new Configuration();
+ initConfWithStripe(conf);
+ NameNodeConnector.setWrite2IdFile(true);
+ doTestBalancerWithExcludeListWithStripedFile(conf);
+ NameNodeConnector.setWrite2IdFile(false);
+ }
+
+ private void doTestBalancerWithExcludeListWithStripedFile(Configuration
conf) throws Exception {
+ int numOfDatanodes = dataBlocks + parityBlocks + 3;
+ int numOfRacks = dataBlocks;
+ long capacity = 20 * defaultBlockSize;
+ long[] capacities = new long[numOfDatanodes];
+ Arrays.fill(capacities, capacity);
+ String[] racks = new String[numOfDatanodes];
+ for (int i = 0; i < numOfDatanodes; i++) {
+ racks[i] = "/rack" + (i % numOfRacks);
+ }
+ cluster = new MiniDFSCluster.Builder(conf)
+ .numDataNodes(numOfDatanodes)
+ .racks(racks)
+ .simulatedCapacities(capacities)
+ .build();
+
+ try {
+ cluster.waitActive();
+ client = NameNodeProxies.createProxy(conf,
cluster.getFileSystem(0).getUri(),
+ ClientProtocol.class).getProxy();
+ client.enableErasureCodingPolicy(
+ StripedFileTestUtil.getDefaultECPolicy().getName());
+ client.setErasureCodingPolicy("/",
+ StripedFileTestUtil.getDefaultECPolicy().getName());
+
+ long totalCapacity = sum(capacities);
+
+ // fill up the cluster with 30% data. It'll be 45% full plus parity.
+ long fileLen = totalCapacity * 3 / 10;
+ long totalUsedSpace = fileLen * (dataBlocks + parityBlocks) / dataBlocks;
+ FileSystem fs = cluster.getFileSystem(0);
+ DFSTestUtil.createFile(fs, filePath, fileLen, (short) 3, r.nextLong());
+
+ // verify locations of striped blocks
+ LocatedBlocks locatedBlocks = client.getBlockLocations(fileName, 0,
fileLen);
+ StripedFileTestUtil.verifyLocatedStripedBlocks(locatedBlocks, groupSize);
+
+ // get datanode report
+ DatanodeInfo[] datanodeReport =
client.getDatanodeReport(DatanodeReportType.ALL);
+
+ // add datanode in new rack
+ String newRack = "/rack" + (++numOfRacks);
+ cluster.startDataNodes(conf, 2, true, null,
+ new String[]{newRack, newRack}, null,
+ new long[]{capacity, capacity});
+ totalCapacity += capacity*2;
+ cluster.triggerHeartbeats();
+
+ // add datanode to exclude list
+ Set<String> dnList = new HashSet<>();
+ dnList.add(datanodeReport[0].getHostName());
+ BalancerParameters.Builder pBuilder = new BalancerParameters.Builder();
+ pBuilder.setExcludedNodes(dnList);
+ waitForHeartBeat(totalUsedSpace, totalCapacity, client, cluster);
+
+ // start balancer and check the failed num of moving task
+ Collection<URI> namenodes = DFSUtil.getInternalNsRpcUris(conf);
+ final int run = runBalancer(namenodes, pBuilder.build(), conf, true);
+ if (conf.getInt(
+ DFSConfigKeys.DFS_DATANODE_BALANCE_MAX_NUM_CONCURRENT_MOVES_KEY,
+ DFSConfigKeys.DFS_DATANODE_BALANCE_MAX_NUM_CONCURRENT_MOVES_DEFAULT)
+ == 0) {
+ assertEquals(ExitStatus.NO_MOVE_PROGRESS.getExitCode(), run);
+ } else {
+ assertEquals(ExitStatus.SUCCESS.getExitCode(), run);
+ }
+ waitForHeartBeat(totalUsedSpace, totalCapacity, client, cluster);
+
+ // verify locations of striped blocks
+ locatedBlocks = client.getBlockLocations(fileName, 0, fileLen);
+ StripedFileTestUtil.verifyLocatedStripedBlocks(locatedBlocks, groupSize);
Review comment:
It makes sense. How about using `GenericTestUtils#waitFor` for checking
it?
--
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]
