Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12142 )
Change subject: IMPALA-7468: Port CancelQueryFInstances() to KRPC. ...................................................................... Patch Set 1: (6 comments) Thanks for the helpful review http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/runtime/coordinator-backend-state.h@294 PS1, Line 294: Status DoCancelQueryFInstancesRrpcWithRetry( > Nit: The prevailing style seems to place reference ('&') and pointer ('*') Up to now I have just been using what the clang-format rules dictate. In clang-format terms I think you are saying we should use 'DerivePointerAlignment: true' (which means: "analyze the formatted file for the most common alignment of & and *. Pointer and reference alignment styles are going to be updated according to the preferences found in the file. PointerAlignment is then used only as fallback"), is that the standard practice for Impala? http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/query-schedule.h@104 PS1, Line 104: const TNetworkAddress& krpc_host, int per_fragment_instance_idx, > Nit: I wonder if there are trailing whitespaces in these lines? I don't und There is no trailing whitespace I could find. I think this formatting is the result of the clang-format parameter: ConstructorInitializerAllOnOneLineOrOnePerLine: true which is splitting the initializers to one-per-line. http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h File be/src/scheduling/scheduler.h: http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.h@433 PS1, Line 433: void ComputeFragmentExecParams(const BackendConfig executor_config, > Nit: Please retain existing placement of '&' and '*'. Thanks for catching my not using a reference for executor_config. http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/12142/1/be/src/scheduling/scheduler.cc@333 PS1, Line 333: ComputeFragmentExecParams(executor_config, plan_exec_info, > Nit: Trailing spaces? Yes there is something wrong here and I have reformatted http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@163 PS1, Line 163: optional UniqueIdPB query_id = 1; > Don't understand how the query id can be optional... OK you made me think! I had just copied what other people had done in implementing krpc. But now I have investigated further and found that many people argue that having required fields is a bad idea, and the syntax is even removed in future protobuf implementations, see for example https://stackoverflow.com/questions/31801257/why-required-and-optional-is-removed-in-protocol-buffers-3 Overall this makes sense to me, so I think optional is OK here, are you persuaded? http://gerrit.cloudera.org:8080/#/c/12142/1/common/protobuf/control_service.proto@179 PS1, Line 179: // Cancellation is asynchronous. > Commit message says cancellation is synchronous. Do you want to change this When the coordinator cancels a query it sends CancelQueryFInstancesRequestPB messages to the hosts executing fragment instances for the query. These calls are synchronous, that is the coordinator waits for a response to each CancelQueryFInstances call before making the next one. This is what the commit message means by 'For now keep the KRPC calls to CancelQueryFInstances() as synchronous'. When a cancellation occurs on an impalad executing a fragment instance then the cancellation is implemented by setting flags saying the query is cancelled in various data structures, and by waking various sleeping threads. This should be enough to stop the query executing, but the CancelQueryFInstances call returns without doing any checking that all query execution has stopped. So in this sense cancel is asynchronous. I updated the comment to make this clearer. -- To view, visit http://gerrit.cloudera.org:8080/12142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I625030c3f1068061aa029e6e242f016cadd84969 Gerrit-Change-Number: 12142 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Thu, 03 Jan 2019 00:33:30 +0000 Gerrit-HasComments: Yes
