Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21762 )
Change subject: IMPALA-13333: Limit memory estimation if PlanNode can spill ...................................................................... Patch Set 10: (21 comments) http://gerrit.cloudera.org:8080/#/c/21762/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21762/7//COMMIT_MSG@22 PS7, Line 22: reasoned about from configs mentioned above. This interface is applied > Does this mean that admission control can admit queries that have been reje Added one sentence below to point that out. http://gerrit.cloudera.org:8080/#/c/21762/7//COMMIT_MSG@36 PS7, Line 36: There are some caveats on this memory bounding patch: > What happens in case of misconfiguration? Is there a way to print a warning To my knowledge, per-executor scratch config is not reported back to coordinator not statestore. So there is no single place to inspect it nor put warning logs. http://gerrit.cloudera.org:8080/#/c/21762/7//COMMIT_MSG@38 PS7, Line 38: Mismatch of > Can it lead to a situation where before this patch a query would be rejecte Explained in one sentence at previous paragraph. http://gerrit.cloudera.org:8080/#/c/21762/7/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/21762/7/common/thrift/ImpalaService.thrift@972 PS7, Line 972: > Nit: wouldn't "operators that have" be clearer, as there may be multiple on Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@606 PS7, Line 606: Maximum memory limit > Could include the unit: is it in bytes? Or megabytes? it is in bytes. Added comment. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@699 PS7, Line 699: /** > Could you add an explanation as a function (doc) comment? Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@711 PS7, Line 711: ); > Shouldn't this be "coordinators"? Good catch. Thanks! http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@712 PS7, Line 712: ( > Shouldn't we include the condition "!isCoordinatorFragment"? If MEM_LIMIT_C Yes, fixed it. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@48 PS7, Line 48: import org.slf4j.Logger; > Unused import. Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@615 PS7, Line 615: totalResource = totalResource.sum(aggProfile); > This line is not needed, this variable is already initialised on L611. Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@40 PS7, Line 40: import org.slf4j.Logger; > Unused import. Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java File fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java@163 PS7, Line 163: ceProfile > Nit: "implements". Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java@163 PS7, Line 163: e > Typo: "Spillable". Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java@164 PS7, Line 164: resourceProfile_ = joinNode_.computeJoinResourceProfile(queryOptions).second; > HashJoinNode also has the inherited one-argument overload of computeJoinRes This is a mistake. It should be implementation of computeResourceProfileIfSpill from SpillableOperator. Fixed it. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1363 PS7, Line 1363: // Include the build-side of a RuntimeFilter; look past the 1st ExchangeNode. > Could you add a comment? Also, maybe "shouldComputeResourcesWithSpill()" or Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java File fe/src/main/java/org/apache/impala/planner/ResourceProfile.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java@36 PS7, Line 36: however, not guaranteed > Would it be difficult or undesirable to guarantee it in the constructor? The main problem exist in sum(), max(), combine(), multiply() method in this java file. maxMemReservationBytes_ is a new addition, and there is a difference between having it set vs not, and its relationship against memEstimateBytes_ are not validated by in constructor. I try adding new Precondition to validate that but failed, presumably because at some point, the aggregated memEstimateBytes_ exceed aggregated maxMemReservationBytes_. I can take another look in separate patch later. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java File fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java@46 PS7, Line 46: maxMemEstimateBytes > The other fields include "Bytes" in their names, this variable could also d Renamed to maxMemEstimateBytesAfterScaling_. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java@98 PS7, Line 98: minMemReservationByte > At this point scaleBasedMemEstimate == minMemReservationBytes_, but I think Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java@113 PS7, Line 113: onditions.checkState(mi > This should be 'memEstimateBytes_', without "min...". Another good catch. Thanks! http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/SpillableOperator.java File fe/src/main/java/org/apache/impala/planner/SpillableOperator.java: http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/SpillableOperator.java@20 PS7, Line 20: import org.apache.impala.thrift.TQueryOptions; > Unused import. Done http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/SpillableOperator.java@34 PS7, Line 34: tance > Nit: "options". Done -- To view, visit http://gerrit.cloudera.org:8080/21762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I290c4e889d4ab9e921e356f0f55a9c8b11d0854e Gerrit-Change-Number: 21762 Gerrit-PatchSet: 10 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 15 Oct 2024 23:26:52 +0000 Gerrit-HasComments: Yes
