You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org> on 2018/04/25 21:52:08 UTC
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10210
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
IMPALA-6885: handle empty s3 dirs in recover_partitions test
Two tests (test_duplicate_partitions, test_support_all_types)
added only directories, which is a no-op on s3. This results
in s3 test failures.
Another test (test_recover_partitions) falsely passed for s3
since its test for a malformed partition name is ambiguous
(did it pass because recovering correctly excluded or because
the directory does not exist, as in the case of s3?).
A recent change fixed the asserts in this test, so these tests
likely were falsely reporting success.
This change bundles directory creation with adding to it a new
file, which is the common case in this test. As a result, the
peculiarity with s3 is not handled via a special case each time
the filesystem is modified for new partitions.
This change also adds an explicit test for empty partition
directories, since the first change leaves this case untested.
Testing:
- tested against a local minicluster and s3
Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
---
M tests/common/skip.py
M tests/metadata/test_recover_partitions.py
2 files changed, 57 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10210/1
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10210 )
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10210/1/tests/metadata/test_recover_partitions.py
File tests/metadata/test_recover_partitions.py:
http://gerrit.cloudera.org:8080/#/c/10210/1/tests/metadata/test_recover_partitions.py@368
PS1, Line 368: num_partitions = 700
do we really need 700? seems like a lot
http://gerrit.cloudera.org:8080/#/c/10210/1/tests/metadata/test_recover_partitions.py@395
PS1, Line 395: self.filesystem_client.make_dir(root_path + new_dir);
os.path.join?
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 01:12:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10210 )
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 15:55:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10210 )
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 19:49:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10210 )
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2371/
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 15:56:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10210 )
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10210/1/tests/metadata/test_recover_partitions.py
File tests/metadata/test_recover_partitions.py:
http://gerrit.cloudera.org:8080/#/c/10210/1/tests/metadata/test_recover_partitions.py@368
PS1, Line 368: # Adds partition directories.
> do we really need 700? seems like a lot
dialed it down (that number was from the test above)
http://gerrit.cloudera.org:8080/#/c/10210/1/tests/metadata/test_recover_partitions.py@395
PS1, Line 395: filesystems."""
> os.path.join?
Done
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 01:33:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10210 )
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
IMPALA-6885: handle empty s3 dirs in recover_partitions test
Two tests (test_duplicate_partitions, test_support_all_types)
added only directories, which is a no-op on s3. This results
in s3 test failures.
Another test (test_recover_partitions) falsely passed for s3
since its test for a malformed partition name is ambiguous
(did it pass because recovering correctly excluded or because
the directory does not exist, as in the case of s3?).
A recent change fixed the asserts in this test, so these tests
likely were falsely reporting success.
This change bundles directory creation with adding to it a new
file, which is the common case in this test. As a result, the
peculiarity with s3 is not handled via a special case each time
the filesystem is modified for new partitions.
This change also adds an explicit test for empty partition
directories, since the first change leaves this case untested.
Testing:
- tested against a local minicluster and s3
Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Reviewed-on: http://gerrit.cloudera.org:8080/10210
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M tests/common/skip.py
M tests/metadata/test_recover_partitions.py
2 files changed, 59 insertions(+), 25 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6885: handle empty s3 dirs in recover partitions test
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10210
to look at the new patch set (#2).
Change subject: IMPALA-6885: handle empty s3 dirs in recover_partitions test
......................................................................
IMPALA-6885: handle empty s3 dirs in recover_partitions test
Two tests (test_duplicate_partitions, test_support_all_types)
added only directories, which is a no-op on s3. This results
in s3 test failures.
Another test (test_recover_partitions) falsely passed for s3
since its test for a malformed partition name is ambiguous
(did it pass because recovering correctly excluded or because
the directory does not exist, as in the case of s3?).
A recent change fixed the asserts in this test, so these tests
likely were falsely reporting success.
This change bundles directory creation with adding to it a new
file, which is the common case in this test. As a result, the
peculiarity with s3 is not handled via a special case each time
the filesystem is modified for new partitions.
This change also adds an explicit test for empty partition
directories, since the first change leaves this case untested.
Testing:
- tested against a local minicluster and s3
Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
---
M tests/common/skip.py
M tests/metadata/test_recover_partitions.py
2 files changed, 59 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10210/2
--
To view, visit http://gerrit.cloudera.org:8080/10210
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6c74f4f2466b5870322bdf373e8a6a4d1d679f4
Gerrit-Change-Number: 10210
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>