Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14777 )
Change subject: 1;10;0cIMPALA-9181: Serialize TQueryCtx once per query ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/14777/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14777/1//COMMIT_MSG@10 PS1, Line 10: TQueryCtx > why is serializing this per fragment expensive, is it typically a very larg Done http://gerrit.cloudera.org:8080/#/c/14777/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/14777/1/be/src/runtime/coordinator-backend-state.h@85 PS1, Line 85: const kudu::Slice& serialized_query_ctx > nit: pass via constructor instead? since that is where query_ctx is passed That makes the memory management more complicated, since we don't want the serialized_query_ctx memory to be used except for when Exec() is running. The only thing we use the query_ctx from the constructor for is to get the TQueryOptions, so we could just pass that in instead of the TQueryCtx if you think that makes it clearer what's going on. http://gerrit.cloudera.org:8080/#/c/14777/1/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: http://gerrit.cloudera.org:8080/#/c/14777/1/be/src/runtime/query-exec-mgr.cc@50 PS1, Line 50: bool dummy; : QueryState* qs = : GetOrCreateQueryState(query_ctx, request->per_backend_mem_limit(), &dummy); > not your code, but is it possible for the QueryState to already exist, or c Yes, its possible for it to already exist if this is the coordinator, as the coordinator calls CreateQueryState() in Coordinator::Exec(). http://gerrit.cloudera.org:8080/#/c/14777/1/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/14777/1/be/src/service/control-service.cc@118 PS1, Line 118: // 'thrift_obj'. > nit: add some method code comments Done http://gerrit.cloudera.org:8080/#/c/14777/1/be/src/service/control-service.cc@135 PS1, Line 135: e sidecars. The QueryState will mak > probably could do this if you make query_ctx a unique_ptr and just transfer Yeah, I think I would prefer to leave this as a followup task rather than making this patch more complicated. http://gerrit.cloudera.org:8080/#/c/14777/1/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/14777/1/common/thrift/ImpalaInternalService.thrift@658 PS1, Line 658: er than fully coverti > could you add some more docs to this? it's not that clear to me what this e Done -- To view, visit http://gerrit.cloudera.org:8080/14777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6a4dd302fd5602ec2775492a041ddd51e7d7a6c6 Gerrit-Change-Number: 14777 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 26 Nov 2019 22:00:00 +0000 Gerrit-HasComments: Yes
