Henry Robinson has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations ......................................................................
Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0); > I am actually quite confused as to the guideline for using FLAGS in ExeEnv. I don't think there's a hard and fast rule. In general I think it's not a bad idea to make configuration dependencies explicit, particularly in the case of ports where there's the possibility of conflict with the rest of the system. There's obviously a balance between making the c'tor too unwieldy and allowing some flexibility. Here, I'd just ask that we treat the krpc_port the same as all other ports. Consistency is important. -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
