[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15904725#comment-15904725
]
Hadoop QA commented on HADOOP-13914:
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
18s{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 12 new or modified test
files. {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 1m
55s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 12m
32s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 19m
43s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
56s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
28s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
39s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
2s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
12s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
16s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 15m
59s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 15m
59s{color} | {color:green} the patch passed {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
2m 5s{color} | {color:orange} root: The patch generated 1 new + 77 unchanged -
2 fixed = 78 total (was 79) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
34s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} xml {color} | {color:green} 0m
1s{color} | {color:green} The patch has no ill-formed XML file. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
49s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
18s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 9m 3s{color}
| {color:red} hadoop-common in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 0m
50s{color} | {color:green} hadoop-aws in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
37s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black}102m 36s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests | hadoop.security.TestKDiag |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | HADOOP-13914 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12857182/HADOOP-13914-HADOOP-13345.008.patch
|
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite
unit findbugs checkstyle xml |
| uname | Linux e37ceadc4c27 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6
15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh
|
| git revision | HADOOP-13345 / b968fb3 |
| Default Java | 1.8.0_121 |
| findbugs | v3.0.0 |
| checkstyle |
https://builds.apache.org/job/PreCommit-HADOOP-Build/11798/artifact/patchprocess/diff-checkstyle-root.txt
|
| unit |
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15904094#comment-15904094 ] Mingliang Liu commented on HADOOP-13914: Mirror Sean's comments, and +1 (again) > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch, > HADOOP-13914-HADOOP-13345.006.patch, HADOOP-13914-HADOOP-13345.007.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15904055#comment-15904055 ] Aaron Fabbri commented on HADOOP-13914: --- Thanks for the review [~mackrorysd]. The innerRename() function length warning is existing. Agree it should be tackled later (ideally after merge to trunk). Thank you for the findbugs-exclude snippet. I thought about removing the null check but still think this is more future-proof. I'll add your exclusion and post a new patch. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch, > HADOOP-13914-HADOOP-13345.006.patch, HADOOP-13914-HADOOP-13345.007.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15903685#comment-15903685
]
Sean Mackrory commented on HADOOP-13914:
I reviewed the .006. patch, and I'm a +1 on the code itself.
hadoop-common unit test results we've seen before and aren't related. There's
still that checkstyle issue with innerRename - I'm of the opinion that we
should try and refactor that to fit under 150, but after this is merged to
trunk. That's probably one of the tricker things we'll have to merge and
refactoring it that way will only make that worse. Anyone disagree?
We can add the following to ignore the findbugs issue:
{quote}
diff --git a/hadoop-tools/hadoop-aws/dev-support/findbugs-exclude.xml
b/hadoop-tools/hadoop-aws/dev-support/findbugs
-exclude.xml
index ffb0a79..3464e71 100644
--- a/hadoop-tools/hadoop-aws/dev-support/findbugs-exclude.xml
+++ b/hadoop-tools/hadoop-aws/dev-support/findbugs-exclude.xml
@@ -26,4 +26,10 @@
+
+
+
+
+
+
{quote}
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch,
> HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch,
> HADOOP-13914-HADOOP-13345.006.patch, HADOOP-13914-HADOOP-13345.007.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15902254#comment-15902254
]
Hadoop QA commented on HADOOP-13914:
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
16s{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 12 new or modified test
files. {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 1m
57s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 13m
57s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 20m
52s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
59s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
54s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
40s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
9s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
13s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
45s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 14m
59s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 14m
59s{color} | {color:green} the patch passed {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
1m 57s{color} | {color:orange} root: The patch generated 1 new + 77 unchanged -
2 fixed = 78 total (was 79) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
34s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 0m
48s{color} | {color:red} hadoop-tools/hadoop-aws generated 1 new + 0 unchanged
- 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
18s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 8m 15s{color}
| {color:red} hadoop-common in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 0m
48s{color} | {color:green} hadoop-aws in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
39s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black}104m 3s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hadoop-tools/hadoop-aws |
| | Redundant nullcheck of
org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus(Path, String), which is
known to be non-null in org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path)
Redundant null check at S3AFileSystem.java:which is known to be non-null in
org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path) Redundant null check at
S3AFileSystem.java:[line 1851] |
| Failed junit tests | hadoop.security.TestKDiag |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | HADOOP-13914 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12856884/HADOOP-13914-HADOOP-13345.007.patch
|
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite
unit findbugs checkstyle |
| uname | Linux 8fd13f756e99 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6
15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15900623#comment-15900623
]
Hadoop QA commented on HADOOP-13914:
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
15s{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 12 new or modified test
files. {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 2m
1s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 12m
50s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 20m
17s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
54s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
30s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
39s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
3s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
12s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
16s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 14m
21s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 14m
21s{color} | {color:green} the patch passed {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
1m 56s{color} | {color:orange} root: The patch generated 2 new + 77 unchanged -
2 fixed = 79 total (was 79) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
34s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 0m
49s{color} | {color:red} hadoop-tools/hadoop-aws generated 1 new + 0 unchanged
- 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
17s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 8m 15s{color}
| {color:red} hadoop-common in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 0m
50s{color} | {color:green} hadoop-aws in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
38s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black}100m 47s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hadoop-tools/hadoop-aws |
| | Redundant nullcheck of
org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus(Path, String), which is
known to be non-null in org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path)
Redundant null check at S3AFileSystem.java:which is known to be non-null in
org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path) Redundant null check at
S3AFileSystem.java:[line 1851] |
| Failed junit tests | hadoop.security.TestKDiag |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | HADOOP-13914 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12856714/HADOOP-13914-HADOOP-13345.006.patch
|
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite
unit findbugs checkstyle |
| uname | Linux 621b39977524 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6
15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15900130#comment-15900130 ] Aaron Fabbri commented on HADOOP-13914: --- Yes [~rajesh.balamohan], I just noticed that too. Good catch. Appreciate the thorough review on this patch. Will post followup patch when i finish testing. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15898949#comment-15898949
]
Rajesh Balamohan commented on HADOOP-13914:
---
{noformat}
S3AFileStatus innerGetFileStatus(final Path f, boolean needEmptyDirectoryFlag)
throws IOException {
..
// Check MetadataStore, if any.
..
PathMetadata pm = metadataStore.get(path, true);
..
{noformat}
Should {{needEmptyDirectoryFlag}} be passed on to {{MetadataStore}} ? This
would avoid additional {QuerySpec}. This is similar [~liuml07]'s comment #1.
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch,
> HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15898693#comment-15898693
]
Aaron Fabbri commented on HADOOP-13914:
---
Thanks for the review [~liuml07]. Great comments..
{quote}
In DynamoDBMetadataStore#get(), we should check wantEmptyDirectoryFlag
{quote}
Good catch. Next patch will skip that extra work if the caller doesn't care.
I meant to do that but forgot.
{quote}
The isAuthoritative is false and we have children, we confidently trust the
child's existence though it's not authoritative (fully cached). Seems
reasonable.
{quote}
Yep.. And when we add support to DDB for tracking isAuthoritative, we may also
be able to answer true if {{authoritative && empty}}.
{quote}To simplify the if clause as if (!allowMissing()){quote}
That logic is intentional: {{allowMissing()}} means a MetadataStore may
discard contents. E.g. LocalMetadataStore discards entries based on LRU or
(eventually) TTL timeout. The case also covers NullMetadataStore which,
legally, discards everything.
Even if the MetadataStore is allowed to discard results, if it does return
results, we *still* run validation for the test to make sure the result is
correct. So, to summarize, allowMissing() means "test shouldn't fail if an
entry is missing" but otherwise, all validation should occur.
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch,
> HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15895426#comment-15895426
]
Mingliang Liu commented on HADOOP-13914:
Thanks [~fabbri], this is really good improvement. I like the design & patch.
Glad we stay with the "improved approach" from v0 wip patch (aka _Treat
isEmptyDirectory as ephemeral_ in the initial doc).
Some comments:
# In {{DynamoDBMetadataStore#get()}}, we should check
{{wantEmptyDirectoryFlag}} value. One candidate place is L337 {{if (meta !=
null) {}}
# {code:title=DirListingMetadata.java}
115 /**
116* Is the underlying directory known to be empty?
117* @return FALSE if directory is known to have a child entry, TRUE if
118* directory is known to be empty, UNKNOWN otherwise.
119*/
120 public Tristate isEmpty() {
121 if (getListing().isEmpty()) {
122 if (isAuthoritative()) {
123 return Tristate.TRUE;
124 } else {
125 // This listing is empty, but may not be full list of
underlying dir.
126 return Tristate.UNKNOWN;
127 }
128 } else { // not empty listing
129 // There exists at least one child, dir not empty.
130 return Tristate.FALSE;
131 }
132 }
{code}
The {{isAuthoritative}} is false and we have children, we confidently trust the
child's existence though it's not authoritative (fully cached). Seems
reasonable.
# {code:title=DynamoDBMetadataStore}
325 meta = new PathMetadata(new FileStatus(0, true, 1, 0, 0, 0,
null,
326 username, null, path));
{code}
a util makeFileStatus()...? I have a gut feeling - we'll reuse this.
# {code:title= MetadataStoreTestBase#testGetEmptyDir()}
368 // 3. Check that either (a) the MS doesn't track whether or not it
is
369 // empty (which is allowed), or (b) the MS knows the dir is empty.
370 if (!allowMissing() || meta != null) {
371 assertNotNull("Get found dir", meta);
372 assertNotEquals("Dir is empty or unknown", Tristate.FALSE,
373 meta.isEmptyDirectory());
374 }
{code}
To simplify the if clause as {{if (!allowMissing()) {}}. I'd prefer error
message be something clearer: "Get should find meta for path" / "Dir should be
empty or unknown" Anyway it's nit. Other test methods as well.
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch,
> HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15895268#comment-15895268 ] Aaron Fabbri commented on HADOOP-13914: --- Checkstyle is clean now. Compile failures are still some Yarn UI javascript stuff (unrelated).. and unit failures are TestKDiag (unrelated) > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > HADOOP-13914-HADOOP-13345.004.patch, HADOOP-13914-HADOOP-13345.005.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15895221#comment-15895221
]
Hadoop QA commented on HADOOP-13914:
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
15s{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 12 new or modified test
files. {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 1m
52s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 13m
7s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:red}-1{color} | {color:red} compile {color} | {color:red} 10m
37s{color} | {color:red} root in HADOOP-13345 failed. {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
59s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
41s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
36s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
25s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
15s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
17s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
15s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} compile {color} | {color:red} 8m
36s{color} | {color:red} root in the patch failed. {color} |
| {color:red}-1{color} | {color:red} javac {color} | {color:red} 8m 36s{color}
| {color:red} root in the patch failed. {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
1m 56s{color} | {color:orange} root: The patch generated 2 new + 77 unchanged -
2 fixed = 79 total (was 79) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
34s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
45s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 0m
53s{color} | {color:red} hadoop-tools/hadoop-aws generated 1 new + 0 unchanged
- 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
15s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 9m 22s{color}
| {color:red} hadoop-common in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 0m
53s{color} | {color:green} hadoop-aws in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
33s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 87m 0s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hadoop-tools/hadoop-aws |
| | Redundant nullcheck of
org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus(Path, String), which is
known to be non-null in org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path)
Redundant null check at S3AFileSystem.java:which is known to be non-null in
org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path) Redundant null check at
S3AFileSystem.java:[line 1851] |
| Failed junit tests | hadoop.security.TestKDiag |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | HADOOP-13914 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12855977/HADOOP-13914-HADOOP-13345.005.patch
|
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite
unit findbugs checkstyle |
| uname | Linux c6b895192175 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6
15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15894806#comment-15894806 ] Aaron Fabbri commented on HADOOP-13914: --- Sigh.. I did run test-patch on this before submitting. The compile error is a Yarn UI Javascript issue (unrelated). The unit failure is TestKDiag (unrelated). The findbugs warning is redundant null check we'd like to leave in. It does look like there are some legitimate checkstyle issues.. not sure why test-patch reported it clean. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > HADOOP-13914-HADOOP-13345.004.patch, s3guard-empty-dirs.md, > test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15894227#comment-15894227 ] Steve Loughran commented on HADOOP-13914: - bq. I wondered if a `public S3AFileStatus getFileStatus(Path, bool)` function might be in order in S3AFileSystem? No idea how much it'll get used, but if anyone IS depending on the current S3AFileStatus return type and wants that work done it'd be useful. No, let's leave it out unless/until someone complains, *and can justify why it should go in". > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > HADOOP-13914-HADOOP-13345.004.patch, s3guard-empty-dirs.md, > test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15893862#comment-15893862
]
Hadoop QA commented on HADOOP-13914:
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
18s{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 12 new or modified test
files. {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
17s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 13m
38s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:red}-1{color} | {color:red} compile {color} | {color:red} 9m
36s{color} | {color:red} root in HADOOP-13345 failed. {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 2m
4s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
40s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
39s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
15s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
15s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
18s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
15s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} compile {color} | {color:red} 9m
7s{color} | {color:red} root in the patch failed. {color} |
| {color:red}-1{color} | {color:red} javac {color} | {color:red} 9m 7s{color}
| {color:red} root in the patch failed. {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
2m 4s{color} | {color:orange} root: The patch generated 11 new + 77 unchanged
- 2 fixed = 88 total (was 79) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
36s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
43s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 0m
50s{color} | {color:red} hadoop-tools/hadoop-aws generated 1 new + 0 unchanged
- 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
31s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 9m 27s{color}
| {color:red} hadoop-common in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 0m
49s{color} | {color:green} hadoop-aws in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
37s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 86m 9s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hadoop-tools/hadoop-aws |
| | Redundant nullcheck of
org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus(Path, String), which is
known to be non-null in org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path)
Redundant null check at S3AFileSystem.java:which is known to be non-null in
org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path) Redundant null check at
S3AFileSystem.java:[line 1851] |
| Failed junit tests | hadoop.security.TestKDiag |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | HADOOP-13914 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12855779/HADOOP-13914-HADOOP-13345.004.patch
|
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite
unit findbugs checkstyle |
| uname | Linux 2d73d524a8bd 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6
15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15893755#comment-15893755 ] Aaron Fabbri commented on HADOOP-13914: --- Yep, those two failures are HADOOP-14129 and HADOOP-14036. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15893676#comment-15893676
]
Sean Mackrory commented on HADOOP-13914:
That all sounds reasonable. By the way I had a few test failures. A couple of
tests fail in TestDynamoDBMetadataStore because I have configured a table named
differently from my bucket (which I've been hoping to address in HADOOP-14068),
ITestS3ACredentialsInURL failed (also being addressed by a separate JIRA), and
I got the following failure which I believe we've seen before:
{code}
testRenameToDirWithSamePrefixAllowed(org.apache.hadoop.fs.s3a.ITestS3AFileSystemContract)
Time elapsed: 4.834 sec <<< ERROR!
org.apache.hadoop.fs.s3a.AWSServiceIOException: move:
com.amazonaws.services.dynamodbv2.model.AmazonDynamoDBException: Provided list
of item keys contains duplicates (Service: AmazonDynamoDBv2; Status Code: 400;
Error Code: ValidationException; Request ID:
KLU3MEVJVD2RAE269JD6SCLP9VVV4KQNSO5AEMVJF66Q9ASUAAJG): Provided list of item
keys contains duplicates (Service: AmazonDynamoDBv2; Status Code: 400; Error
Code: ValidationException; Request ID:
KLU3MEVJVD2RAE269JD6SCLP9VVV4KQNSO5AEMVJF66Q9ASUAAJG)
{code}
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15893332#comment-15893332
]
Aaron Fabbri commented on HADOOP-13914:
---
Thanks for the review [~mackrorysd].
{quote}
I'm a bit concerned about all the S3AFileStatus -> FileStatus changes (although
I could've sworn that we already removed that assertion in
PathMetadataTranslation...). I think it's definitely a change for the better,
but I'm a little worried there is some application out there that this change
will break, despite being @Private and @Evolving...
{quote}
Yes, this is a fundamental challenge for this patch. I explicitly want to
remove isEmptyDirectory from the public API. It should only be used internally
because we don't want the cost of always computing it, when it is usually never
used.
{quote}
Along similar lines, I wondered if a `public S3AFileStatus getFileStatus(Path,
bool)` function might be in order in S3AFileSystem? No idea how much it'll get
used, but if anyone IS depending on the current S3AFileStatus return type and
wants that work done it'd be useful.
{quote}
My understanding is that having isEmptyDirectory() on the S3AFileStatus is an
internal optimization to save a round trip for delete and rename. Without a
compelling need for this shortcut in the public API, I would suggest people
just use {{listStatus(dir)}} to determine emptiness.
{quote}
There's an assertion in TestDynamoDBMetadataStore.java that isEmptyDirectory()
is what we expect. Why is that removed? Is it a Boolean -> Tristate issue? If
so, shouldn't we modify the logic to accept either the expected one or UNKNOWN?
{quote}
After this patch, MetadataStore implementations are not required to return
S3AFileStatus with isEmptyDirectory set properly (which was broken anyways for
DDB). So that assertion no longer makes sense. I should probably:
- Replace that assert with one that checks PathMetadata#isEmptyDirectory() does
not conflict with expected value (i.e. UNKNOWN is always correct, but false
versus true would be a failure).
- We should probably have some new MetadataStore contract test cases around
{{PathMetadata#isEmptyDirectory}}. I can add one in the next patch.
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15893273#comment-15893273 ] Sean Mackrory commented on HADOOP-13914: Already discussed this a bit with you while reviewing, but sharing all thoughts for everyone else's sake: * In case other reviewers get confused by it, the hints from Git in each hunk about which function a change is in are misleading S3AFileSystem, and in the places where putAndReturn is removed it is only removed in the S3-only version of the function that does no metadatastore operation. putAndReturn now happens higher in the callstack only when there is a metadatastore context. * I'm a bit concerned about all the S3AFileStatus -> FileStatus changes (although I could've sworn that we already removed that assertion in PathMetadataTranslation...). I think it's definitely a change for the better, but I'm a little worried there is some application out there that this change will break, despite being @Private and @Evolving... Let's definitely at least call this out in a release note or something. * Along similar lines, I wondered if a `public S3AFileStatus getFileStatus(Path, bool)` function might be in order in S3AFileSystem? No idea how much it'll get used, but if anyone IS depending on the current S3AFileStatus return type and wants that work done it'd be useful. * I also thought about a S3AFileStatus.setIsEmptyDirectory(bool) wrapper to avoid any unnecessary change in compatibility, but again, very unlikely to be used publicly, so feel free to ignore. isEmptyDirectory is probably going break the same set of applications that might use it anyway (which for all I know is an empty set), so no perfectly compatible way to do this anyway... * JavaDoc comment on ITestSGuardEmptyDirs recommends a refactoring after HADOOP-13345 is merged - we should file a JIRA dependent on HADOOP-13345 for that when this is committed. * There's an assertion in TestDynamoDBMetadataStore.java that isEmptyDirectory() is what we expect. Why is that removed? Is it a Boolean -> Tristate issue? If so, shouldn't we modify the logic to accept either the expected one or UNKNOWN? * +1 to ignoring the findbugs issue that way. Tests are running now - almost done and no related failures, will report back if the end result is otherwise. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15888668#comment-15888668
]
Aaron Fabbri commented on HADOOP-13914:
---
The findbugs warning is here:
{code}
try {
return s3GetFileStatus(path, key) != null;
} catch (FileNotFoundException e) {
return false;
}
{code}
s3GetFileStatus() never returns null, thus the warning of redundant null check.
It feels like the null check is a small price to pay to be future-proof /
paranoid, but I'm happy to remove it and rely only on exception behavior.
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, HADOOP-13914-HADOOP-13345.003.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15887482#comment-15887482
]
Hadoop QA commented on HADOOP-13914:
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
17s{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} test4tests {color} | {color:green} 0m
0s{color} | {color:green} The patch appears to include 11 new or modified test
files. {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
14s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 13m
40s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 14m
0s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
58s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
35s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
39s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
24s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
14s{color} | {color:green} HADOOP-13345 passed {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
16s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
1s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 13m
2s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 13m
2s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
56s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
35s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
46s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 0m
55s{color} | {color:red} hadoop-tools/hadoop-aws generated 1 new + 0 unchanged
- 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
25s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 8m 30s{color}
| {color:red} hadoop-common in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 0m
50s{color} | {color:green} hadoop-aws in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
38s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 93m 14s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hadoop-tools/hadoop-aws |
| | Redundant nullcheck of
org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus(Path, String), which is
known to be non-null in org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path)
Redundant null check at S3AFileSystem.java:which is known to be non-null in
org.apache.hadoop.fs.s3a.S3AFileSystem.s3Exists(Path) Redundant null check at
S3AFileSystem.java:[line 1851] |
| Failed junit tests | hadoop.security.TestKDiag |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:a9ad5d6 |
| JIRA Issue | HADOOP-13914 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12855065/HADOOP-13914-HADOOP-13345.003.patch
|
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite
unit findbugs checkstyle |
| uname | Linux 31a182871d73 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6
15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh
|
| git revision | HADOOP-13345 / 0abbb70 |
|
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15887312#comment-15887312 ] Mingliang Liu commented on HADOOP-13914: Thanks [~fabbri]! I'll review this week. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Aaron Fabbri > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > HADOOP-13914-HADOOP-13345.002.patch, s3guard-empty-dirs.md, > test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15887120#comment-15887120
]
Aaron Fabbri commented on HADOOP-13914:
---
Currently running tests on a second version of this patch. Will post it soon
assuming the tests all pass.
There are two bugs I want to fix ASAP here:
1. DDB metadata store reports a directory is empty when it might not be.
2. {{S3AFileSystem#createFakeDirectoryIfNecessary()}} is broken w/ S3Guard: It
needs a "raw" version of {{FileSystem#exists()}} to see if the blob is needed:
When S3Guard is enabled, {{exists()}} may return true even though there is no
empty dir blob in S3.
Both of these bugs are exercised by the new {{ITestS3GuardEmptyDirs}} test
included here and my next patch addresses both of them.
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Aaron Fabbri
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> HADOOP-13914-HADOOP-13345.002.patch, s3guard-empty-dirs.md,
> test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15872640#comment-15872640 ] Mingliang Liu commented on HADOOP-13914: Thanks for helping me out (and the doc/test patch). I didn't upload the patch before vacation because the test was still failing. I did some analysis and will post comment here soon. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Mingliang Liu > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15872633#comment-15872633 ] Aaron Fabbri commented on HADOOP-13914: --- Ok.. I don't want to step on your toes [~liuml07]. I was not sure if you were working on it or not.I am here to help. I'd like to get this JIRA resolved soon. There are some interesting design decisions here so I look forward to discussing more. Feel free to post in-progress / RFC patches if you want. Thanks and welcome back! > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Mingliang Liu > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15872582#comment-15872582 ] Mingliang Liu commented on HADOOP-13914: [~fabbri] I came back to work this week. I'll wrap up the current patch boolean->tristate and upload that one before you take it over if necessary. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Mingliang Liu > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.15#6346) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15813283#comment-15813283 ] Aaron Fabbri commented on HADOOP-13914: --- Ok, thanks for clarifying. Enjoy your break! > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Mingliang Liu > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15813276#comment-15813276 ] Mingliang Liu commented on HADOOP-13914: Thanks. I saw the doc, it's very helpful. Yeah, tristate seems more meaningful. The v0 patch was from offline discussion with Steve last week. I'll rebase later. p.s. [~fabbri] and [~eddyxu] I'll be on PTO for ~1mo. I'll not be responsive during that period, but I'll catch up emails/commits silently. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Mingliang Liu > Attachments: HADOOP-13914-HADOOP-13345.000.patch, > s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15813249#comment-15813249
]
Aaron Fabbri commented on HADOOP-13914:
---
Thanks for uploading an RFC patch [~liuml07].
Did you have a chance to read [the
doc|https://gist.github.com/ajfabbri/db3b2ad5b1f111277681ba4b7f788404] I
attached here? If I'm reading this patch right, the test case I attached to
this JIRA would not pass.
In particular, my doc asserts that MetadataStore#isEmptyDirectory would need to
return a tristate, not a boolean, as it can answer {yes, no, not enough
information}.
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Mingliang Liu
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[
https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15813216#comment-15813216
]
Aaron Fabbri commented on HADOOP-13914:
---
Thanks [~ste...@apache.org]. I'm not sure I'm parsing your comment right so
let me try to paraphrase, and you can correct me:
{quote}
We only want that dir as an optimisation of followon work in s3aFS, so that if
you get a delete(path) you can do a getFileStatus, and, if status=directory,
see if it is empty (so skip the need for recursive=true) without another round
trip.
{quote}
Do you mean that the only purpose of the isEmptyDirectory() predicate in the
future will be for saving a round trip on directory deletes?
{quote}
with s3guard you don't need that caching of state. It can be be done on demand,
only in those few cases where we actually need to know about it...which pushes
for it being something that the metadatastore can work out on demand. We would
need to document that the status field is only valid without an MD store
{quote}
Makes sense. And this "status field only valid w/o MD store" could be
encapsulated in a helper function.
Does the basic solution I proposed still make sense for the case where
determining that flag is still needed?
> s3guard: improve S3AFileStatus#isEmptyDirectory handling
>
>
> Key: HADOOP-13914
> URL: https://issues.apache.org/jira/browse/HADOOP-13914
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/s3
>Affects Versions: HADOOP-13345
>Reporter: Aaron Fabbri
>Assignee: Mingliang Liu
> Attachments: HADOOP-13914-HADOOP-13345.000.patch,
> s3guard-empty-dirs.md, test-only-HADOOP-13914.patch
>
>
> As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag
> stored in S3AFileStatus is missing from DynamoDBMetadataStore.
> The approach taken by LocalMetadataStore is not suitable for the DynamoDB
> implementation, and also sacrifices good code separation to minimize
> S3AFileSystem changes pre-merge to trunk.
> I will attach a design doc that attempts to clearly explain the problem and
> preferred solution. I suggest we do this work after merging the HADOOP-13345
> branch to trunk, but am open to suggestions.
> I can also attach a patch of a integration test that exercises the missing
> case and demonstrates a failure with DynamoDBMetadataStore.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15805814#comment-15805814 ] Steve Loughran commented on HADOOP-13914: - Good writeup. I had a talk with mingliang & rajesh about this. We only want that dir as an optimisation of followon work in s3aFS, so that if you get a delete(path) you can do a getFileStatus, and, if status=directory, see if it is empty (so skip the need for recursive=true) without another round trip. with s3guard you don't need that caching of state. It can be be done on demand, only in those few cases where we actually need to know about it...which pushes for it being something that the metadatastore can work out on demand. We would need to document that the status field is only valid without an MD store > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri >Assignee: Mingliang Liu > Attachments: s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15762119#comment-15762119 ] Mingliang Liu commented on HADOOP-13914: Post-merge-to-trunk works for me. Steve is also on vocation. You guys enjoy! > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri > Attachments: s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-13914) s3guard: improve S3AFileStatus#isEmptyDirectory handling
[ https://issues.apache.org/jira/browse/HADOOP-13914?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15756111#comment-15756111 ] Aaron Fabbri commented on HADOOP-13914: --- [~liuml07] [~steve_l] for your reading pleasure. I will be on vacation for two weeks. Let me know what you think about doing this work post-merge-to-trunk. > s3guard: improve S3AFileStatus#isEmptyDirectory handling > > > Key: HADOOP-13914 > URL: https://issues.apache.org/jira/browse/HADOOP-13914 > Project: Hadoop Common > Issue Type: Sub-task > Components: fs/s3 >Affects Versions: HADOOP-13345 >Reporter: Aaron Fabbri > Attachments: s3guard-empty-dirs.md, test-only-HADOOP-13914.patch > > > As discussed in HADOOP-13449, proper support for the isEmptyDirectory() flag > stored in S3AFileStatus is missing from DynamoDBMetadataStore. > The approach taken by LocalMetadataStore is not suitable for the DynamoDB > implementation, and also sacrifices good code separation to minimize > S3AFileSystem changes pre-merge to trunk. > I will attach a design doc that attempts to clearly explain the problem and > preferred solution. I suggest we do this work after merging the HADOOP-13345 > branch to trunk, but am open to suggestions. > I can also attach a patch of a integration test that exercises the missing > case and demonstrates a failure with DynamoDBMetadataStore. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
