Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14944 )
Change subject: IMPALA-4224: part 1: schedule join builds ...................................................................... Patch Set 2: Code-Review+2 (4 comments) Looks good, just a few nits about comments. Also, thanks for expanding on the old comments inside the scheduler, really helped http://gerrit.cloudera.org:8080/#/c/14944/2/be/src/scheduling/scheduler.h File be/src/scheduling/scheduler.h: http://gerrit.cloudera.org:8080/#/c/14944/2/be/src/scheduling/scheduler.h@403 PS2, Line 403: so those instances, which will be in a previous TPlanExecInfo, : /// must have already been created by the scheduler. Did you mean that "this expects that the instances that consume the join build to have already been created by the scheduler." http://gerrit.cloudera.org:8080/#/c/14944/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/14944/2/common/thrift/ImpalaInternalService.thrift@618 PS2, Line 618: in this fragment nit: that will consume the output of this join build http://gerrit.cloudera.org:8080/#/c/14944/2/common/thrift/ImpalaInternalService.thrift@621 PS2, Line 621: // Fragment instance id of the fragment instance nit: input fragment instance http://gerrit.cloudera.org:8080/#/c/14944/2/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/14944/2/fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java@41 PS2, Line 41: private final JoinNode joinNode_; nit: maybe add a small one line comment like we have for joinTableId_ -- To view, visit http://gerrit.cloudera.org:8080/14944 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I779463cfa2ea9b372607d2be6d5d2252a6469e34 Gerrit-Change-Number: 14944 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 17 Jan 2020 02:23:00 +0000 Gerrit-HasComments: Yes
