Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15154 )
Change subject: IMPALA-8712: Make ExecQueryFInstances async ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/15154/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15154/2//COMMIT_MSG@23 PS2, Line 23: Removing this thread pool has potential performance implications, as : it means that the Exec() parameters are serialized in serialize rather : than in parallel (with the level of parallelism determined by the size : of the thread pool, which was configurable by an Advanced flag and : defaulted to 12). > Yes, we record the amount of time query startup takes, the time between whe right, a more fine grained counter around all the calls to the ThriftSerializer might be nice though, especially since this has bitten us in the past: IMPALA-8732 there is also a lot of stuff that happens between those two timeline events - e.g. issuing all the Exec RPCs and waiting for them to complete http://gerrit.cloudera.org:8080/#/c/15154/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15154/3//COMMIT_MSG@46 PS3, Line 46: TODO: once IMPALA-9335 (krpc rebase) goes in, the change in : be/src/kudu/rpc/connection.cc can be removed from this patch. this can be rebased now, right? http://gerrit.cloudera.org:8080/#/c/15154/3/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/15154/3/be/src/runtime/coordinator-backend-state.h@406 PS3, Line 406: " nit: single quote http://gerrit.cloudera.org:8080/#/c/15154/3/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/15154/3/be/src/runtime/coordinator-backend-state.cc@310 PS3, Line 310: done nit: better name might be 'error' since this is only invoked on error http://gerrit.cloudera.org:8080/#/c/15154/3/be/src/runtime/coordinator-backend-state.cc@549 PS3, Line 549: exec_done_cv_.NotifyAll(); can this be re-factored as well so that notify is called after releasing the lock? http://gerrit.cloudera.org:8080/#/c/15154/3/be/src/runtime/coordinator-backend-state.cc@900 PS3, Line 900: For nit: the 'For' seems extraneous and doesn't always make sense in the context of the usages of this method http://gerrit.cloudera.org:8080/#/c/15154/3/be/src/runtime/coordinator-backend-state.cc@901 PS3, Line 901: backend nit: maybe 'target backend' to make it clear that the Coordinator is attempting to make a call to a remote backend? http://gerrit.cloudera.org:8080/#/c/15154/3/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/15154/3/tests/custom_cluster/test_rpc_exception.py@28 PS3, Line 28: def get_rpc_debug_action(rpc, action, port=KRPC_PORT): : """Returns a debug action that causes rpcs with the name 'rpc' that are sent to the : impalad at 'port' to execute the debug aciton 'action'.""" : return "IMPALA_SERVICE_POOL:127.0.0.1:{port}:{rpc}:{action}" \ : .format(rpc=rpc, port=port, action=action) : : : def get_fail_action(rpc, error=None, port=KRPC_PORT, p=0.1): : """Returns a debug action that causes rpcs with the name 'rpc' that are sent to the : impalad at 'port' to FAIL with probability 'p' and return 'error' if specified.""" : action = "FAIL@%s" % p : if error is not None: : action += "@" + error : return get_rpc_debug_action(rpc, action, port=port) do we expect to use these methods outside of the TestRPCException class? -- To view, visit http://gerrit.cloudera.org:8080/15154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I33ec96e5885af094c294cd3a76c242995263ba32 Gerrit-Change-Number: 15154 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Tue, 11 Feb 2020 17:31:51 +0000 Gerrit-HasComments: Yes
