steveloughran commented on a change in pull request #3071:
URL: https://github.com/apache/hadoop/pull/3071#discussion_r645020297
##########
File path:
hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpConstants.java
##########
@@ -191,4 +191,8 @@ private DistCpConstants() {
public static final String CLASS_INSTANTIATION_ERROR_MSG =
"Unable to instantiate ";
+
+ public static final String TARGET_TEMP_FILE_PREFIX_COMMA = ".distcp.tmp.";
+
+ public static final String TARGET_TEMP_FILE_PREFIX = "distcp.tmp.";
Review comment:
add _FTP and javadocs to explain why it is needed
##########
File path:
hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/DistCpConstants.java
##########
@@ -191,4 +191,8 @@ private DistCpConstants() {
public static final String CLASS_INSTANTIATION_ERROR_MSG =
"Unable to instantiate ";
+
+ public static final String TARGET_TEMP_FILE_PREFIX_COMMA = ".distcp.tmp.";
Review comment:
1. use DOT instead of COMMA here.
2. add javadoc to explain what it does
##########
File path:
hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/util/DistCpUtils.java
##########
@@ -662,4 +662,20 @@ public static Path getSplitChunkPath(Path targetFile,
+ ".____distcpSplit____" + srcFileStatus.getChunkOffset()
+ "." + srcFileStatus.getChunkLength());
}
+
+ /**
+ * Return the target temp file prefix
Review comment:
add . on javadoc sentence to keep javadoc happy
##########
File path:
hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/mapred/RetriableFileCopyCommand.java
##########
@@ -257,17 +257,18 @@ private void promoteTmpToTarget(Path tmpTarget, Path
target, FileSystem fs)
}
}
- private Path getTempFile(Path target, Mapper.Context context) {
+ private Path getTempFile(Path target, Mapper.Context context, FileSystem
fileSystem) {
Path targetWorkPath = new Path(context.getConfiguration().
get(DistCpConstants.CONF_LABEL_TARGET_WORK_PATH));
Path root = target.equals(targetWorkPath) ? targetWorkPath.getParent()
: targetWorkPath;
- Path tempFile = new Path(root, ".distcp.tmp." +
+ String tempFilePrefix = DistCpUtils.getTargetTempFilePrefix(fileSystem);
Review comment:
assuming the Path is resolved, the FS Scheme is that of
Path.toURI.getScheme(); no need to pass the FS around.
##########
File path:
hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/mapred/CopyCommitter.java
##########
@@ -170,8 +170,9 @@ private void deleteAttemptTempFiles(Path targetWorkPath,
return;
}
+ String tempFilePrefix = DistCpUtils.getTargetTempFilePrefix(targetFS);
FileStatus[] tempFiles = targetFS.globStatus(
- new Path(targetWorkPath, ".distcp.tmp." +
jobId.replaceAll("job","attempt") + "*"));
+ new Path(targetWorkPath, tempFilePrefix +
jobId.replaceAll("job","attempt") + "*"));
Review comment:
probably need to cut the line down to keep it under 80 chars width
##########
File path:
hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/mapred/RetriableFileCopyCommand.java
##########
@@ -129,16 +129,15 @@ private long doCopy(CopyListingFileStatus source, Path
target,
throws IOException {
LOG.info("Copying {} to {}", source.getPath(), target);
+ final Configuration configuration = context.getConfiguration();
Review comment:
If you just get the schema in getTempFile() from the path, no need to
pass in FileSystem, so no need to move this up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
