Skip to content

HDFS-16227. De-flake TestMover#testMoverWithStripedFile #3429

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
ferhui merged 4 commits into from
Sep 18, 2021
Merged

HDFS-16227. De-flake TestMover#testMoverWithStripedFile #3429

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ebc4800
HDFS-16227. De-flake TestMover#testMoverWithStripedFile
virajjasani Sep 13, 2021
8c520fc
checkstyle fix
virajjasani Sep 13, 2021
3ddebe0
update the log - addressing review
virajjasani Sep 16, 2021
8871564
refactored wait logic - addressing review
virajjasani Sep 17, 2021
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
31 changes: 23 additions & 8 deletions ...hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/mover/TestMover.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,7 +872,6 @@ void initConfWithStripe(Configuration conf) {
public void testMoverWithStripedFile() throws Exception {
final Configuration conf = new HdfsConfiguration();
initConfWithStripe(conf);

Copy link
Member

@goiri goiri Sep 15, 2021

Choose a reason for hiding this comment

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

I would avoid removing this empty line (and 987).

Copy link
Contributor Author

@virajjasani virajjasani Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review @goiri. The reason why I had to remove at least 2 lines is because of checkstyle warning, this is part of build#1 :)

Screenshot 2021-09-16 at 11 23 31 AM

./hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/mover/TestMover.java:871:  @Test(timeout = 300000):3: Method length is 151 lines (max allowed is 150). [MethodLength]

Copy link
Contributor Author

@virajjasani virajjasani Sep 17, 2021

Choose a reason for hiding this comment

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

Are you fine with this @goiri? After the recent commit, the method has exactly 150 lines, one more line will create checkstyle warning.
Thanks

Copy link
Member

@goiri goiri Sep 17, 2021

Choose a reason for hiding this comment

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

If the goal is to avoid the 150 lines issue, I would prefer trying to make refactor the method a little instead of just removing lines that hurt readability. Not strongly opposed but not ideal.

Copy link
Contributor Author

@virajjasani virajjasani Sep 17, 2021

Choose a reason for hiding this comment

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

Not strongly opposed but not ideal.

I agree with your review, addressed in the latest commit. No change in logic, just the refactor as you have suggested.

// start 10 datanodes
int numOfDatanodes =10;
int storagesPerDatanode=2;
Expand Down Expand Up @@ -958,13 +957,30 @@ public void testMoverWithStripedFile() throws Exception {
new String[] { "-p", barDir });
Assert.assertEquals("Movement to ARCHIVE should be successful", 0, rc);

// verify storage types and locations
locatedBlocks = client.getBlockLocations(fooFile, 0, fileLen);
for(LocatedBlock lb : locatedBlocks.getLocatedBlocks()){
for( StorageType type : lb.getStorageTypes()){
Assert.assertEquals(StorageType.ARCHIVE, type);
// Verify storage types and locations.
// Wait until Namenode confirms ARCHIVE storage type for all blocks of
// fooFile.
GenericTestUtils.waitFor(() -> {
LocatedBlocks blocks;
try {
blocks = client.getBlockLocations(fooFile, 0, fileLen);
} catch (IOException e) {
throw new RuntimeException(e);
}
}
for (LocatedBlock lb : blocks.getLocatedBlocks()) {
for (StorageType type : lb.getStorageTypes()) {
if (!StorageType.ARCHIVE.equals(type)) {
LOG.info("Block {} has StorageType: {}. It might not have been "
+ "updated yet, awaiting the latest update.",
lb.getBlock().toString(), type);
return false;
}
}
}
return true;
}, 500, 5000, "Blocks storage type must be ARCHIVE");

locatedBlocks = client.getBlockLocations(fooFile, 0, fileLen);
StripedFileTestUtil.verifyLocatedStripedBlocks(locatedBlocks,
dataBlocks + parityBlocks);

Expand All @@ -984,7 +1000,6 @@ public void testMoverWithStripedFile() throws Exception {
{ StorageType.SSD, StorageType.DISK } },
true, null, null, null, capacities, null, false, false, false, null);
cluster.triggerHeartbeats();

// move file blocks to ONE_SSD policy
client.setStoragePolicy(barDir, "ONE_SSD");

Expand Down