Daniel Becker 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 7: (21 comments) I'm not familiar with this part of the code base but I've left some comments. Note that I haven't looked at the test files, only the others. 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 the three configs mentioned above. This interface is Does this mean that admission control can admit queries that have been rejected so far? If so, we could include it in the commit message. http://gerrit.cloudera.org:8080/#/c/21762/7//COMMIT_MSG@36 PS7, Line 36: however, is rare and can be considered as misconfiguration. What happens in case of misconfiguration? Is there a way to print a warning message? http://gerrit.cloudera.org:8080/#/c/21762/7//COMMIT_MSG@38 PS7, Line 38: spill anyway Can it lead to a situation where before this patch a query would be rejected by admission control but with this patch it is admitted and fails with running out of scratch space? I don't know if that is problematic in itself, it may be acceptable. 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: operator that has Nit: wouldn't "operators that have" be clearer, as there may be multiple ones? 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? http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@699 PS7, Line 699: public long getMaxMemLimitPerHost(boolean isCoordinatorFragment) { Could you add an explanation as a function (doc) comment? http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@711 PS7, Line 711: executors Shouldn't this be "coordinators"? 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_COORDINATORS is not set, we can still get into this branch if this is a coordinator fragment. 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.apache.impala.util.MathUtil; Unused import. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@615 PS7, Line 615: totalResource = ResourceProfile.noReservation(0); This line is not needed, this variable is already initialised on L611. 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.apache.impala.util.MathUtil; Unused import. 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: implement Nit: "implements". http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java@163 PS7, Line 163: l Typo: "Spillable". http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java@164 PS7, Line 164: resourceProfile_ = ((HashJoinNode) joinNode_) HashJoinNode also has the inherited one-argument overload of computeJoinResourceProfile(). That overload delegates to the two-argument overload with exactly Long.MAX_VALUE as the second argument, so we don't need to separate out HashJoinNode here. 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: public static boolean biasToSpill(TQueryOptions queryOptions) { Could you add a comment? Also, maybe "shouldComputeResourcesWithSpill()" or something similar would be more understandable. 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? 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: maxMemoryOnScaling_ The other fields include "Bytes" in their names, this variable could also do that. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java@98 PS7, Line 98: scaleBasedMemEstimate At this point scaleBasedMemEstimate == minMemReservationBytes_, but I think the formula is easier to understand if 'minMemReservationBytes_' is used instead. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/ResourceProfileBuilder.java@113 PS7, Line 113: minMemReservationBytes_ This should be 'memEstimateBytes_', without "min...". 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.service.BackendConfig; Unused import. http://gerrit.cloudera.org:8080/#/c/21762/7/fe/src/main/java/org/apache/impala/planner/SpillableOperator.java@34 PS7, Line 34: option Nit: "options". -- 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: 7 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, 01 Oct 2024 15:00:25 +0000 Gerrit-HasComments: Yes
