HADOOP-17332. S3A MarkerTool -min and -max are inverted. #2425
Conversation
This patch * fixes the inversion * adds a precondition check * if the commands are supplied inverted, swaps them with a warning. This is to stop breaking any tests written to cope with the existing behavior. * adds tests for all this Change-Id: Ibff4201abb0c26332c96c9c701901e0611b43ab8
|
Anyone got any opinions here @mukund-thakur @mehakmeet @bgaborg ? |
This comment has been minimized.
This comment has been minimized.
|
|
||
| // safety check: min and max are correctly ordered at this point. | ||
| Preconditions.checkArgument(minMarkerCount <= maxMarkerCount, | ||
| "Min marker count of %d is greather than the max value of %d", |
| throws IOException, ExitUtil.ExitException { | ||
|
|
||
| // safety check: min and max are correctly ordered at this point. | ||
| Preconditions.checkArgument(minMarkerCount <= maxMarkerCount, |
There was a problem hiding this comment.
nit: Is this check required again since we have already swapped above?
There was a problem hiding this comment.
no, but I'm just being extra paranoid
There was a problem hiding this comment.
it will harm nobody, we can leave this check here.
mukund-thakur
left a comment
mukund-thakur
left a comment
There was a problem hiding this comment.
LGTM apart from just a minor question.
steveloughran
added
bug
fs/s3
Change-Id: I75916829a95605869fa7088d28ed63e95854f11b
This comment has been minimized.
This comment has been minimized.
bgaborg
left a comment
bgaborg
left a comment
There was a problem hiding this comment.
+1, and there is a new test so these values won't be swapped anymore.
| throws IOException, ExitUtil.ExitException { | ||
|
|
||
| // safety check: min and max are correctly ordered at this point. | ||
| Preconditions.checkArgument(minMarkerCount <= maxMarkerCount, |
There was a problem hiding this comment.
it will harm nobody, we can leave this check here.
| } | ||
|
|
||
| @Test | ||
| public void testRunAuditWithExpectedMarkersSwappedMinMax() throws Throwable { |
There was a problem hiding this comment.
I wanted to ask for a test like this, but it's already here so LGTM.
There was a problem hiding this comment.
wanted to make sure that anyone in QE wouldn't be upset with the fix :)
|
merged to trunk; will backport with a retest and patch of the guava import |
This patch * fixes the inversion * adds a precondition check * if the commands are supplied inverted, swaps them with a warning. This is to stop breaking any tests written to cope with the existing behavior. Contributed by Steve Loughran Change-Id: I15c40863f0db0675c7d60db477cb3bf1693cae49
…pache#2425) This patch * fixes the inversion * adds a precondition check * if the commands are supplied inverted, swaps them with a warning. This is to stop breaking any tests written to cope with the existing behavior. Contributed by Steve Loughran Change-Id: I15c40863f0db0675c7d60db477cb3bf1693cae49
5 participants
This patch
This is to stop breaking any tests written to cope with the existing
behavior.
Tested (IMarkerTool tests): S3 london