Details
-
Type:
Improvement
-
Status: Resolved
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: 3.0.0-alpha2
-
Component/s: None
-
Labels:None
-
Hadoop Flags:Reviewed
Description
TransferFsImage#doGetUrl downloads files from the specified url and stores them in the specified storage location. HDFS-4025 plans to synchronize the log segments in JournalNodes. If a log segment is missing from a JN, the JN downloads it from another JN which has the required log segment. We need TransferFsImage#doGetUrl and TransferFsImage#receiveFile to accomplish this.
So we propose to move the said functions to a Utility class so as to be able to use it for JournalNode syncing as well, without duplication of code.
Issue Links
- blocks
-
HDFS-4025
QJM: Sychronize past log segments to JNs that missed them
-
- Resolved
-
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
Thanks for the patch, Harsha Koneru! The patch looks good to me. Just two nits:
- We can use this chance to cleanup the imports of TransferFsImage and Util.
- In Util.java, CONTENT_LENGTH, MD5_HEADER, and deleteTmpFiles do not need to be public.
Besides we can add a little more details in the description to explain why moving the code is necessary.
Thank you Jing Zhao for reviewing the patch. I have addressed your comments in patch v01.
 -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 12s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
| +1 | mvninstall | 13m 46s | trunk passed |
| +1 | compile | 0m 50s | trunk passed |
| +1 | checkstyle | 0m 29s | trunk passed |
| +1 | mvnsite | 0m 58s | trunk passed |
| +1 | mvneclipse | 0m 14s | trunk passed |
| +1 | findbugs | 1m 54s | trunk passed |
| +1 | javadoc | 0m 44s | trunk passed |
| +1 | mvninstall | 0m 54s | the patch passed |
| +1 | compile | 0m 51s | the patch passed |
| +1 | javac | 0m 51s | the patch passed |
| -0 | checkstyle | 0m 27s | hadoop-hdfs-project/hadoop-hdfs: The patch generated 2 new + 20 unchanged - 2 fixed = 22 total (was 22) |
| +1 | mvnsite | 0m 56s | the patch passed |
| +1 | mvneclipse | 0m 12s | the patch passed |
| +1 | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 | findbugs | 2m 7s | the patch passed |
| +1 | javadoc | 0m 42s | the patch passed |
| -1 | unit | 80m 8s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
| 107m 6s |
| Reason | Tests |
|---|---|
| Timed out junit tests | org.apache.hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureToleration |
| Subsystem | Report/Notes |
|---|---|
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | |
| JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12845604/HDFS-11273.001.patch |
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
| uname | Linux 6740543e8724 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / a0a2761 |
| Default Java | 1.8.0_111 |
| findbugs | v3.0.0 |
| checkstyle | https://builds.apache.org/job/PreCommit-HDFS-Build/18023/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt |
| unit | https://builds.apache.org/job/PreCommit-HDFS-Build/18023/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/18023/testReport/ |
| modules | C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs |
| Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/18023/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks for updating the patch, Harsha Koneru! The updated patch is almost there. I have several extra minor comments (sorry I did not mention them in my last review...):
- The new Util#setTimeout method may no longer only load timeout value from "DFS_IMAGE_TRANSFER_TIMEOUT_KEY". Thus the code loading timeout from configuration can be left in TransferFsImage#doGetUrl.
+ /** + * Sets a timeout value in millisecods for the Http connection. + * @param connection the Http connection for which timeout needs to be set + * @param timeout value to be set as timeout in milliseconds + */ + public static void setTimeout(HttpURLConnection connection, int timeout) { + if (timeout <= 0) { + Configuration conf = new HdfsConfiguration(); + timeout = conf.getInt( + DFSConfigKeys.DFS_IMAGE_TRANSFER_TIMEOUT_KEY, + DFSConfigKeys.DFS_IMAGE_TRANSFER_TIMEOUT_DEFAULT); + LOG.info("Image Transfer timeout configured to " + timeout + + " milliseconds"); + } + + if (timeout > 0) { + connection.setConnectTimeout(timeout); + connection.setReadTimeout(timeout); + } + }
- HttpGetFailedException can be defined as an upper level class and be moved to the o.a.h.hdfs.server.common package.
- The following code can be reformatted.
+ public static MD5Hash doGetUrl(URL url, List<File> localPaths, + Storage dstStorage, boolean getChecksum, URLConnectionFactory + connectionFactory, int ioFileBufferSize, boolean isSpnegoEnabled, int + timeout) throws IOException {
Thank you Jing Zhao. I have uploaded patch v02 with the new changes.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 11s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
| +1 | mvninstall | 13m 21s | trunk passed |
| +1 | compile | 0m 56s | trunk passed |
| +1 | checkstyle | 0m 28s | trunk passed |
| +1 | mvnsite | 1m 0s | trunk passed |
| +1 | mvneclipse | 0m 14s | trunk passed |
| +1 | findbugs | 1m 54s | trunk passed |
| +1 | javadoc | 0m 41s | trunk passed |
| -1 | mvninstall | 0m 27s | hadoop-hdfs in the patch failed. |
| -1 | compile | 0m 27s | hadoop-hdfs in the patch failed. |
| -1 | javac | 0m 27s | hadoop-hdfs in the patch failed. |
| -0 | checkstyle | 0m 28s | hadoop-hdfs-project/hadoop-hdfs: The patch generated 11 new + 19 unchanged - 3 fixed = 30 total (was 22) |
| -1 | mvnsite | 0m 29s | hadoop-hdfs in the patch failed. |
| +1 | mvneclipse | 0m 12s | the patch passed |
| -1 | whitespace | 0m 0s | The patch 10 line(s) with tabs. |
| -1 | findbugs | 0m 27s | hadoop-hdfs in the patch failed. |
| -1 | javadoc | 0m 38s | hadoop-hdfs-project_hadoop-hdfs generated 1 new + 7 unchanged - 0 fixed = 8 total (was 7) |
| -1 | unit | 0m 25s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
| 23m 47s |
This message was automatically generated.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 13s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
| +1 | mvninstall | 23m 55s | trunk passed |
| +1 | compile | 0m 46s | trunk passed |
| +1 | checkstyle | 0m 26s | trunk passed |
| +1 | mvnsite | 0m 50s | trunk passed |
| +1 | mvneclipse | 0m 12s | trunk passed |
| +1 | findbugs | 1m 44s | trunk passed |
| +1 | javadoc | 0m 39s | trunk passed |
| -1 | mvninstall | 0m 25s | hadoop-hdfs in the patch failed. |
| -1 | compile | 0m 23s | hadoop-hdfs in the patch failed. |
| -1 | javac | 0m 23s | hadoop-hdfs in the patch failed. |
| -0 | checkstyle | 0m 24s | hadoop-hdfs-project/hadoop-hdfs: The patch generated 11 new + 19 unchanged - 3 fixed = 30 total (was 22) |
| -1 | mvnsite | 0m 25s | hadoop-hdfs in the patch failed. |
| +1 | mvneclipse | 0m 9s | the patch passed |
| -1 | whitespace | 0m 0s | The patch 10 line(s) with tabs. |
| -1 | findbugs | 0m 23s | hadoop-hdfs in the patch failed. |
| -1 | javadoc | 0m 36s | hadoop-hdfs-project_hadoop-hdfs generated 1 new + 7 unchanged - 0 fixed = 8 total (was 7) |
| -1 | unit | 0m 28s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 18s | The patch does not generate ASF License warnings. |
| 33m 27s |
This message was automatically generated.
Fixing jenkings errors in v03.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 12s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
| +1 | mvninstall | 14m 38s | trunk passed |
| +1 | compile | 0m 54s | trunk passed |
| +1 | checkstyle | 0m 32s | trunk passed |
| +1 | mvnsite | 0m 57s | trunk passed |
| +1 | mvneclipse | 0m 14s | trunk passed |
| +1 | findbugs | 2m 3s | trunk passed |
| +1 | javadoc | 0m 40s | trunk passed |
| +1 | mvninstall | 0m 54s | the patch passed |
| +1 | compile | 0m 49s | the patch passed |
| +1 | javac | 0m 49s | the patch passed |
| -0 | checkstyle | 0m 29s | hadoop-hdfs-project/hadoop-hdfs: The patch generated 3 new + 129 unchanged - 4 fixed = 132 total (was 133) |
| +1 | mvnsite | 0m 52s | the patch passed |
| +1 | mvneclipse | 0m 11s | the patch passed |
| -1 | whitespace | 0m 0s | The patch 1 line(s) with tabs. |
| +1 | findbugs | 2m 0s | the patch passed |
| +1 | javadoc | 0m 38s | the patch passed |
| -1 | unit | 74m 45s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
| 102m 29s |
| Reason | Tests |
|---|---|
| Timed out junit tests | org.apache.hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting |
| Subsystem | Report/Notes |
|---|---|
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | |
| JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12846136/HDFS-11273.003.patch |
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
| uname | Linux 1e91904b6f0e 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 71a4acf |
| Default Java | 1.8.0_111 |
| findbugs | v3.0.0 |
| checkstyle | https://builds.apache.org/job/PreCommit-HDFS-Build/18099/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt |
| whitespace | https://builds.apache.org/job/PreCommit-HDFS-Build/18099/artifact/patchprocess/whitespace-tabs.txt |
| unit | https://builds.apache.org/job/PreCommit-HDFS-Build/18099/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/18099/testReport/ |
| modules | C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs |
| Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/18099/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 18s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
| +1 | mvninstall | 14m 35s | trunk passed |
| +1 | compile | 0m 59s | trunk passed |
| +1 | checkstyle | 0m 31s | trunk passed |
| +1 | mvnsite | 1m 3s | trunk passed |
| +1 | mvneclipse | 0m 15s | trunk passed |
| +1 | findbugs | 2m 7s | trunk passed |
| +1 | javadoc | 0m 41s | trunk passed |
| +1 | mvninstall | 0m 55s | the patch passed |
| +1 | compile | 0m 50s | the patch passed |
| +1 | javac | 0m 50s | the patch passed |
| -0 | checkstyle | 0m 28s | hadoop-hdfs-project/hadoop-hdfs: The patch generated 7 new + 124 unchanged - 9 fixed = 131 total (was 133) |
| +1 | mvnsite | 0m 58s | the patch passed |
| +1 | mvneclipse | 0m 12s | the patch passed |
| +1 | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 | findbugs | 2m 11s | the patch passed |
| +1 | javadoc | 0m 42s | the patch passed |
| -1 | unit | 83m 46s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 23s | The patch does not generate ASF License warnings. |
| 112m 18s |
| Reason | Tests |
|---|---|
| Failed junit tests | hadoop.hdfs.TestDFSStripedOutputStreamWithFailure160 |
| Timed out junit tests | org.apache.hadoop.hdfs.server.blockmanagement.TestBlockStatsMXBean |
| Subsystem | Report/Notes |
|---|---|
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | |
| JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12846411/HDFS-11273.004.patch |
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
| uname | Linux 545690ddc75b 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 91bf504 |
| Default Java | 1.8.0_111 |
| findbugs | v3.0.0 |
| checkstyle | https://builds.apache.org/job/PreCommit-HDFS-Build/18114/artifact/patchprocess/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt |
| unit | https://builds.apache.org/job/PreCommit-HDFS-Build/18114/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/18114/testReport/ |
| modules | C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs |
| Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/18114/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
The latest patch looks good to me. The failed tests should be unrelated and they passed in my local machine. +1
I've committed the patch to trunk. Thanks for the contribution, Harsha Koneru!
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11095 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11095/)
HDFS-11273. Move TransferFsImage#doGetUrl function to a Util class. (jing9: rev 7ec609b28989303fe0cc36812f225028b0251b32)
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Util.java
- (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HttpPutFailedException.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ImageServlet.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java
- (add) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HttpGetFailedException.java
Thank you Jing Zhao for the reviews and for committing the patch.
HDFS-11273This message was automatically generated.