Joe McDonnell 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 16: (4 comments) Took another pass. It is mostly nits about beeswax::QueryHandle vs impala::QueryHandle and some stale comments. http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/runtime/coordinator.h@248 PS16, Line 248: A shared_ptr is used because the : /// Coordinator can outlive its associated QueryDriver. This comment needs updating http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/service/client-request-state.h@607 PS16, Line 607: A shared_ptr is used because the : /// ClientRequestState can outlive its associated QueryDriver. Update this comment (or remove) http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/service/impala-beeswax-server.cc@212 PS16, Line 212: query_handle; Nit: It would be nice to standardize the naming for variables/arguments for beeswax::QueryHandle vs impala::QueryHandle across the functions in this file. e.g. the beeswax::QueryHandle is always 'handle' or 'beeswax_handle' or whatever and the impala::QueryHandle is always 'query_handle' or 'impala_query_handle' or whatever. I think I would lean toward beeswax::QueryHandle -> 'beeswax_handle' and impala::QueryHandle -> 'query_handle', but I'm flexible. This variability predates your change, but I don't think there is anything stopping us from standardizing. http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/14824/16/be/src/service/impala-server.h@933 PS16, Line 933: void TUniqueIdToQueryHandle(const TUniqueId& query_id, beeswax::QueryHandle* handle); : void QueryHandleToTUniqueId(const beeswax::QueryHandle& handle, TUniqueId* query_id); Nit: Should we rename this to emphasize that this is a beeswax::QueryHandle? Something like: TUniqueIdToBeeswaxHandle() BeeswaxHandleToTUniqueId() -- 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: 16 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: Tue, 05 May 2020 23:38:53 +0000 Gerrit-HasComments: Yes
