Sahil Takiar 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 11: (10 comments) Patch set 10 is a rebase. Patch set 11 address the following comments. Addressed / responded to the rest of the comments from Thomas. Addressed most (but not all) comments from Joe. http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/query-driver.h File be/src/runtime/query-driver.h: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/query-driver.h@113 PS9, Line 113: (2) the : /// query has not already been ret > not sure what this is supposed to mean revised http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/query-driver.h@116 PS9, Line 116: d if : /// there has been a c > I think this should be singular "a statestore update" Done http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/query-driver.h@122 PS9, Line 122: /// is set to true if the query was actually retried, false otherwise. This method is : /// idempotent, it can safely be called multiple tim > From reading through TryQueryRetry(), it looks like this is safe to call mu updated docs to mention that method is idempotent http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/client-request-state.h@84 PS9, Line 84: ; > I don't think this is used anywhere? Done http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc@148 PS9, Line 148: shared_ptr<QueryDriver> query_driver; > I don't think this is used anywhere? Done http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc@763 PS9, Line 763: HS2_RETURN_IF_ERROR( > I had the same concern. yeah, thats a good point, thanks for catching that. changed it to use a shared_ptr http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc@1039 PS9, Line 1039: // The original query profile should still be accessible via the web ui. Don't use > I think its definitely necessary that we provide a way for all relevant pro agree this is an issue. the reason i went with the current approach is so that all methods in the hs2/beeswax interface follow the same "transparent" pattern. i think it might be slightly more intuitive to return the profile of the successful query rather than the originally failed one. although I think returning the original, failed profile could make sense as well. I don't know if one approach is necessarily that much better than the other. i agree that there should be a way to fetch the failed profiles via the hs2 interface, I'm planning to tackle that in IMPALA-9229 http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-server.h@673 PS9, Line 673: std::shared_ptr<Clie > Stylistically, I personally don't like mutable references for out arguments Done since I had to change this all the shared_ptr, so now it is "shared_ptr<ClientRequestState>*" http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-server.cc@1453 PS9, Line 1453: // ClientRequestState::output_exprs_, which > Nit: you could move this declaration down past the first BlockOnWait() Done http://gerrit.cloudera.org:8080/#/c/14824/9/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14824/9/common/thrift/ImpalaService.thrift@523 PS9, Line 523: > typo Done -- 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: 11 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: Thu, 30 Apr 2020 00:04:51 +0000 Gerrit-HasComments: Yes
