impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4856: Include KRPC services in plan fragment's destinations
Date Tue, 29 Aug 2017 19:53:01 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations

Patch Set 4:

File be/src/common/

PS4, Line 39: Kudu
still says Kudu. Isn't that confusing since it has nothing to do with kudu (other than being
derived from it)? Maybe just call it "KRPC".

Or should we make this flag hidden so users don't find it until the feature is finished?
File be/src/scheduling/

Line 72:   // doesn't have to resolve it on every heartbeat.
would help to update that comment to include KRPC needing resolved IP.

PS4, Line 231: DCHECK
File common/thrift/ImpalaInternalService.thrift:

Line 353:   // Initiating coordinator.
would be nice to indicate this is the thrift address.

Line 407:   // ... which is being executed on this server
is that the thrift address? would be nice to comment that.
File common/thrift/StatestoreService.thrift:

Line 53:   // Network address of the Impala service on this backend
specify that's the thrift address

PS4, Line 73: svc_
maybe just call it krpc_address for consistency e.g. address/krpc_address and server/krpc_server
naming. the 'address' field is also that of a "service". okay to ignore if you feel this naming
is better.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message