Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10873 )
Change subject: IMPALA-6031: Distributed plan describes coordinator-only nodes as scanning ...................................................................... Patch Set 2: (11 comments) lgtm, just a few nits http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG@7 PS2, Line 7: Distributed plan describes coordinator-only nodes as scanning the title should usually describe what this patch does/fixed http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG@9 PS2, Line 9: The MembershipCallback function of ImpalaServer class was modified : to only send details of executor nodes to the frontend. Thus, while : generating scan plans, the frontend would not assign fragments to : coordinator-only nodes. The MembershipSnapshot class was renamed to : ExecutorMembershipSnapshot for better semantics. you dont need to describe all the changes in the commit message. Just describing the problem, its high level solution, things like that should be good enough. On the other hand you can make a call on weather some specific change to the code needs to be highlighted in the commit message. http://gerrit.cloudera.org:8080/#/c/10873/2//COMMIT_MSG@16 PS2, Line 16: Added a new custom_cluster test where one impalad is a : coordinator-only node. The test verifies that the scan fragments : are assigned only to the executors. again, you dont need to be verbose while explaining this. You can just write something like: "Added a new custom cluster test to verify the same" http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/frontend.h@48 PS2, Line 48: cluster membership snapshot nit: cluster membership snapshot of executors http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc@1674 PS2, Line 1674: // Send the hostname and ip_address to the frontend only for executor nodes. nit: can probably incorporate this into the comment on L1669 http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc@1678 PS2, Line 1678: Increment the number of executor nodes. nit: superfluous comment. http://gerrit.cloudera.org:8080/#/c/10873/2/be/src/service/impala-server.cc@1678 PS2, Line 1678: We need to track the number of : // executors because there may be multiple impalads running on a single host. we can probably move this comment to the thrift definition of TUpdateMembershipRequest in Frontend.thrift http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift@714 PS2, Line 714: // Sent from the impalad BE to FE with the latest cluster membership snapshot resulting : // from the Membership heartbeat. update comment to mention executor membership is only updated. http://gerrit.cloudera.org:8080/#/c/10873/2/common/thrift/Frontend.thrift@716 PS2, Line 716: TUpdateMembershipRequest would be a pretty big name but just to be consistent, how about we rename this to TUpdateExecutorMembershipRequest http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1153 PS2, Line 1153: cluster nit: executorNodes http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java File fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java: http://gerrit.cloudera.org:8080/#/c/10873/2/fe/src/main/java/org/apache/impala/util/ExecutorMembershipSnapshot.java@28 PS2, Line 28: * Singleton class that represents a snapshot of the Impalad cluster membership. Host update comment to mention cluster membership of only executor nodes -- To view, visit http://gerrit.cloudera.org:8080/10873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44af6b40099a495e13a0a5dc72c491d486d23aa8 Gerrit-Change-Number: 10873 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 06 Jul 2018 00:33:52 +0000 Gerrit-HasComments: Yes
