[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Matthew Jacobs has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 ) Change subject: IMPALA-5736: Add impala-shell argument to set default query options .. Patch Set 1: > MJ, do you prefer one option with a comma separated list of > key=value pairs, or using several options similar to --var ? I don't have a preference. When I commented on the JIRA I hadn't considered the alternative proposed by Phil. I think that sounds reasonable. -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 1 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 21 Sep 2017 17:45:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5908: Allow SET to unset modified query options. .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG Commit Message: PS9, Line 29: For request_pool, this means that setting the default : request_pool via impalad command line is now a bad idea > Okay. What is the right way for the user to do this (so that we can documen To do what? To have a default pool? That should rely on using the 'default' rule. -- To view, visit http://gerrit.cloudera.org:8080/8070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5908: Allow SET to unset modified query options. .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG Commit Message: PS9, Line 29: For request_pool, this means that setting the default : request_pool via impalad command line is now a bad idea > could you check with MJ about whether people do that? I get the feeling tha I don't have data to back this up, but I'd guess that some folks are probably doing it even though it isn't the right thing to do (they should use the "default" placement rule to map specifically into the default pool.) The case that might break is when a user has: a) placement rules set up like "1) specified, 2) anything else, e.g. 'default' or 'user'" b) they set --default_query_options=request_pool=foo c) then rely on manually setting the session query option request_pool="" to get the mapping defined by the 2nd placement rule. That said, I think this is a bad practice so I wouldn't be opposed to fixing this as long as we clearly release note it, and perhaps issue a warning if starting up the impala server with default_query_options=request_pool=foo -- To view, visit http://gerrit.cloudera.org:8080/8070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5920: addendum - add missing RAT check
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5920: addendum - add missing RAT check .. Patch Set 2: Thanks for fixing this. @Alex The test job had a known spurious failure cleaning up the workspace, so I didn't notice the RAT failure. Also I hadn't seen RAT fail before, so didn't know to even look for it. -- To view, visit http://gerrit.cloudera.org:8080/8108 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. Patch Set 7: > The rat-check job actually failed because of a file introduced here > - it broke my GVO. https://jenkins.impala.io/job/rat-check/1929/ Sorry about that - I haven't seen the RAT check fail before. Thanks for updating the rat exclude list. -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has submitted this change and it was merged. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. IMPALA-5920: Remove admission control dependency on YARN RM jar Impala's admission controller relies on the YARN fair-scheduler.xml for configuration. That configuration is loaded using YARN directly (ie. as a library by the frontend). In Hadoop 3, a number of changes were made to the YARN resourcemanager which break Impala. While we eventually want to rethink the admission control configuration (IMPALA-4159), in the meantime we at least should avoid using unsupported YARN APIs. This patch removes the fe dependency on the YARN artifact 'hadoop-yarn-server-resourcemanager' which contains private APIs and isn't meant to be used as a library. A subset of the required code has been added in 'common/yarn-extras', taken from Hadoop 2.6.0 in CDH, e.g. see [1]. The code is added in packages 'org.apache.impala.*' instead of 'org.apache.yarn.*'. Some code could be copied as-is, those files are marked with the comment: //YARNUTIL: VANILLA Files that required some modifications are marked with: //YARNUTIL: MODIFIED Or, if all code except a dummy interface could be added: //YARNUTIL: DUMMY IMPL The goal the 'yarn-extras' is to make Impala's handling of the AC configuration self-sufficient, i.e. it shouldn't matter what version of Hadoop exists. As-is, this was tested and found to work when Hadoop 2.6 is installed. Because the yarn-extras/pom.xml still references hadoop-common, hadoop-yarn-common, and hadoop-yarn-api, additional testing will be required to ensure Impala using yarn-extras works when installed along side Hadoop 3. That testing for Hadoop 3 will be done later. Future changes will make any other changes required for existing code to work when Hadoop 3 is installed. Testing: * Ran existing tests on master. 1: https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Reviewed-on: http://gerrit.cloudera.org:8080/8035 Reviewed-by: Matthew Jacobs Tested-by: Matthew Jacobs --- M CMakeLists.txt A common/yarn-extras/CMakeLists.txt A common/yarn-extras/README.txt A common/yarn-extras/pom.xml A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/resource/ResourceWeights.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/utils/BuilderUtils.java M fe/CMakeLists.txt M fe/pom.xml M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java 18 files changed, 1,690 insertions(+), 17 deletions(-) Approvals: Matthew Jacobs: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. Patch Set 6: Verified+1 The job actually passed, cleaning up the workspace failed 21:19:36 ERROR: Step ‘Delete workspace when build is done’ failed: Cannot delete workspace: remote file operation failed: /home/ubuntu at hudson.remoting.Channel@43c5313:ubuntu-16.04 (i-0f8cf68ee32aebe80): java.io.IOException: Unable to delete '/home/ubuntu'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts. 21:19:36 Finished: SUCCESS -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. Patch Set 6: Code-Review+2 I'll count the +1s from Tim and Zach, and no further comments from Phil as enough to commit this change. -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..
Patch Set 5:
(2 comments)
Thanks for the detailed review, Zach. If nobody has further feedback, I'll try
to get this in soon.
http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
File
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java:
Line 103: public void serviceInit(Configuration conf) throws Exception {
> Can you file a JIRA? This is actually a well compartmentalized ramp-up or
https://issues.apache.org/jira/browse/IMPALA-5958
http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java
File
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java:
Line 57:
> Is it worth fixing the trailing spaces so future diffs don't give the linte
These are their trailing spaces though - I figured leaving them in would make
diffing easier later.
--
To view, visit http://gerrit.cloudera.org:8080/8035
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zach Amsden
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8035 to look at the new patch set (#5). Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. IMPALA-5920: Remove admission control dependency on YARN RM jar Impala's admission controller relies on the YARN fair-scheduler.xml for configuration. That configuration is loaded using YARN directly (ie. as a library by the frontend). In Hadoop 3, a number of changes were made to the YARN resourcemanager which break Impala. While we eventually want to rethink the admission control configuration (IMPALA-4159), in the meantime we at least should avoid using unsupported YARN APIs. This patch removes the fe dependency on the YARN artifact 'hadoop-yarn-server-resourcemanager' which contains private APIs and isn't meant to be used as a library. A subset of the required code has been added in 'common/yarn-extras', taken from Hadoop 2.6.0 in CDH, e.g. see [1]. The code is added in packages 'org.apache.impala.*' instead of 'org.apache.yarn.*'. Some code could be copied as-is, those files are marked with the comment: //YARNUTIL: VANILLA Files that required some modifications are marked with: //YARNUTIL: MODIFIED Or, if all code except a dummy interface could be added: //YARNUTIL: DUMMY IMPL The goal the 'yarn-extras' is to make Impala's handling of the AC configuration self-sufficient, i.e. it shouldn't matter what version of Hadoop exists. As-is, this was tested and found to work when Hadoop 2.6 is installed. Because the yarn-extras/pom.xml still references hadoop-common, hadoop-yarn-common, and hadoop-yarn-api, additional testing will be required to ensure Impala using yarn-extras works when installed along side Hadoop 3. That testing for Hadoop 3 will be done later. Future changes will make any other changes required for existing code to work when Hadoop 3 is installed. Testing: * Ran existing tests on master. 1: https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf --- M CMakeLists.txt A common/yarn-extras/CMakeLists.txt A common/yarn-extras/README.txt A common/yarn-extras/pom.xml A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/resource/ResourceWeights.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/utils/BuilderUtils.java M fe/CMakeLists.txt M fe/pom.xml M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java 18 files changed, 1,690 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/5 -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..
Patch Set 4:
(13 comments)
Thanks, Zach - some good observations. I made a bunch of the changes for
additional cleanup that you suggested, but some I held off on because I don't
wanna make diffing the more complex classes harder later. We can always choose
to simplify this further later, if we want. Let me know if you feel strongly
about it.
http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
File
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java:
Line 61: private final float queueMaxAMShareDefault;
> Impala doesn't make use of YARN's concept of ApplicationMasters. Why not k
Done
Line 64: private final Map>
queueAcls;
> Useless (just SUBMIT_APPLICATIONS)
I don't think it's worth changing this one otherwise I'll have to modify a
bunch of code that I don't wanna bother adding extra tests for.
Line 69: private final Map minSharePreemptionTimeouts;
> Unused by Impala
Done
Line 74: private final Map fairSharePreemptionTimeouts;
> Also unused by Impala
Done
Line 80: private final Map fairSharePreemptionThresholds;
> Also unused
Done
Line 82: private final Map schedulingPolicies;
> The only policy is DEFAULT_POLICY so this is also not needed.
Done
Line 84: private final SchedulingPolicy defaultSchedulingPolicy;
> Ditto
Done
http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
File
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java:
Line 30: public class AllocationConfigurationException extends Exception {
> The only point of this class is capturing configuration exceptions and thro
We still use the resource parsing code that throws this, e.g. in
FairSchedulerConfiguration. I don't think we should change the API of those
functions.
http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
File
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java:
Line 103: public void serviceInit(Configuration conf) throws Exception {
> 103-165 duplicate code and functionality provided by impala.util.FileWatchS
Yeah you're right, though I'm on the fence about how much we should doctor this
right now given we know this works and that we don't have great automated
end-to-end test coverage.
Unless you feel strongly about cleaning up this now, I'd rather leave this as a
cleanup opportunity for later (ramp up JIRA?) when we can add some additional
tests which include modifying the allocation file.
Line 223: Map minSharePreemptionTimeouts = new HashMap<>();
> Preemption configuration does nothing in Impala; could also be killed.
I think it may be worth leaving this code as-is even if it's doing extra work.
The AllocationConfiguration will ignore the things we don't care about, but
leaving this as-is will make diff-ing this later much easier.
http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
File
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java:
Line 70: Pattern pattern = Pattern.compile("(\\d+)(\\.\\d*)?\\s*" + units);
> This seems a pretty inefficient way to look up resources. This would be a c
Agreed, though I don't think it's worth spending time improving this code. I'd
rather not make diffing harder later. Do you feel strongly?
http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
File
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java:
Line 25: return DEFAULT_POLICY;
> This whole class is pretty useless and in Impala's case, only serves to thr
Per my other comments, I'd prefer not to change the parsing code too much yet,
in which case I think it'll just be worth keeping this dummy impl here for now.
http://gerrit.cloudera.org:8080/#/c/8035/4/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
File fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java:
Line 124: new Configuration().get(HADOOP_SECURITY_AUTH_TO_LOCAL,
"DEFAULT"));
> Not clear to me why this is needed now.
I'm actually not
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8035 to look at the new patch set (#4). Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. IMPALA-5920: Remove admission control dependency on YARN RM jar Impala's admission controller relies on the YARN fair-scheduler.xml for configuration. That configuration is loaded using YARN directly (ie. as a library by the frontend). In Hadoop 3, a number of changes were made to the YARN resourcemanager which break Impala. While we eventually want to rethink the admission control configuration (IMPALA-4159), in the meantime we at least should avoid using unsupported YARN APIs. This patch removes the fe dependency on the YARN artifact 'hadoop-yarn-server-resourcemanager' which contains private APIs and isn't meant to be used as a library. A subset of the required code has been added in 'common/yarn-extras', taken from Hadoop 2.6.0 in CDH, e.g. see [1]. The code is added in packages 'org.apache.impala.*' instead of 'org.apache.yarn.*'. Some code could be copied as-is, those files are marked with the comment: //YARNUTIL: VANILLA Files that required some modifications are marked with: //YARNUTIL: MODIFIED Or, if all code except a dummy interface could be added: //YARNUTIL: DUMMY IMPL The goal the 'yarn-extras' is to make Impala's handling of the AC configuration self-sufficient, i.e. it shouldn't matter what version of Hadoop exists. As-is, this was tested and found to work when Hadoop 2.6 is installed. Because the yarn-extras/pom.xml still references hadoop-common, hadoop-yarn-common, and hadoop-yarn-api, additional testing will be required to ensure Impala using yarn-extras works when installed along side Hadoop 3. That testing for Hadoop 3 will be done later. Future changes will make any other changes required for existing code to work when Hadoop 3 is installed. Testing: * Ran existing tests on master. 1: https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf --- M CMakeLists.txt A common/yarn-extras/CMakeLists.txt A common/yarn-extras/README.txt A common/yarn-extras/pom.xml A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/resource/ResourceWeights.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java A common/yarn-extras/src/main/java/org/apache/impala/yarn/server/utils/BuilderUtils.java M fe/CMakeLists.txt M fe/pom.xml M fe/src/main/java/org/apache/impala/util/RequestPoolService.java M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java 18 files changed, 1,805 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/4 -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/8035/3/common/yarn-extras/pom.xml File common/yarn-extras/pom.xml: Line 23: org.apache.impala > I think we should move the implementing classes here to "org.apache.impala" Done -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5905: build-all-flag-combinations addendum
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5905: build-all-flag-combinations addendum .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa97a981846a2397ecabb90b9039ba61c2c7af4e Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/8035/2//COMMIT_MSG Commit Message: Line 23: 'common/yarn-extras', taken from Hadoop 2.6.0. > Is the original source code that you cribbed this from available publicly s Done http://gerrit.cloudera.org:8080/#/c/8035/2/common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java File common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java: Line 19: //YARNUTIL: MODIFIED > Is there an easy way I can see the diff? If you point me to the hadoop sour Yeah, based on the CDH source tarball, e.g. https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html#cm_vd_cdh_package_tarball_512 I'll update the commit message. http://gerrit.cloudera.org:8080/#/c/8035/2/fe/pom.xml File fe/pom.xml: Line 111: system > Is systemPath necessary? We seem to be able to resolve data-source-api abov Done -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has uploaded a new patch set (#3). Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. IMPALA-5920: Remove admission control dependency on YARN RM jar Impala's admission controller relies on the YARN fair-scheduler.xml for configuration. That configuration is loaded using YARN directly (ie. as a library by the frontend). In Hadoop 3, a number of changes were made to the YARN resourcemanager which break Impala. While we eventually want to rethink the admission control configuration (IMPALA-4159), in the meantime we at least should avoid using unsupported YARN APIs. This patch removes the fe dependency on the YARN artifact 'hadoop-yarn-server-resourcemanager' which contains private APIs and isn't meant to be used as a library. A subset of the required code has been added in 'common/yarn-extras', taken from Hadoop 2.6.0 in CDH, e.g. see [1]. The goal the 'yarn-extras' is to make Impala's handling of the AC configuration self-sufficient, i.e. it shouldn't matter what version of Hadoop exists. As-is, this was tested and found to work when Hadoop 2.6 is installed. Because the yarn-extras/pom.xml still references hadoop-common, hadoop-yarn-common, and hadoop-yarn-api, additional testing will be required to ensure Impala using yarn-extras works when installed along side Hadoop 3. That testing for Hadoop 3 will be done later. Future changes will make any other changes required for existing code to work when Hadoop 3 is installed. Testing: * Ran existing tests on master. 1: https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf --- M CMakeLists.txt A common/yarn-extras/CMakeLists.txt A common/yarn-extras/README.txt A common/yarn-extras/pom.xml A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/resource/ResourceWeights.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java M fe/CMakeLists.txt M fe/pom.xml M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java 17 files changed, 1,802 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/3 -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] [DOCS] Explain Boost setting needed for 96-bit timestamps
Matthew Jacobs has posted comments on this change. Change subject: [DOCS] Explain Boost setting needed for 96-bit timestamps .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/7983/2/docs/shared/impala_common.xml File docs/shared/impala_common.xml: PS2, Line 848: our who is 'our' here? PS2, Line 850: , : which matches the representation for TIMESTAMP in Impala , and is required in order to use TIMESTAMP. Line 855: it might be good to just say that you should probably just define this variable always, unless you have a really good reason not to -- To view, visit http://gerrit.cloudera.org:8080/7983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b67cd7762f682c3a054e0d9641080aa51801c83 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: John Russell Gerrit-Reviewer: John Russell Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Bump Kudu version to 3f49724
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 3f49724 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21aff562e2bca90436a8d0206ffca44b712bc1f1 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar
Matthew Jacobs has uploaded a new patch set (#2). Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar .. IMPALA-5920: Remove admission control dependency on YARN RM jar Impala's admission controller relies on the YARN fair-scheduler.xml for configuration. That configuration is loaded using YARN directly (ie. as a library by the frontend). In Hadoop 3, a number of changes were made to the YARN resourcemanager which break Impala. While we eventually want to rethink the admission control configuration (IMPALA-4159), in the meantime we at least should avoid using unsupported YARN APIs. This patch removes the fe dependency on the YARN artifact 'hadoop-yarn-server-resourcemanager' which contains private APIs and isn't meant to be used as a library. A subset of the required code has been added in 'common/yarn-extras', taken from Hadoop 2.6.0. The goal the 'yarn-extras' is to make Impala's handling of the AC configuration self-sufficient, i.e. it shouldn't matter what version of Hadoop exists. As-is, this was tested and found to work when Hadoop 2.6 is installed. Because the yarn-extras/pom.xml still references hadoop-common, hadoop-yarn-common, and hadoop-yarn-api, additional testing will be required to ensure Impala using yarn-extras works when installed along side Hadoop 3. That testing for Hadoop 3 will be done later. Future changes will make any other changes required for existing code to work when Hadoop 3 is installed. Testing: * Ran existing tests on master. Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf --- M CMakeLists.txt A common/yarn-extras/CMakeLists.txt A common/yarn-extras/README.txt A common/yarn-extras/pom.xml A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/resource/ResourceWeights.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java M fe/CMakeLists.txt M fe/pom.xml M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java 17 files changed, 1,804 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/2 -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN resourcemanager
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/8035 Change subject: IMPALA-5920: Remove admission control dependency on YARN resourcemanager .. IMPALA-5920: Remove admission control dependency on YARN resourcemanager Impala's admission controller relies on the YARN fair-scheduler.xml for configuration. That configuration is loaded using YARN directly (ie. as a library by the frontend). In Hadoop 3, a number of changes were made to the YARN resourcemanager which break Impala. While we eventually want to rethink the admission control configuration (IMPALA-4159), in the meantime we at least should avoid using unsupported YARN APIs. This patch removes the fe dependency on the YARN artifact 'hadoop-yarn-server-resourcemanager' which contains private APIs and isn't meant to be used as a library. A subset of the required code has been added in 'common/yarn-extras', taken from Hadoop 2.6.0. The goal the 'yarn-extras' is to make Impala's handling of the AC configuration self-sufficient, i.e. it shouldn't matter what version of Hadoop exists. As-is, this was tested and found to work when Hadoop 2.6 is installed. Because the yarn-extras/pom.xml still references hadoop-common, hadoop-yarn-common, and hadoop-yarn-api, additional testing will be required to ensure Impala using yarn-extras works when installed along side Hadoop 3. That testing for Hadoop 3 will be done later. Future changes will make any other changes required for existing code to work when Hadoop 3 is installed. Testing: * Ran existing tests on master. Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf --- M CMakeLists.txt A common/yarn-extras/CMakeLists.txt A common/yarn-extras/README.txt A common/yarn-extras/pom.xml A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/resource/ResourceWeights.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java A common/yarn-extras/src/main/java/org/apache/hadoop/yarn/server/utils/BuilderUtils.java M fe/CMakeLists.txt M fe/pom.xml M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java 17 files changed, 1,804 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/1 -- To view, visit http://gerrit.cloudera.org:8080/8035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
[native-toolchain-CR] Bump Kudu version to 3f49724
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 3f49724 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8028 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38334bdbaa1e7a6c6f5d595c26efb76efa062305 Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] Bump Kudu version to a71ecfd
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/8000 Change subject: Bump Kudu version to a71ecfd .. Bump Kudu version to a71ecfd Change-Id: Ie23d852f0d630f9484d8ae4f772af6bba13ea24f --- M bin/impala-config.sh 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8000/1 -- To view, visit http://gerrit.cloudera.org:8080/8000 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie23d852f0d630f9484d8ae4f772af6bba13ea24f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
[Impala-ASF-CR] IMPALA-5867: Fix bugs parsing 2-digit year
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5867: Fix bugs parsing 2-digit year .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7910/3//COMMIT_MSG Commit Message: PS3, Line 22: 1. A date without month/day is considered invalid. Note this is :different from Hive. > Yeah this is a pre-existing difference from Hive. If you omit month or day It'd be good to make it clear what is a pre-existing difference. This made it sound like this may have changed. -- To view, visit http://gerrit.cloudera.org:8080/7910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4f430caea88b6c33f8050a1984ee0ee32ecb0a1 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[native-toolchain-CR] Bump Kudu version to a71ecfd
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to a71ecfd .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/7975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4fb177b80f6c193af3f6b6bf9d7b205ab31d2f3e Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: No
[native-toolchain-CR] Bump Kudu version to a71ecfd
Matthew Jacobs has submitted this change and it was merged. Change subject: Bump Kudu version to a71ecfd .. Bump Kudu version to a71ecfd Change-Id: I4fb177b80f6c193af3f6b6bf9d7b205ab31d2f3e --- M buildall.sh 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Matthew Jacobs: Verified Thomas Tauber-Marshall: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I4fb177b80f6c193af3f6b6bf9d7b205ab31d2f3e Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall
[native-toolchain-CR] Bump Kudu version to a71ecfd
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/7975 Change subject: Bump Kudu version to a71ecfd .. Bump Kudu version to a71ecfd Change-Id: I4fb177b80f6c193af3f6b6bf9d7b205ab31d2f3e --- M buildall.sh 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/75/7975/1 -- To view, visit http://gerrit.cloudera.org:8080/7975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I4fb177b80f6c193af3f6b6bf9d7b205ab31d2f3e Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options .. Patch Set 2: Please fix the typos before submitting -- To view, visit http://gerrit.cloudera.org:8080/7886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options .. Patch Set 2: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/7886/2//COMMIT_MSG Commit Message: PS2, Line 39: the simply empty string approach extra word? PS2, Line 44: beneift typo -- To view, visit http://gerrit.cloudera.org:8080/7886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5891: fix PeriodicCounterUpdater initialization
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5891: fix PeriodicCounterUpdater initialization .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7942 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieede9fa194605fb53033033959110f3ef12f18c3 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5867: Fix bugs parsing 2-digit year
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5867: Fix bugs parsing 2-digit year .. Patch Set 3: (1 comment) The fact that we found 3 issues in this code (after IMPALA-3894) makes me nervous. I didn't look in close detail yet, but given 2.10 is looming and I'm worried about all the subtleties of the Timestamp code, I think we should consider reverting 5797f859692ced6f30a790568b29669c663fa65c for 2.10 and getting this fix into master afterwards. I think we need to think about ways to get more test coverage here to make sure we don't have other crashes or incorrect results in other corner cases. http://gerrit.cloudera.org:8080/#/c/7910/3//COMMIT_MSG Commit Message: PS3, Line 22: 1. A date without month/day is considered invalid. Note this is :different from Hive. why don't we do the same thing as hive? -- To view, visit http://gerrit.cloudera.org:8080/7910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4f430caea88b6c33f8050a1984ee0ee32ecb0a1 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tianyi Wang Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5871: KuduPartitionExpr incorrectly handles its child types
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5871: KuduPartitionExpr incorrectly handles its child types .. Patch Set 1: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/7922/1//COMMIT_MSG Commit Message: PS1, Line 22: compatibly types col compatible type? http://gerrit.cloudera.org:8080/#/c/7922/1/fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java File fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java: PS1, Line 76: children_.set( wow, whoops... -- To view, visit http://gerrit.cloudera.org:8080/7922 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I44cf31e46a77f3e7c92cf6b9112653808a001705 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
..
Patch Set 3: Code-Review+1
(3 comments)
Looks good, thanks! Just a few small things. Clearly Alex still needs to review
this :)
http://gerrit.cloudera.org:8080/#/c/7829/3/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
PS3, Line 319: boolean ret = (rewritten instanceof BoolLiteral &&
((BoolLiteral) rewritten).getValue());
I assume you meant to delete this line
http://gerrit.cloudera.org:8080/#/c/7829/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 436: RewritesOk("isnottrue(null)", rule, "TRUE");
RewritesOk("isfalse(true)", rule, "FALSE");
nit next line
PS3, Line 451: rules
as we discussed, it'd be helpful to rename this
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Philip Zeyliger
Gerrit-HasComments: Yes
[Impala-ASF-CR] Bump Kudu version to 1c70e5d
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 1c70e5d .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7903 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I39eee4dc1541adfda0743a5a064dbd2eae6f3d48 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5854: Update external hadoop versions
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5854: Update external hadoop versions .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7892/1/bin/impala-config.sh File bin/impala-config.sh: PS1, Line 126: export KUDU_JAVA_VERSION=1.5.0-cdh5.14.0-SNAPSHOT > Not sure how I missed that - this only seems to be used in the pom.xml and I think they may have goofed in publishing multiple versions, 1.5.0-cdh5.14.0 seems to exist in the artifactory as well, though that is wrong. We do use this for the FE Kudu client usage, so we should update it. -- To view, visit http://gerrit.cloudera.org:8080/7892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ab9512c3fa3e94c3a4fa9eeb49ff5563a114954 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5863: Include-what-you-use for Kudu client
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5863: Include-what-you-use for Kudu client .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5854: Update external hadoop versions
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5854: Update external hadoop versions .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7892/1/bin/impala-config.sh File bin/impala-config.sh: PS1, Line 126: export KUDU_JAVA_VERSION=1.5.0-cdh5.14.0-SNAPSHOT > Can Thomas or MJ sign off on this line? I think this should be 1.6.0-cdh5.14.0-SNAPSHOT http://maven.jenkins.cloudera.com:8081/artifactory/cdh-snapshot-local/org/apache/kudu/kudu-client/1.6.0-cdh5.14.0-SNAPSHOT/ -- To view, visit http://gerrit.cloudera.org:8080/7892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ab9512c3fa3e94c3a4fa9eeb49ff5563a114954 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7886/1//COMMIT_MSG Commit Message: PS1, Line 7: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options an impala shell test would be nice too e.g. some test file in tests/shell/ PS1, Line 7: imapla this would be the name of our imap server PS1, Line 40: : The other users of this state, I think HiveServer2's OpenSession() : call and HiveServer2's response to a "SET" query are affected. It seems : like they'd benefit from the same fix, but I've not been able to : adequately run through that code path. you can add a hs2 test in tests/hs2/ , probably test_hs2.py http://gerrit.cloudera.org:8080/#/c/7886/1/be/src/service/query-options.h File be/src/service/query-options.h: PS1, Line 103: that that aren't set and lack defaults PS1, Line 104: considered "unset", which is mapped this is just a bit wordy, I think this makes it more clear: ...are mapped to the empty string -- To view, visit http://gerrit.cloudera.org:8080/7886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] Include-what-you-use for Kudu client
Matthew Jacobs has posted comments on this change. Change subject: Include-what-you-use for Kudu client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7872/2//COMMIT_MSG Commit Message: Line 7: Include-what-you-use for Kudu client since this may need to be tracked for backports, please file a JIRA for this even though it's trivial -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5857: avoid invalid free of hedged read metrics .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] Bump Kudu version to 1c70e5d
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 1c70e5d .. Patch Set 1: (1 comment) does the code not compile without the header changes? It's best to separate code changes from toolchain version bumps when possible because it makes porting to other branches harder. It's OK if it's not possible. http://gerrit.cloudera.org:8080/#/c/7872/1/be/src/exprs/kudu-partition-expr.h File be/src/exprs/kudu-partition-expr.h: PS1, Line 21: #include : #include can you try to just forward declare instead of including here (then including in the .cc)? for both kudu::client::KuduPartitioner and kudu::KuduPartialRow? I think that a unique_ptr doesn't need the full class definition. -- To view, visit http://gerrit.cloudera.org:8080/7872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5855: reserve enough memory for preaggs
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5855: reserve enough memory for preaggs .. Patch Set 2: I think you may need to update test admission-reject-min-reservation.test now too, since that went in last night -- To view, visit http://gerrit.cloudera.org:8080/7871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I870fbe2f1da01c6123d3716a1198376f9a454c3b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5852: improve MINIMUM RESERVATION UNAVAILABLE error
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5852: improve MINIMUM_RESERVATION_UNAVAILABLE error .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7861/1/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: PS1, Line 328: memory-based admission control > are we ready to recommend that widely? maybe we should just say "admission I think we already do recommend it if you have a very specific set of use cases. If you think this message would imply to some users that it might work in all cases, then I agree with your simplification. -- To view, visit http://gerrit.cloudera.org:8080/7861 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e367e1b0cb08e11fdd0546880df23b785e3b7c9 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5852: improve MINIMUM RESERVATION UNAVAILABLE error
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5852: improve MINIMUM_RESERVATION_UNAVAILABLE error .. Patch Set 1: Code-Review+1 Looks more helpful to me, thanks -- To view, visit http://gerrit.cloudera.org:8080/7861 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e367e1b0cb08e11fdd0546880df23b785e3b7c9 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. Patch Set 6: Code-Review+2 Agg memory changed in the test cases after Tim's recent patch for pre-agg reservations. Updated the tests. -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Hello Philip Zeyliger, Impala Public Jenkins, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7834 to look at the new patch set (#6). Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. IMPALA-5838: Improve errors on AC buffer mem rejection The error message returned when a query is rejected due to insufficient buffer memory is misleading. It recommended a mem_limit which would be high enough, but changing the mem_limit may result in changing the plan, which may result in further changes to the buffer memory requirement. In particular, this can happen when the planner compares the expected hash table size to the mem_limit, and decides to choose a partitioned join over a broadcast join. While we might consider other code changes to improve this, for now lets just be clear in the error message. Testing: * Adds tests that verify the expected behavior with the new error message. Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 --- M be/src/scheduling/admission-controller.cc A testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/custom_cluster/test_admission_controller.py 3 files changed, 57 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/7834/6 -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
..
Patch Set 2:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
PS2, Line 145: /**
:* Helper for simplifying unary boolean function (like
istrue(x)).
:*/
: private Expr unaryHelper(Expr expr, LiteralExpr child,
: boolean onTrue, boolean onFalse, boolean onNull)
I'm not crazy about this interface because I had to read all the code to really
understand what it was doing. I don't have a better suggestion yet.
PS2, Line 153: Boolean
is there a reason you use Boolean over boolean? Given getValue() returns the
primitive type, I'd prefer the primitive type so that readers don't have to
reason about this being null.
PS2, Line 154: val
also, I don't see a reason not to getValue() inline here
PS2, Line 165:
: /**
:* Helper for simplifying unary functions like isnull(x).
:*/
: private Expr unaryHelper2(Expr expr, LiteralExpr child,
boolean onNull,
: boolean onOther)
same
Line 179: private Expr simplifyFunctionCallExpr(FunctionCallExpr expr,
Analyzer analyzer) throws AnalysisException {
nit: long line, please wrap at 90
PS2, Line 197: //
ontrue onfalse onnull
: if (fnName.equals("isfalse"))return
unaryHelper(expr, c, false, true, false);
nit: we typically don't use cool whitespace formatting to line things up. I
don't feel strongly though, and in this case it does seem to be helpful. That
said, I think we should try to find a way to make the function names/parameters
a bit easier to understand on their own.
PS2, Line 235: x
y
Line 237:* Note that we currently don't simplify all possible equal
expressions
This may be obvious, but not to me. Would you mind elaborating? I understand
why the cast case can't be simplified, but not the case involving argument
ordering.
Line 243: private Expr simplifyNullIfFunctionCallExpr(Expr expr, Analyzer
analyzer) throws AnalysisException {
nit long line
PS2, Line 248: literalExprsEqual
can you explain why this does constant folding but other optimizations do not?
PS2, Line 300: Rewrites a = b
Doesn't indicate that this creates Expr 'a = b'. How about:
Returns a new Expr 'a = b', after folding constants.
PS2, Line 303: literal
this fn doesn't appear to require literal exprs. A more precise name might be
something like: foldedExprsEqualExpr
PS2, Line 306: Expr rewritten = analyzer.getConstantFolder().rewrite(pred,
analyzer);
: return rewritten;
remove local var and return result
PS2, Line 315: return rewritten instanceof BoolLiteral &&
: ((BoolLiteral) rewritten).getValue();
nit 1line? It looks less than 90chars...
http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
PS2, Line 424: // Exhaustive test for istrue and friends on boolean
literals.
It'd be helpful to have a few more cases where you have constant expressions,
e.g. including some cases that don't get rewritten (istrue(true and true))
PS2, Line 426: // $for f in istrue isnottrue isfalse isnotfalse; do
: // for y in true false null; do
: // echo 'RewritesOk("'"$f($y)"'", rule,
"'$(impala-shell.sh -B --quiet --query "select $f($y)" 2> /dev/null | tr
'[a-z]' '[A-Z]')'");'
: // done
: // done
fancy
--
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger
Gerrit-Reviewer: Matthew Jacobs
Gerrit-HasComments: Yes
[native-toolchain-CR] Bump Kudu version to 1c70e5d
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 1c70e5d .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba57fbe903a2aea0e34851f1a0611691a9bb319e Gerrit-PatchSet: 2 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: No
[native-toolchain-CR] Bump Kudu version to 1c70e5d
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 1c70e5d .. Patch Set 2: > (1 comment) Ah, that's on Cloudera's github mirror. Let's choose the Apache repo. -- To view, visit http://gerrit.cloudera.org:8080/7855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba57fbe903a2aea0e34851f1a0611691a9bb319e Gerrit-PatchSet: 2 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: No
[native-toolchain-CR] Bump Kudu version to 22a19d9
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 22a19d9 .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7855/1//COMMIT_MSG Commit Message: PS1, Line 7: 22a19d9 where is this kudu commit? I don't see it in master -- To view, visit http://gerrit.cloudera.org:8080/7855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba57fbe903a2aea0e34851f1a0611691a9bb319e Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Hello Philip Zeyliger, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7834 to look at the new patch set (#5). Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. IMPALA-5838: Improve errors on AC buffer mem rejection The error message returned when a query is rejected due to insufficient buffer memory is misleading. It recommended a mem_limit which would be high enough, but changing the mem_limit may result in changing the plan, which may result in further changes to the buffer memory requirement. In particular, this can happen when the planner compares the expected hash table size to the mem_limit, and decides to choose a partitioned join over a broadcast join. While we might consider other code changes to improve this, for now lets just be clear in the error message. Testing: * Adds tests that verify the expected behavior with the new error message. Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 --- M be/src/scheduling/admission-controller.cc A testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/custom_cluster/test_admission_controller.py 3 files changed, 57 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/7834/5 -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Hello Philip Zeyliger, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7834 to look at the new patch set (#4). Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. IMPALA-5838: Improve errors on AC buffer mem rejection The error message returned when a query is rejected due to insufficient buffer memory is misleading. It recommended a mem_limit which would be high enough, but changing the mem_limit may result in changing the plan, which may result in further changes to the buffer memory requirement. In particular, this can happen when the planner compares the expected hash table size to the mem_limit, and decides to choose a partitioned join over a broadcast join. While we might consider other code changes to improve this, for now lets just be clear in the error message. Testing: * Adds tests that verify the expected behavior with the new error message. Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 --- M be/src/scheduling/admission-controller.cc A testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/custom_cluster/test_admission_controller.py 3 files changed, 57 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/7834/4 -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7834/3/testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test File testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test: PS3, Line 33: Mem available for buffer reservations : (based on mem_limit): 1.00 KB > I thought the mem_limit value would go inside the parentheses. Or did you i Agreed, this is backwards. -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Hello Philip Zeyliger, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7834 to look at the new patch set (#3). Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. IMPALA-5838: Improve errors on AC buffer mem rejection The error message returned when a query is rejected due to insufficient buffer memory is misleading. It recommended a mem_limit which would be high enough, but changing the mem_limit may result in changing the plan, which may result in further changes to the buffer memory requirement. In particular, this can happen when the planner compares the expected hash table size to the mem_limit, and decides to choose a partitioned join over a broadcast join. While we might consider other code changes to improve this, for now lets just be clear in the error message. Testing: * Adds tests that verify the expected behavior with the new error message. Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 --- M be/src/scheduling/admission-controller.cc A testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/custom_cluster/test_admission_controller.py 3 files changed, 57 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/7834/3 -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7834/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: PS2, Line 118: based on mem_limit: " > Yeah that would be clearer for me. Done -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7834/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: PS2, Line 118: based on mem_limit: " > I find this wording a bit confusing since it's not clear to me upon first r would it be more clear if I put (based on the mem_limit) in parens? -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7834/1/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:
Line 334: #vector.get_value('exec_option')['num_nodes'] = 1
> What's up here?
Yeah, fixed that in rev2 already. At first I was toying with this, but then
realized the join strategy change is a moot point if it's always run on 1 node.
--
To view, visit http://gerrit.cloudera.org:8080/7834
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Matthew Jacobs has uploaded a new patch set (#2). Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. IMPALA-5838: Improve errors on AC buffer mem rejection The error message returned when a query is rejected due to insufficient buffer memory is misleading. It recommended a mem_limit which would be high enough, but changing the mem_limit may result in changing the plan, which may result in further changes to the buffer memory requirement. In particular, this can happen when the planner compares the expected hash table size to the mem_limit, and decides to choose a partitioned join over a broadcast join. While we might consider other code changes to improve this, for now lets just be clear in the error message. Testing: * Adds tests that verify the expected behavior with the new error message. Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 --- M be/src/scheduling/admission-controller.cc A testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/custom_cluster/test_admission_controller.py 3 files changed, 54 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/7834/2 -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5838: Improve errors on AC buffer mem rejection
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/7834 Change subject: IMPALA-5838: Improve errors on AC buffer mem rejection .. IMPALA-5838: Improve errors on AC buffer mem rejection The error message returned when a query is rejected due to insufficient buffer memory is misleading. It recommended a mem_limit which would be high enough, but changing the mem_limit may result in changing the plan, which may result in further changes to the buffer memory requirement. In particular, this can happen when the planner compares the expected hash table size to the mem_limit, and decides to choose a partitioned join over a broadcast join. While we might consider other code changes to improve this, for now lets just be clear in the error message. Testing: * Adds tests that verify the expected behavior with the new error message. Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 --- M be/src/scheduling/admission-controller.cc A testdata/workloads/functional-query/queries/QueryTest/admission-reject-min-reservation.test M tests/custom_cluster/test_admission_controller.py 3 files changed, 56 insertions(+), 25 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/7834/1 -- To view, visit http://gerrit.cloudera.org:8080/7834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3dc3517195508d86078a8a4b537ae7d2f52fbcb7 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal. .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/7781/6/testdata/workloads/functional-planner/queries/PlannerTest/simplify-expressions.test File testdata/workloads/functional-planner/queries/PlannerTest/simplify-expressions.test: Line 1: # ifnull simplified out because first argument is constant. > Remove. My bad -- To view, visit http://gerrit.cloudera.org:8080/7781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e4b8bf6fedd595f9ea54ffb30fc71a058c7f16c Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7781/5/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: PS5, Line 298: RewritesOk(f + "(null, id)", rule, "id"); : RewritesOk(f + "(null, id)", rule, "id"); : RewritesOk(f + "(null, id)", rule, "id"); some dupes now, also f(null, null) still missing -- To view, visit http://gerrit.cloudera.org:8080/7781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e4b8bf6fedd595f9ea54ffb30fc71a058c7f16c Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5799: Kudu DML can crash if schema has changed
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5799: Kudu DML can crash if schema has changed .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9fd6bf164310df0041144f75f5ee722665e9f587 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5780,IMPALA-5779: extra spilling tests
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5780,IMPALA-5779: extra spilling tests .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03de00394bb6bbcf381250f816e22a4b987f1135 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5602: Fix query optimization for kudu and datasource tables
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5602: Fix query optimization for kudu and datasource tables .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7560 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93822d67ebda41d5d0456095c429e3915a3f40c4 Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is
a literal.
..
Patch Set 4:
(2 comments)
> > Matt: Might be nice to have some tests like those added in this
> patch which make sure the rewritten queries work with nullable
> tuples (which are a weird wart, arguably):
>
> The bug there was, if I read it right, that coalesce(non-nullable-column,
> x) was getting simplified to x, but the column could have been
> coming from an outer join. Since this change deals only in literals
> (and not columnns), I decided that adding a query test wasn't that
> valuable. If you disagree, I'd be happy to add one.
You're correct, I agree.
>
> I also wanted to see if I could tell from the explain output
> whether or not the optimization was happening. It looks like I can,
> if I put the relevant expression inside of an aggregation, since
> "output: max:merge(id)" shows up. This could also power a test. Do
> you think it's worth it?
Seems like a good idea.
>
>
> [...:21000] > explain select max(nvl(null, id)) from
> functional.alltypes limit 3;
> Query: explain select max(nvl(null, id)) from functional.alltypes
> limit 3
> +--+
> | Explain String |
> +--+
> | Max Per-Host Resource Reservation: Memory=0B |
> | Per-Host Resource Estimates: Memory=180.00MB |
> | Codegen disabled by planner |
> | |
> | PLAN-ROOT SINK |
> | ||
> | 03:AGGREGATE [FINALIZE] |
> | | output: max:merge(id) |
> | | limit: 3 |
> | ||
> | 02:EXCHANGE [UNPARTITIONED] |
> | ||
> | 01:AGGREGATE |
> | | output: max(id) |
> | ||
> | 00:SCAN HDFS [functional.alltypes] |
> |partitions=24/24 files=24 size=478.45KB |
> +--+
> Fetched 17 row(s) in 0.02s
http://gerrit.cloudera.org:8080/#/c/7781/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 298: RewritesOk("nvl(null, id)", rule, "id");
how about ifnull(null, null), though clearly the test framework handles that in
a bit of an ambiguous manner.
PS4, Line 296: RewritesOk("ifnull(null, id)", rule, "id");
: RewritesOk("isnull(null, id)", rule, "id");
: RewritesOk("nvl(null, id)", rule, "id");
: RewritesOk("ifnull(id, id + 1)", rule, null);
:
: RewritesOk("ifnull(1, 2)", rule, "1");
: RewritesOk("ifnull(0, id)", rule, "0");
wrap this in a for loop and iterate over all 3 fns?
--
To view, visit http://gerrit.cloudera.org:8080/7781
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e4b8bf6fedd595f9ea54ffb30fc71a058c7f16c
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Philip Zeyliger
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5780,IMPALA-5779: extra spilling tests
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5780,IMPALA-5779: extra spilling tests
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7787/1/testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test
File
testdata/workloads/functional-query/queries/QueryTest/disable-unsafe-spills.test:
Line 5: set disable_unsafe_spills=true;
is there a similar test w/ SCRATCH_LIMIT set to 0 and maybe another non-zero
value?
http://gerrit.cloudera.org:8080/#/c/7787/1/tests/query_test/test_spilling.py
File tests/query_test/test_spilling.py:
Line 67: self.run_test_case('QueryTest/disable-unsafe-spills', vector)
can this just go in test_spilling? I don't think we'll need to add more test
cases to this file later, so may be worth reducing the number of test files.
--
To view, visit http://gerrit.cloudera.org:8080/7787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I03de00394bb6bbcf381250f816e22a4b987f1135
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Matthew Jacobs
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784: Separate planner and user set query options in profile .. Patch Set 4: Code-Review+1 Great, thanks! -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5602: Fix query optimization for kudu and datasource tables
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5602: Fix query optimization for kudu and datasource
tables
..
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7560/8/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
PS8, Line 200: fail("Failed to add table, external kudu table
expected:\n" + createTableSql);
: }
: t
> This wont work for managed kudu tables because this method requires we only
Right, makes sense. Yeah, so we're already tied to Kudu in planner tests. I
guess this is the downside of not keeping this metadata in the catalog. I agree
then it's not worth trying to address here. I'm fine with this approach if Alex
is as well.
http://gerrit.cloudera.org:8080/#/c/7560/10/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
PS10, Line 170: but no
:* other metadata
This isn't true, for HDFS tables we set the partition map (to the 'default
partition').
It might just be easier and more clear to say:
Add a new dummy table to the catalog based on the given CREATE TABLE sql. The
returned table only has its metadata partially set, but is capable of being
planned.
I'd be curious to hear what Alex thinks.
--
To view, visit http://gerrit.cloudera.org:8080/7560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93822d67ebda41d5d0456095c429e3915a3f40c4
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5602: Fix query optimization for kudu and datasource tables
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5602: Fix query optimization for kudu and datasource
tables
..
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7560/8/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
PS8, Line 200: if (!Table.isExternalTable(msTbl)) {
: fail("Failed to add table, external kudu table
expected:\n" + createTableSql);
: }
My concern with this approach is that this now does depend on Kudu, and I'm not
sure we have to. Is there a reason not to make this work for managed Kudu
tables, and just set the KuduTable.primaryKeyColumnNames_ from the
stmt.getKuduPartitionParams()? Then I think this doesn't require calling into
Kudu. Also it's less restrictive than only supporting Kudu tables that already
exist.
--
To view, visit http://gerrit.cloudera.org:8080/7560
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93822d67ebda41d5d0456095c429e3915a3f40c4
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784 : Separate planner and user set query options in profile .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7721/3//COMMIT_MSG Commit Message: PS3, Line 7: nit: extra space -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784 : Separate planner and user set query options in profile .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: PS3, Line 149: manually > Seems okay. Or slight tweak: lgtm -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784 : Separate planner and user set query options in profile .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: PS3, Line 149: manually > That's a good point. Also, referring to these as "non-default" can be confu Yeah true. How about Query Options (set by configuration) Query Options (including planner set) ? -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal. .. Patch Set 3: Might be nice to have some tests like those added in this patch which make sure the rewritten queries work with nullable tuples (which are a weird wart, arguably): https://gerrit.cloudera.org/#/c/7567/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test -- To view, visit http://gerrit.cloudera.org:8080/7781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e4b8bf6fedd595f9ea54ffb30fc71a058c7f16c Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Philip Zeyliger Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784 : Separate planner and user set query options in profile .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: PS3, Line 149: manually Hm. This word may be misleading, query options may have been set when assigned to an admission control pool, i.e. the per-pool defaults. Also there are server-wide defaults that can be set by the gflag -default_query_options, and those would show up here too. 'manually or automatically set' is more precise, but verbose. Lets see if Dan or Balasz have suggestions. -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5352: Age out unused file handles from the cache
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7640/9/be/src/runtime/disk-io-mgr-handle-cache.inline.h
File be/src/runtime/disk-io-mgr-handle-cache.inline.h:
PS9, Line 190: bool timed_out = true;
: // This Get() will time out until shutdown, when the promise
is set.
: while (timed_out) {
this is weird. normally I'm not in favor of while (true), but in this case I
think this code would be more clear if this was:
while (true) {
...
bool timed_out;
shut_down_promise_.Get(period, &timed_out);
if (!timed_out) break;
}
...
--
To view, visit http://gerrit.cloudera.org:8080/7640
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784 : Separate planner and user set query options in profile .. Patch Set 1: > > > > (1 comment) > > > Relying to Dan and Balasz: > > > > > > It's actually hard to determine what gets set where using the > > > QueryOptions map thing we have right now, mostly because of how > > > inconsistent we are with default values. E.g. what if a > property > > is > > > set in the session, then the planner sets it as well, but > happens > > > to set it to the default value. Right now we can't even > identify > > > that as a non-default query option. > > > > > > > I don't follow this. We're defining "non-default" query option to > > mean everything that doesn't match the static query option value, > > right? > > > > If so, why can't we just compare the query option values after > > planning and to the values before planning, and print any > > differences as "planner set" (or whatever name). And then > compare > > the query options before planning with the static defaults, > remove > > anything we've already printed as "planner set" and print those > > remaining differences as "manually set". > > > > or are you saying that doing that is tricky due to the > > datastructures we currently have? > > Yes, I'm saying the data structures we have right now don't lend > themselves to what you're describing. Its certainly possible, but > it'd be awkward given how it currently works and I think it'd be > kind of a pain to test and make sure we don't break different cases > (e.g. values set in the session/query x modified or not by the > planner x with/without a static default). If you think it's worth > it in terms of usability, then Bikram can keep exploring that path. The code in query-options.cc/h is pretty hairy, e.g. https://github.com/apache/incubator-impala/blob/master/be/src/service/query-options.cc#L82 -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784 : Separate planner and user set query options in profile .. Patch Set 1: > > > (1 comment) > > Relying to Dan and Balasz: > > > > It's actually hard to determine what gets set where using the > > QueryOptions map thing we have right now, mostly because of how > > inconsistent we are with default values. E.g. what if a property > is > > set in the session, then the planner sets it as well, but happens > > to set it to the default value. Right now we can't even identify > > that as a non-default query option. > > > > I don't follow this. We're defining "non-default" query option to > mean everything that doesn't match the static query option value, > right? > > If so, why can't we just compare the query option values after > planning and to the values before planning, and print any > differences as "planner set" (or whatever name). And then compare > the query options before planning with the static defaults, remove > anything we've already printed as "planner set" and print those > remaining differences as "manually set". > > or are you saying that doing that is tricky due to the > datastructures we currently have? Yes, I'm saying the data structures we have right now don't lend themselves to what you're describing. Its certainly possible, but it'd be awkward given how it currently works and I think it'd be kind of a pain to test and make sure we don't break different cases (e.g. values set in the session/query x modified or not by the planner x with/without a static default). If you think it's worth it in terms of usability, then Bikram can keep exploring that path. -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5798: ASAN use-after-poison in Parquet decoder
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5798: ASAN use-after-poison in Parquet decoder .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3f3d0d998f7581c7c935d98fde886f145efd61a8 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5784 : Separate planner and user set query options in profile .. Patch Set 1: > (1 comment) Relying to Dan and Balasz: It's actually hard to determine what gets set where using the QueryOptions map thing we have right now, mostly because of how inconsistent we are with default values. E.g. what if a property is set in the session, then the planner sets it as well, but happens to set it to the default value. Right now we can't even identify that as a non-default query option. Obviously anything is possible with more changes, but we'd probably need to change how the query options map works so that they have a standard scheme for providing default values and are properly support composition and diff'ing, e.g. the planner sets the options = the new map - the old map. Not bad to think about going down that path, but it'll be a bigger change and not for Impala 2.10. I think the safer thing is to print the maps more honestly (i.e. less special interpretation) and settle on some names for these fields that we're happy with. -- To view, visit http://gerrit.cloudera.org:8080/7721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Balazs Jeszenszky Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5799: Kudu DML can crash if schema has changed
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5799: Kudu DML can crash if schema has changed .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7688/3/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: PS3, Line 147: <= > Suppose you have a table with '3' columns. The highest col_idx that would b Yes you're right, I misread it. I think it'll be more obvious if you flip the inequality. -- To view, visit http://gerrit.cloudera.org:8080/7688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9fd6bf164310df0041144f75f5ee722665e9f587 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements .. Patch Set 10: Code-Review+2 String msg changed and an existing test needed to be updated -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Hello Impala Public Jenkins, Dan Hecht, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7678 to look at the new patch set (#10). Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements .. IMPALA-5644,IMPALA-5810: Min reservation improvements Rejects queries during admission control if: * the largest (across all backends) min buffer reservation is greater than the query mem_limit or buffer_pool_limit * the sum of the min buffer reservations across the cluster is larger than the pool max mem resources There are some other interesting cases to consider later: * every per-backend min buffer reservation is less than the associated backend's process mem_limit; the current admission control code doesn't know about other backend's proc mem_limits. Also reduces minimum non-reservation memory (IMPALA-5810). See the JIRA for experimental results that show this slightly improves min memory requirements for small queries. One reason to tweak this is to compensate for the fact that BufferedBlockMgr didn't count small buffers against the BlockMgr limit, but BufferPool counts all buffers against it. Testing: * Adds new test cases in test_admission_controller.py * Adds BE tests in reservation-tracker-test for the reservation-util code. Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b --- M be/src/runtime/bufferpool/CMakeLists.txt M be/src/runtime/bufferpool/reservation-tracker-test.cc A be/src/runtime/bufferpool/reservation-util.cc A be/src/runtime/bufferpool/reservation-util.h M be/src/runtime/query-state.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/scheduler.h M common/thrift/generate_error_codes.py M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test M testdata/workloads/functional-query/queries/QueryTest/spilling-aggs.test M tests/custom_cluster/test_admission_controller.py M tests/query_test/test_mem_usage_scaling.py 13 files changed, 298 insertions(+), 104 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/10 -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5799: Kudu DML can crash if schema has changed
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5799: Kudu DML can crash if schema has changed
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7688/3/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:
PS3, Line 147: <=
'<' ?
PS3, Line 147: if (table_->schema().num_columns() <= col_idx) {
: return Status(strings::Substitute(
: "Table $0 has fewer columns than expected.",
table_desc_->name()));
: }
while the new test is really great new coverage, I think there are some
functional test cases we aren't covering with that test. e.g. I don't think we
could have caught the off-by-one error in the comment on l147. Obviously
getting good coverage of these cases is kinda tricky. I'm not sure yet if
there's some reasonable way to do it. E.g. maybe we can inject a delay between
planning and the BE exec by making it queue in admission control.
--
To view, visit http://gerrit.cloudera.org:8080/7688
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fd6bf164310df0041144f75f5ee722665e9f587
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Hello Dan Hecht, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7678 to look at the new patch set (#9). Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements .. IMPALA-5644,IMPALA-5810: Min reservation improvements Rejects queries during admission control if: * the largest (across all backends) min buffer reservation is greater than the query mem_limit or buffer_pool_limit * the sum of the min buffer reservations across the cluster is larger than the pool max mem resources There are some other interesting cases to consider later: * every per-backend min buffer reservation is less than the associated backend's process mem_limit; the current admission control code doesn't know about other backend's proc mem_limits. Also reduces minimum non-reservation memory (IMPALA-5810). See the JIRA for experimental results that show this slightly improves min memory requirements for small queries. One reason to tweak this is to compensate for the fact that BufferedBlockMgr didn't count small buffers against the BlockMgr limit, but BufferPool counts all buffers against it. Testing: * Adds new test cases in test_admission_controller.py * Adds BE tests in reservation-tracker-test for the reservation-util code. Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b --- M be/src/runtime/bufferpool/CMakeLists.txt M be/src/runtime/bufferpool/reservation-tracker-test.cc A be/src/runtime/bufferpool/reservation-util.cc A be/src/runtime/bufferpool/reservation-util.h M be/src/runtime/query-state.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/scheduler.h M common/thrift/generate_error_codes.py M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test M testdata/workloads/functional-query/queries/QueryTest/spilling-aggs.test M tests/custom_cluster/test_admission_controller.py M tests/query_test/test_mem_usage_scaling.py 13 files changed, 297 insertions(+), 103 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/9 -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/7678/8/be/src/runtime/bufferpool/reservation-util.h File be/src/runtime/bufferpool/reservation-util.h: PS8, Line 35: . > That's much clearer, but I think it would help to clearly say these limits Done -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] Bump Kudu version to 44a820b
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/7767 Change subject: Bump Kudu version to 44a820b .. Bump Kudu version to 44a820b Change-Id: I1e86ce13d1fdc73487b8067f3670ee73b9269366 --- M bin/impala-config.sh 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7767/1 -- To view, visit http://gerrit.cloudera.org:8080/7767 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1e86ce13d1fdc73487b8067f3670ee73b9269366 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
[native-toolchain-CR] Bump Kudu version to 44a820b
Matthew Jacobs has posted comments on this change. Change subject: Bump Kudu version to 44a820b .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/7755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I599843a7c230ec75699b9f236f9f0a97949278b9 Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-HasComments: No
[native-toolchain-CR] Bump Kudu version to 44a820b
Matthew Jacobs has submitted this change and it was merged. Change subject: Bump Kudu version to 44a820b .. Bump Kudu version to 44a820b Change-Id: I599843a7c230ec75699b9f236f9f0a97949278b9 --- M buildall.sh 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Matthew Jacobs: Verified Thomas Tauber-Marshall: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I599843a7c230ec75699b9f236f9f0a97949278b9 Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall
[native-toolchain-CR] Bump Kudu version to 44a820b
Matthew Jacobs has uploaded a new change for review. http://gerrit.cloudera.org:8080/7755 Change subject: Bump Kudu version to 44a820b .. Bump Kudu version to 44a820b Change-Id: I599843a7c230ec75699b9f236f9f0a97949278b9 --- M buildall.sh 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/55/7755/1 -- To view, visit http://gerrit.cloudera.org:8080/7755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I599843a7c230ec75699b9f236f9f0a97949278b9 Gerrit-PatchSet: 1 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs
[Impala-ASF-CR] IMPALA-5158,IMPALA-5236: account for unused buffer pool reservations
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5158,IMPALA-5236: account for unused buffer pool reservations .. Patch Set 8: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/7380/8/be/src/util/metrics.h File be/src/util/metrics.h: PS8, Line 249: /// The metrics to be summed. metric to be negated PS8, Line 391: MakeTMetricDef not your change, but this could use a comment. Would you mind adding one while you're here? -- To view, visit http://gerrit.cloudera.org:8080/7380 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idb1fa3110dc893321f9f4e8ced6b7ede12194dad Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements .. Patch Set 8: looks like test_spilling.py will still require a lot of changes -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5799: Kudu DML can crash if schema has changed
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5799: Kudu DML can crash if schema has changed
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7688/1//COMMIT_MSG
Commit Message:
PS1, Line 14: its
> Done
my bad, I meant for this comment to be on l10 which you changed
http://gerrit.cloudera.org:8080/#/c/7688/2//COMMIT_MSG
Commit Message:
PS2, Line 11: '
this one should be removed
http://gerrit.cloudera.org:8080/#/c/7688/2/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:
PS2, Line 146: udu_table_sink_.referenced_columns.back()
wouldn't this not work when the insert specifies all columns, i.e.
referenced_cols is empty() ?
PS2, Line 146: if (table_->schema().num_columns() <=
kudu_table_sink_.referenced_columns.back()) {
: return Status(strings::Substitute(
: "Table $0 has fewer columns than expected.",
table_desc_->name()));
: }
this makes me a bit nervous because it seems like expecting the list is in
ascending order of col position an implementation detail that could
change/break, and I don't think we really rely on it elsewhere or test it
explicitly. I think you can just skip this check here, and instead just check
right before l153 that table_->schema().num_columns() > col. We don't have to
worry about doing a bit more work since this only happens on Open()
PS2, Line 151: col
nit: col_idx maybe
http://gerrit.cloudera.org:8080/#/c/7688/2/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:
Line 412: time.sleep(random.random())
comment that this sleeps for up to a second
PS2, Line 414: client = self.create_impala_client()
nit: move this outside of the loop
--
To view, visit http://gerrit.cloudera.org:8080/7688
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9fd6bf164310df0041144f75f5ee722665e9f587
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall
Gerrit-Reviewer: Matthew Jacobs
Gerrit-Reviewer: Thomas Tauber-Marshall
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7640/3/be/src/runtime/disk-io-mgr-handle-cache.h File be/src/runtime/disk-io-mgr-handle-cache.h: PS3, Line 137: : /// in_use is true for a file handle checked out via GetFileHandle() that has not : /// been returned via ReleaseFileHandle(). : bool in_use = false; : Is this necessary? I think the compiler should generate a move constructor for this struct. http://gerrit.cloudera.org:8080/#/c/7640/2/be/src/runtime/disk-io-mgr-handle-cache.inline.h File be/src/runtime/disk-io-mgr-handle-cache.inline.h: PS2, Line 196: FileHandleCache::FileHandleCachePartition& p > I added detailed comments about the lru list manipulations, but I don't thi wfm, I was thinking out loud but didn't feel strongly about making a big change. Thanks for adding some more comments. -- To view, visit http://gerrit.cloudera.org:8080/7640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5504: Fix TupleIsNullPredicate evaluation.
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5504: Fix TupleIsNullPredicate evaluation. .. Patch Set 1: Code-Review+2 Nice catch -- To view, visit http://gerrit.cloudera.org:8080/7737 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id659f849a68d88cfe22c65dd1747dd6d6a916163 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5644,IMPALA-5810: Min reservation improvements
Hello Dan Hecht, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7678 to look at the new patch set (#8). Change subject: IMPALA-5644,IMPALA-5810: Min reservation improvements .. IMPALA-5644,IMPALA-5810: Min reservation improvements Rejects queries during admission control if: * the largest (across all backends) min buffer reservation is greater than the query mem_limit or buffer_pool_limit * the sum of the min buffer reservations across the cluster is larger than the pool max mem resources There are some other interesting cases to consider later: * every per-backend min buffer reservation is less than the associated backend's process mem_limit; the current admission control code doesn't know about other backend's proc mem_limits. Also reduces minimum non-reservation memory (IMPALA-5810). See the JIRA for experimental results that show this slightly improves min memory requirements for small queries. One reason to tweak this is to compensate for the fact that BufferedBlockMgr didn't count small buffers against the BlockMgr limit, but BufferPool counts all buffers against it. Testing: * Adds new test cases in test_admission_controller.py * Adds BE tests in reservation-tracker-test for the reservation-util code. Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b --- M be/src/runtime/bufferpool/CMakeLists.txt M be/src/runtime/bufferpool/reservation-tracker-test.cc A be/src/runtime/bufferpool/reservation-util.cc A be/src/runtime/bufferpool/reservation-util.h M be/src/runtime/query-state.cc M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/scheduler.h M common/thrift/generate_error_codes.py M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test M tests/custom_cluster/test_admission_controller.py M tests/query_test/test_mem_usage_scaling.py 12 files changed, 296 insertions(+), 102 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7678/8 -- To view, visit http://gerrit.cloudera.org:8080/7678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iabe87ce8f460356cfe4d1be4d7092c5900f9d79b Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Tim Armstrong
