Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4856: Port ImpalaInternalService to KRPC ......................................................................
Patch Set 3: (14 comments) Had a first look. http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/runtime/data-stream-mgr.cc File be/src/runtime/data-stream-mgr.cc: PS3, Line 327: VLOG_QUERY << "DataStreamMgr maintenance tasks complete. Took: " : << PrettyPrinter::Print(MonotonicMillis() - start, TUnit::TIME_MS); Do you think it's worth printing what maintenance tasks were done in this iteration? And maybe don't print anything if no work was done. Might help with debugging. Feel free to ignore if you disagree. Also, wondering if VLOG_QUERY is the right log level to print this. Wouldn't this cause a lot of spam? http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/runtime/data-stream-mgr.h File be/src/runtime/data-stream-mgr.h: PS3, Line 135: idea I would rather use the word 'assumption' here. PS3, Line 212: TRANSMIT_DATA_TIMEOUT_SECONDS I think there needs to be some comment clearly distinguishing between this timeout and FLAGS_datastream_sender_timeout_ms. http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/runtime/data-stream-recvr.cc File be/src/runtime/data-stream-recvr.cc: PS3, Line 129: pending_senders_ Maybe this could be future work, but I foresee a need to cap this at some number (which could be a function of the number of nodes) after which it would be beneficial to just fail the query. PS3, Line 137: SpinLock Isn't this a rather large amount of work to have under a spinlock? Also, we would expect this lock to be more than fairly contended. PS3, Line 191: // num_remaining_senders_ could be 0 because an AddBatch() can arrive *after* a : // EndDataStream() RPC for the same sender, due to asynchrony on the sender side (the : // sender gets closed or cancelled, but doesn't wait for the oustanding TransmitData() : // to complete before trying to close the channel). If this is the only case where num_remaining_senders_ can be 0, then is there any point in responding at all? http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/runtime/data-stream-recvr.h File be/src/runtime/data-stream-recvr.h: PS3, Line 174: good_bytes_received_counter_ bytes_accepted_counter_ ? http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/service/impala-internal-service.cc File be/src/service/impala-internal-service.cc: Line 80: ExecEnv::GetInstance()->stream_mgr()->AddData(finst_id, move(payload)); This is a tad bit confusing. Why is there not a necessity to have a RespondSuccess() for this case? I do see that there is a payload.context->RespondSuccess() inside AddData(), but that's only on the error case. Also, ideally, for the sake of consistency, I would prefer that the error status be returned here and then see that the RPC is responded to from here. Unless you see a good reason for it not to be that way. PS3, Line 99: // TODO: Check return What he said. Line 118: context->GetInboundSidecar(filter.header.directory_sidecar_idx(), &filter.directory); Same here, check return status. Line 155: context->RespondSuccess(); No need to return status.SetTStatus(&return_val) ? I see that we check it in ReportStatusCb(). This changes behavior a bit. It looks like currently, if a ReportExecStatus() RPC fails, we fail the query. That won't happen if the sidecar deserialization fails. http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS3, Line 1925: move Include what you use? <utility> http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/service/impala-server.h File be/src/service/impala-server.h: PS3, Line 123: Shutdown Who calls this? http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: PS3, Line 79: exec_env->Init(); Need to check returned status and fail if necessary. -- To view, visit http://gerrit.cloudera.org:8080/5888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95229290566a8ccffd80ed2d74c1c57cf1479238 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Anonymous Coward #168 Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
