Michael Ho has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS3, Line 39: Kudu RPC > that seems confusing since it has nothing to do with Kudu from the users' p Done http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/backend-config.h File be/src/scheduling/backend-config.h: PS3, Line 60: hostname > if it's a host name, why is the type TNetworkAddress as opposed to Hostname This is needed to compare the port too. Apparently, each hostname maps to a list of backend descriptor. We return the first descriptor with matching IP and port. http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: PS3, Line 95: > do we have a check anywhere that this flag is set, and give an error messag It's set to default value 29000 if the user doesn't specify it during startup. We can potentially do a check for the validity of its value but I don't see the need to make an exception for it if we aren't already testing the values of other ports or whether the port is already in use. Line 241: ADD_TIMER(schedule->summary_profile(), "ComputeScanRangeAssignmentTimer"); > isn't this dcheck really saying that if LookupBackendDesc() returned nullpt Done. Note that in release builds, we will just return the wrong descriptor. Is it better to crash or should we trust our testing to have covered it ? I suppose similar questions can be asked about other DCHECKs in the existing code base. Line 711: #endif > I don't understand that. isn't this just copying the shared_ptr, not the Ba The way the update happens is that it overwrites the shared_ptr "executors_config_" atomically to point to the new copy. So, if you call GetExecutorsConfig() twice, you can potentially get back two different pointers. Checking out a copy here (with shared_ptr) makes sure that we retain reference to the old copy even after "executors_config_" has been overwritten. http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 410: // IP address + port of the KRPC based services on the destination > does this have to be resolved IP as well? Done http://gerrit.cloudera.org:8080/#/c/7760/3/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: Line 72: // IP address + port of KRPC based services on this backend > this one has to be a resolved IP, right? let's make that clear. Done -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
