[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Tim Armstrong has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. IMPALA-7025: ignore resources in some planner test The issue was that the tablesample test verified the mem-estimate number, which depends on file sizes, which can vary slightly between data loads. Instead of trying to tweak the test to avoid the issue, instead provide a mechanism to ignore the exact values of resources in planner tests where they are not significant. Testing: Manually modified some values in tablesample.test, made sure that the test still passed. Manually modified the partition count in the expected output, made sure that the test failed. Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac --- M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 3 files changed, 91 insertions(+), 45 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10410/2 -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10410/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10410/2//COMMIT_MSG@13 PS2, Line 13: Instead of trying to tweak the test to avoid the issue, instead provide Why not ignore them everywhere instead of the tests that are specifically designed to check them? If the issue is related to data loading, then somebody might add a new planner test that breaks. It's not commented or documented anywhere why some tests ignore the mem estimates and others don't, so other people will not easily know whether to ignore or not when authoring new tests. -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Comment-Date: Tue, 15 May 2018 19:12:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10410/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10410/2//COMMIT_MSG@13 PS2, Line 13: Instead of trying to tweak the test to avoid the issue, instead provide > Why not ignore them everywhere instead of the tests that are specifically d We need to test the values in the tests that are meant to validate the resource calculations. We will need to tweak or design those tests so that they're robust to slight changes in file size or data layout, but this approach means we don't need to tweak tests like testTableSample unnecessarily. Or are you saying that we should whitelist tests that are explicitly checking them? -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 May 2018 20:07:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/10410/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10410/2//COMMIT_MSG@13 PS2, Line 13: Instead of trying to tweak the test to avoid the issue, instead provide > We need to test the values in the tests that are meant to validate the reso Correct. I'm saying a whitelist may be more robust because it means new tests will by default not validate the mem reservation. The people adding tests around resources will know to enable them :) (but others may not know to disable them in the blacklisting approach) -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 May 2018 20:33:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10410 to look at the new patch set (#3). Change subject: IMPALA-7025: ignore resources in some planner test .. IMPALA-7025: ignore resources in some planner test The issue was that the tablesample test verified the mem-estimate number, which depends on file sizes, which can vary slightly between data loads. Instead of trying to tweak the test to avoid the issue, instead provide a mechanism to ignore the exact values of resources in planner tests where they are not significant. Testing: Manually modified some values in tablesample.test, made sure that the test still passed. Manually modified the partition count in the expected output, made sure that the test failed. Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac --- M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 3 files changed, 136 insertions(+), 72 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10410/3 -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 3: I changed the runPlannerTestFile() API to do a whitelist approach as suggested. I rolled a couple of other options into the same mechanism so each test can provide a list of the extra things it wants to enable. -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 May 2018 22:07:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@458 PS3, Line 458: PlannerTestOption.INCLUDE_EXPLAIN_HEADER, Inconsistent formatting here. -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 May 2018 22:08:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/10410 )
Change subject: IMPALA-7025: ignore resources in some planner test
..
Patch Set 3:
(3 comments)
Looks good. Minor additional cleanup would be nice.
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java
File fe/src/test/java/org/apache/impala/testutil/TestUtils.java:
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@110
PS3, Line 110: this.keyPrefix = " " + key + "=";
Why the leading space?
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@154
PS3, Line 154: boolean filterFileSize, List lineFilters) {
I think we can get rid of the 'filterFileSize' param and change the caller to
pass the appropriate filter instead
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@179
PS3, Line 179: if (SCAN_RANGE_ROW_COUNT_FILTER.matches(expectedStr)) {
Why not clump them all into one list?
--
To view, visit http://gerrit.cloudera.org:8080/10410
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac
Gerrit-Change-Number: 10410
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 15 May 2018 22:30:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Hello Alex Behm, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10410 to look at the new patch set (#4). Change subject: IMPALA-7025: ignore resources in some planner test .. IMPALA-7025: ignore resources in some planner test The issue was that the tablesample test verified the mem-estimate number, which depends on file sizes, which can vary slightly between data loads. Instead of trying to tweak the test to avoid the issue, instead provide a mechanism to ignore the exact values of resources in planner tests where they are not significant. Testing: Manually modified some values in tablesample.test, made sure that the test still passed. Manually modified the partition count in the expected output, made sure that the test failed. Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac --- M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 3 files changed, 138 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/10410/4 -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10410 )
Change subject: IMPALA-7025: ignore resources in some planner test
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java
File fe/src/test/java/org/apache/impala/testutil/TestUtils.java:
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@110
PS3, Line 110: this.keyPrefix = " " + key + "=";
> Why the leading space?
This was pretty much just preserved from the previous filters - the idea is
just that it should be a separate token rather than a suffix of a different
token. E.g. with "size" below we don't want to accidentally ignore foo-size=
too.
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@154
PS3, Line 154: boolean filterFileSize, List lineFilters) {
> I think we can get rid of the 'filterFileSize' param and change the caller
Good point.
http://gerrit.cloudera.org:8080/#/c/10410/3/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@179
PS3, Line 179: if (SCAN_RANGE_ROW_COUNT_FILTER.matches(expectedStr)) {
> Why not clump them all into one list?
Done
--
To view, visit http://gerrit.cloudera.org:8080/10410
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac
Gerrit-Change-Number: 10410
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 15 May 2018 22:52:43 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 May 2018 23:06:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2484/ -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 May 2018 23:06:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 5: Code-Review+2 carry -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 15 May 2018 23:10:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 16 May 2018 02:23:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7025: ignore resources in some planner test
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10410 ) Change subject: IMPALA-7025: ignore resources in some planner test .. IMPALA-7025: ignore resources in some planner test The issue was that the tablesample test verified the mem-estimate number, which depends on file sizes, which can vary slightly between data loads. Instead of trying to tweak the test to avoid the issue, instead provide a mechanism to ignore the exact values of resources in planner tests where they are not significant. Testing: Manually modified some values in tablesample.test, made sure that the test still passed. Manually modified the partition count in the expected output, made sure that the test failed. Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Reviewed-on: http://gerrit.cloudera.org:8080/10410 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java 3 files changed, 138 insertions(+), 86 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/10410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I91e3e416ec6242fbf22d9f566fdd1ce225cb16ac Gerrit-Change-Number: 10410 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
