Skip to content

HADOOP-17332. S3A MarkerTool -min and -max are inverted. #2425

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
steveloughran merged 2 commits into from
Nov 23, 2020
Merged

HADOOP-17332. S3A MarkerTool -min and -max are inverted. #2425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
003df8e
HADOOP-17332 s3a MarkerTool -min and -max are inverted.
steveloughran Oct 30, 2020
c32dae7
HADOOP-17332. Fix typo in precondition
steveloughran Nov 17, 2020
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
22 changes: 20 additions & 2 deletions hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/tools/MarkerTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.apache.hadoop.fs.s3a.impl.StoreContext;
import org.apache.hadoop.fs.s3a.s3guard.S3GuardTool;
import org.apache.hadoop.fs.shell.CommandFormat;
import org.apache.hadoop.thirdparty.com.google.common.base.Preconditions;
import org.apache.hadoop.util.DurationInfo;
import org.apache.hadoop.util.ExitUtil;

Expand Down Expand Up @@ -395,10 +396,22 @@ ScanResult execute(final ScanArgs scanArgs)
} else {
filterPolicy = null;
}
int minMarkerCount = scanArgs.getMinMarkerCount();
int maxMarkerCount = scanArgs.getMaxMarkerCount();
if (minMarkerCount > maxMarkerCount) {
// swap min and max if they are wrong.
// this is to ensure any test scripts written to work around
// HADOOP-17332 and min/max swapping continue to work.
println(out, "Swapping -min (%d) and -max (%d) values",
minMarkerCount, maxMarkerCount);
int m = minMarkerCount;
minMarkerCount = maxMarkerCount;
maxMarkerCount = m;
}
ScanResult result = scan(target,
scanArgs.isDoPurge(),
scanArgs.getMaxMarkerCount(),
scanArgs.getMinMarkerCount(),
minMarkerCount,
maxMarkerCount,
scanArgs.getLimit(),
filterPolicy);
return result;
Expand Down Expand Up @@ -513,6 +526,11 @@ private ScanResult scan(
final DirectoryPolicy filterPolicy)
throws IOException, ExitUtil.ExitException {

// safety check: min and max are correctly ordered at this point.
Preconditions.checkArgument(minMarkerCount <= maxMarkerCount,
Copy link
Contributor

@mukund-thakur mukund-thakur Nov 13, 2020

Choose a reason for hiding this comment

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

nit: Is this check required again since we have already swapped above?

Copy link
Contributor Author

@steveloughran steveloughran Nov 17, 2020

Choose a reason for hiding this comment

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

no, but I'm just being extra paranoid

Copy link

@bgaborg bgaborg Nov 23, 2020

Choose a reason for hiding this comment

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

it will harm nobody, we can leave this check here.

"The min marker count of %d is greater than the max value of %d",
minMarkerCount, maxMarkerCount);

ScanResult result = new ScanResult();

// Mission Accomplished
Expand Down
21 changes: 19 additions & 2 deletions hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/tools/ITestMarkerTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,25 @@ public void testRunAuditWithExpectedMarkers() throws Throwable {
AUDIT,
m(OPT_LIMIT), 0,
m(OPT_OUT), audit,
m(OPT_MIN), expectedMarkersWithBaseDir,
m(OPT_MAX), expectedMarkersWithBaseDir,
m(OPT_MIN), expectedMarkersWithBaseDir - 1,
m(OPT_MAX), expectedMarkersWithBaseDir + 1,
createdPaths.base);
expectMarkersInOutput(audit, expectedMarkersWithBaseDir);
}

@Test
public void testRunAuditWithExpectedMarkersSwappedMinMax() throws Throwable {
Copy link

@bgaborg bgaborg Nov 23, 2020

Choose a reason for hiding this comment

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

I wanted to ask for a test like this, but it's already here so LGTM.

Copy link
Contributor Author

@steveloughran steveloughran Nov 23, 2020

Choose a reason for hiding this comment

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

wanted to make sure that anyone in QE wouldn't be upset with the fix :)

describe("Run a verbose audit with the min/max ranges swapped;"
+ " see HADOOP-17332");
// a run under the keeping FS will create paths
CreatedPaths createdPaths = createPaths(getKeepingFS(), methodPath());
final File audit = tempAuditFile();
run(MARKERS, V,
AUDIT,
m(OPT_LIMIT), 0,
m(OPT_OUT), audit,
m(OPT_MIN), expectedMarkersWithBaseDir + 1,
m(OPT_MAX), expectedMarkersWithBaseDir - 1,
createdPaths.base);
expectMarkersInOutput(audit, expectedMarkersWithBaseDir);
}
Expand Down