Skip to content
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.

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

Hadoop 16479 #1207

Closed
wants to merge 5 commits into from
Closed

Hadoop 16479 #1207

wants to merge 5 commits into from

Conversation

@bilaharith
Copy link
Contributor

bilaharith commented Aug 1, 2019

The pattern to parse the timezone was wrong. Corrected the same.

@bilaharith

This comment has been minimized.

Copy link
Contributor Author

bilaharith commented Aug 1, 2019

Driver test results using a Namespace enabled account in Central India:

mvn -T 1C -Dparallel-tests=abfs -Dscale -DtestsThreadCount=8 clean verify

Tests run: 42, Failures: 0, Errors: 0, Skipped: 0

Tests run: 393, Failures: 0, Errors: 1, Skipped: 21

Tests run: 190, Failures: 0, Errors: 0, Skipped: 15

@hadoop-yetus

This comment has been minimized.

Copy link

hadoop-yetus commented Aug 1, 2019

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 39 Docker mode activated.
_ Prechecks _
+1 dupname 0 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 mvninstall 1060 trunk passed
+1 compile 29 trunk passed
+1 checkstyle 22 trunk passed
+1 mvnsite 35 trunk passed
+1 shadedclient 719 branch has no errors when building and testing our client artifacts.
+1 javadoc 22 trunk passed
0 spotbugs 53 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 49 trunk passed
_ Patch Compile Tests _
+1 mvninstall 28 the patch passed
+1 compile 22 the patch passed
+1 javac 22 the patch passed
-0 checkstyle 18 hadoop-tools/hadoop-azure: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 mvnsite 25 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 721 patch has no errors when building and testing our client artifacts.
+1 javadoc 16 the patch passed
+1 findbugs 53 the patch passed
_ Other Tests _
+1 unit 77 hadoop-azure in the patch passed.
+1 asflicense 25 The patch does not generate ASF License warnings.
3049
Subsystem Report/Notes
Docker Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/1/artifact/out/Dockerfile
GITHUB PR #1207
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux b8f68e78ec09 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / e111789
Default Java 1.8.0_212
checkstyle https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/1/artifact/out/diff-checkstyle-hadoop-tools_hadoop-azure.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/1/testReport/
Max. process+thread count 446 (vs. ulimit of 5500)
modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/1/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@@ -21,6 +21,7 @@
import java.io.IOException;

import org.apache.hadoop.fs.CommonConfigurationKeys;
import org.apache.hadoop.fs.FsStatus;

This comment has been minimized.

Copy link
@steveloughran

steveloughran Aug 5, 2019

Contributor

can you move this down with the other org.apache.hadoop group? We try to keep the org.apache imports together, and while it's generally too late to move things around in committed code (backport pain amplification), it's good to try and follow the plan on newer imports

This comment has been minimized.

Copy link
@bilaharith

bilaharith Aug 7, 2019

Author Contributor

Removed the import as the same is unused

long createEndTime = System.currentTimeMillis();
FileStatus fStat = fs.getFileStatus(testFilePath);
long lastModifiedTime = fStat.getModificationTime();
assertTrue((createStartTime / 1000) * 1000 - 1 < lastModifiedTime);

This comment has been minimized.

Copy link
@steveloughran

steveloughran Aug 5, 2019

Contributor

assertion needs a message

This comment has been minimized.

Copy link
@bilaharith

bilaharith Aug 7, 2019

Author Contributor

Done

// Dividing and multiplying by 1000 to make last 3 digits 0.
// It is observed that modification time is returned with last 3
// digits 0 always.
assertTrue(createEndTime > lastModifiedTime);

This comment has been minimized.

Copy link
@steveloughran

steveloughran Aug 5, 2019

Contributor

needs a message

This comment has been minimized.

Copy link
@bilaharith

bilaharith Aug 7, 2019

Author Contributor

Done

// digits 0 always.
assertTrue(createEndTime > lastModifiedTime);
}
}

This comment has been minimized.

Copy link
@steveloughran

steveloughran Aug 5, 2019

Contributor

nit: needs newline at the end

This comment has been minimized.

Copy link
@bilaharith

bilaharith Aug 7, 2019

Author Contributor

Done

@steveloughran

This comment has been minimized.

Copy link
Contributor

steveloughran commented Aug 5, 2019

regarding the current patch, production code looks OK, some minor changes suggested to the tests, primarily to satisfy my requirement "an assertion failure in a jenkins build to be informative enough to explain what is wrong". AssertTrue/false without any specifics never qualify there

But: that test is verifying that created files against the test zone are readable —will it verify that the problem seen in the JIRA -user in a different TZ gets a different time?

Some unit tests which just try to parse the strings should be able to do that, without needing to go near the object store

@bilaharith

This comment has been minimized.

Copy link
Contributor Author

bilaharith commented Aug 7, 2019

Hi @steveloughran
Both time comparisons in this test case is done with time in millis(long). Now fStat.getModificationTime() returns correct value. Previously fStat.getModificationTime() was not returning correct time in millis as the time was parsed in JVM time zone instead of UTC. This test ensures the correctness of fStat.getModificationTime().

@bilaharith

This comment has been minimized.

Copy link
Contributor Author

bilaharith commented Aug 7, 2019

Driver test results using a Namespace enabled account in Central India:

mvn -T 1C -Dparallel-tests=abfs -Dscale -DtestsThreadCount=8 clean verify

Tests run: 42, Failures: 0, Errors: 0, Skipped: 0

Tests run: 393, Failures: 0, Errors: 1, Skipped: 21

Tests run: 190, Failures: 0, Errors: 0, Skipped: 15

@hadoop-yetus

This comment has been minimized.

Copy link

hadoop-yetus commented Aug 8, 2019

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 82 Docker mode activated.
_ Prechecks _
+1 dupname 0 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 mvninstall 1211 trunk passed
+1 compile 27 trunk passed
+1 checkstyle 20 trunk passed
+1 mvnsite 31 trunk passed
+1 shadedclient 825 branch has no errors when building and testing our client artifacts.
+1 javadoc 24 trunk passed
0 spotbugs 58 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 56 trunk passed
_ Patch Compile Tests _
+1 mvninstall 31 the patch passed
+1 compile 24 the patch passed
+1 javac 24 the patch passed
+1 checkstyle 15 the patch passed
+1 mvnsite 28 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 871 patch has no errors when building and testing our client artifacts.
+1 javadoc 20 the patch passed
+1 findbugs 58 the patch passed
_ Other Tests _
+1 unit 79 hadoop-azure in the patch passed.
+1 asflicense 28 The patch does not generate ASF License warnings.
3520
Subsystem Report/Notes
Docker Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/2/artifact/out/Dockerfile
GITHUB PR #1207
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 51c760730966 4.15.0-52-generic #56-Ubuntu SMP Tue Jun 4 22:49:08 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 70b4617
Default Java 1.8.0_222
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/2/testReport/
Max. process+thread count 308 (vs. ulimit of 5500)
modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/2/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@hadoop-yetus

This comment has been minimized.

Copy link

hadoop-yetus commented Aug 8, 2019

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 45 Docker mode activated.
_ Prechecks _
+1 dupname 1 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
+1 mvninstall 1062 trunk passed
+1 compile 30 trunk passed
+1 checkstyle 23 trunk passed
+1 mvnsite 32 trunk passed
+1 shadedclient 735 branch has no errors when building and testing our client artifacts.
+1 javadoc 22 trunk passed
0 spotbugs 46 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 45 trunk passed
_ Patch Compile Tests _
+1 mvninstall 24 the patch passed
+1 compile 20 the patch passed
+1 javac 20 the patch passed
+1 checkstyle 14 the patch passed
+1 mvnsite 24 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 692 patch has no errors when building and testing our client artifacts.
+1 javadoc 17 the patch passed
+1 findbugs 49 the patch passed
_ Other Tests _
+1 unit 74 hadoop-azure in the patch passed.
+1 asflicense 25 The patch does not generate ASF License warnings.
2998
Subsystem Report/Notes
Docker Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/3/artifact/out/Dockerfile
GITHUB PR #1207
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux a2f22d2f98eb 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 00b5a27
Default Java 1.8.0_212
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/3/testReport/
Max. process+thread count 422 (vs. ulimit of 5500)
modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1207/3/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@adamantal

This comment has been minimized.

Copy link
Contributor

adamantal commented Aug 8, 2019

LGTM (non-binding).

@steveloughran

This comment has been minimized.

Copy link
Contributor

steveloughran commented Aug 8, 2019

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.