steveloughran commented on code in PR #6006:
URL: https://github.com/apache/hadoop/pull/6006#discussion_r1323354035
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/commit/MagicCommitPaths.java:
##########
@@ -96,7 +101,18 @@ public static boolean containsBasePath(List<String>
elements) {
* @throws IllegalArgumentException if there is no magic element
*/
public static int magicElementIndex(List<String> elements) {
- int index = elements.indexOf(MAGIC);
+ int index = 0;
Review Comment:
actually, you could coalesce this with a L114-115 some method to get the
index without raising an exception; isMagicPath(elements) calls this and
returns true if the index > 0. or you do very fancy java8 Optional stuff...
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/commit/MagicCommitPaths.java:
##########
@@ -208,8 +215,8 @@ public static List<String> finalDestination(List<String>
elements) {
if (isMagicPath(elements)) {
List<String> destDir = magicPathParents(elements);
List<String> children = magicPathChildren(elements);
- checkArgument(!children.isEmpty(), "No path found under " +
- MAGIC);
+ checkArgument(!children.isEmpty(), "No path found under the prefix" +
Review Comment:
needs a space at the end
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/commit/integration/ITestS3ACommitterMRJob.java:
##########
@@ -586,7 +586,7 @@ private MagicCommitterTestBinding() {
protected void validateResult(final Path destPath,
final SuccessData successData)
throws Exception {
- Path magicDir = new Path(destPath, MAGIC);
+ Path magicDir = new Path(destPath, MAGIC_PATH_PREFIX);
Review Comment:
is a job id getting down? it should be, for completeness
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/commit/MagicCommitPaths.java:
##########
@@ -76,7 +76,12 @@ public static List<String> splitPathToElements(Path path) {
* @return true if a path is considered magic
*/
public static boolean isMagicPath(List<String> elements) {
- return elements.contains(MAGIC);
+ for (String element : elements) {
Review Comment:
or use some java8 stream thing?
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/commit/ITestCommitOperationCost.java:
##########
@@ -151,10 +151,10 @@ public void testMagicMkdir() throws Throwable {
*/
@Test
public void testMagicMkdirs() throws Throwable {
- describe("Mkdirs __magic/subdir always skips dir marker deletion");
+ describe("Mkdirs __magic_job-/subdir always skips dir marker deletion");
S3AFileSystem fs = getFileSystem();
Path baseDir = methodPath();
- Path magicDir = new Path(baseDir, MAGIC);
+ Path magicDir = new Path(baseDir, MAGIC_PATH_PREFIX);
Review Comment:
for this test use the base prefix so we can see that it is still recognised
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/commit/magic/MagicS3GuardCommitter.java:
##########
@@ -131,16 +134,19 @@ protected ActiveCommit listPendingUploadsToCommit(
* Delete the magic directory.
*/
public void cleanupStagingDirs() {
- final Path out = getOutputPath();
- Path path = magicSubdir(out);
- try(DurationInfo ignored = new DurationInfo(LOG, true,
- "Deleting magic directory %s", path)) {
- Invoker.ignoreIOExceptions(LOG, "cleanup magic directory",
path.toString(),
- () -> deleteWithWarning(getDestFS(), path, true));
- // and the job temp directory with manifests
- Invoker.ignoreIOExceptions(LOG, "cleanup job directory", path.toString(),
- () -> deleteWithWarning(getDestFS(),
- new Path(out, TEMP_DATA), true));
+ if (cleanupMagicDirectory) {
+ final Path out = getOutputPath();
+ Path path = getMagicJobPath(getUUID(), out);
+ try (DurationInfo ignored = new DurationInfo(LOG, true,
+ "Deleting magic directory %s", path)) {
+ Invoker.ignoreIOExceptions(LOG, "cleanup magic directory",
path.toString(),
+ () -> deleteWithWarning(getDestFS(), path, true));
+ // and the job temp directory with manifests
+ Invoker.ignoreIOExceptions(LOG, "cleanup job directory",
path.toString(),
Review Comment:
job dir cleanup MUST always happen; make the root path cleanup the optional
one
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
