Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14824 )
Change subject: IMPALA-9199: Add support for single query retries on cluster membership changes ...................................................................... Patch Set 28: Code-Review+2 (5 comments) http://gerrit.cloudera.org:8080/#/c/14824/27/be/src/runtime/query-driver.h File be/src/runtime/query-driver.h: http://gerrit.cloudera.org:8080/#/c/14824/27/be/src/runtime/query-driver.h@58 PS27, Line 58: inline ClientRequestState* o > isn't the point here that the pointer returned by query_driver() should be Sure, I mean there are basically two cases for how this might be used: - Most of the uses of this in the current patch are calls like "query_handle.query_driver()->SomeQueryDriverFn()", in which case the QueryHandle is going to be in scope for the whole call and making this a "const&" saves an atomic inc/dec without any risk. - If a developer wants to make a local variable like "shared_ptr<QueryDriver> = query_handle.query_driver()", making this a "const&" gives them the option of whether or not to make a copy (i.e. whether the local variable itself is declared a const& or not). This introduces a little risk, cause a dev could not make a copy in a situation where they actually needed one. Its different than the case with adding the operator-> since there we were returning a bare pointer to a ClientRequestState which doesn't allow the caller to get any guarantees on its lifetime even if they want. Obviously this isn't really super perf critical code, so its not a big deal. Feel free to leave as is. http://gerrit.cloudera.org:8080/#/c/14824/24/be/src/runtime/query-driver.h File be/src/runtime/query-driver.h: http://gerrit.cloudera.org:8080/#/c/14824/24/be/src/runtime/query-driver.h@136 PS24, Line 136: /// id, which will be distinct from the query id of the originally submitted query that > I think in that case the error still > gets propagated to the the ClientRequestState::query_status_ > though, which is why the errors show up in the runtime profile. I think that's true in some cases but not in other cases, but its fine. http://gerrit.cloudera.org:8080/#/c/14824/28/be/src/runtime/query-driver.cc File be/src/runtime/query-driver.cc: http://gerrit.cloudera.org:8080/#/c/14824/28/be/src/runtime/query-driver.cc@35 PS28, Line 35: StartUnregister Finalize() http://gerrit.cloudera.org:8080/#/c/14824/28/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/14824/28/be/src/service/impala-hs2-server.cc@150 PS28, Line 150: query_driver->CreateClientRequestState(query_ctx, session, &query_handle); You could also move the call to CreateClientRequestState into CreateNewDriver and then I think it wouldn't need to be public anymore and you wouldn't need to define the 'query_driver' variable on the line before this, and then pass the 'query_handle' into RegisterQuery which is nice for being consistent about passing QueryHandles everywhere. Fine to leave as is though http://gerrit.cloudera.org:8080/#/c/14824/27/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14824/27/be/src/service/impala-server.cc@1177 PS27, Line 1177: _m > CloseClientRequestState requires a bunch of non-const access - e.g. it pass In that case, a non-const QueryHandle& or a QueryHandle*. Not a big deal though -- To view, visit http://gerrit.cloudera.org:8080/14824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e4a0e72a9bf8ec10b91639aefd81bef17886ddd Gerrit-Change-Number: 14824 Gerrit-PatchSet: 28 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Wed, 13 May 2020 18:01:22 +0000 Gerrit-HasComments: Yes
