Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8023 )
Change subject: IMPALA-4856: Port data stream service to KRPC ...................................................................... Patch Set 3: (17 comments) Some initial comments. http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/common/status.cc File be/src/common/status.cc: http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/common/status.cc@246 PS3, Line 246: } this is the same as FromThrift() effectively, right? Can we make the two look the same to make that obvious? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/common/status.cc@262 PS3, Line 262: same comment. let's make this and ToThrift look the same so it's obvious they do the same things. nit: also, could we order the functions consistently? We currently have ToThrift, FromThrift, FromProto, ToProto, and that ordering just makes it slightly slower to read through since it doesn't follow a pattern. http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/exec/exchange-node.cc File be/src/exec/exchange-node.cc: http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/exec/exchange-node.cc@109 PS3, Line 109: RETURN_IF_CANCELLED(state); why do we do this in some Open() but not all? Should we just do it in ExecNode::Open() and remove the ones in the derived classes? okay to do separately from this patch. http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/rpc/rpc-mgr.h@154 PS3, Line 154: } nit: one-liner? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h File be/src/runtime/krpc-data-stream-mgr.h: http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@94 PS3, Line 94: buffer what buffer? do you mean queue? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@108 PS3, Line 108: During ordinary operation what does that mean? Is it saying that during unordinary operation, a sender can have both a TransmitData() and EndDataStream() call in-flight simultaneously? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@140 PS3, Line 140: it will quietly drop its what are "it" and "its" here? "the sender" and "the RPC's"? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@141 PS3, Line 141: /// it returns. is that still true now that we have cancellation of RPCs? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@153 PS3, Line 153: fragment sending fragment? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@166 PS3, Line 166: request is that talking about the 'request' field below, or something different? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@175 PS3, Line 175: const TransmitDataRequestPB* request; what's the relationship between this and proto_batch? http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@178 PS3, Line 178: /// responded to. who owns it? http://gerrit.cloudera.org:8080/#/c/8023/3/common/protobuf/data_stream_service.proto File common/protobuf/data_stream_service.proto: http://gerrit.cloudera.org:8080/#/c/8023/3/common/protobuf/data_stream_service.proto@29 PS3, Line 29: optional int32 sender_id = 2; : : optional int32 dest_node_id = 3; what are "IDs" in these cases? let's improve the documentation here. Especially since type is no longer PlanNodeId (and why is that?). http://gerrit.cloudera.org:8080/#/c/8023/3/common/protobuf/row_batch.proto File common/protobuf/row_batch.proto: http://gerrit.cloudera.org:8080/#/c/8023/3/common/protobuf/row_batch.proto@30 PS3, Line 30: int32 in thrift we had TTupleId. Is there a reason we aren't defining those types as well to make the structure clearer? http://gerrit.cloudera.org:8080/#/c/8023/3/common/protobuf/row_batch.proto@32 PS3, Line 32: tuple_data what's tuple_data? not a field in this structure... http://gerrit.cloudera.org:8080/#/c/8023/3/common/protobuf/row_batch.proto@39 PS3, Line 39: Size size of what? http://gerrit.cloudera.org:8080/#/c/8023/3/common/protobuf/row_batch.proto@42 PS3, Line 42: (TODO(KRPC): native enum) do we plan to fix that? -- To view, visit http://gerrit.cloudera.org:8080/8023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic0b8c1e50678da66ab1547d16530f88b323ed8c1 Gerrit-Change-Number: 8023 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Tue, 10 Oct 2017 20:15:20 +0000 Gerrit-HasComments: Yes
