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 5: (7 comments) Addressed some of the comments and filed a few follow up JIRAs. Going to start a doc to discuss the "transparent" part of this feature (e.g. the internal query ids). Working on doing some of the re-factoring you mentioned (e.g. adding another layer of abstraction on top of ClientRequestState). http://gerrit.cloudera.org:8080/#/c/14824/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14824/5//COMMIT_MSG@26 PS5, Line 26: A query cannot be retried once any results from the original query : have been fetched, this is to prevent users from seeing incorrect results > Do we have any mechanism for a user to specify that they want their results Not yet, but planning to tackle that in a follow up JIRA: IMPALA-9225 http://gerrit.cloudera.org:8080/#/c/14824/5//COMMIT_MSG@41 PS5, Line 41: The Impala Web UI will list all retried queries as being in the : "RETRIED" state > This may be a reasonable approach, but I think we should think about the us Yeah, these are legitimate issues. We have few follow up JIRAs to tackle some of these problems: IMPALA-9229, IMPALA-9230, IMPALA-9213 and I just filed IMPALA-9364 as well - open to more suggestions about how to improve supportability. For this patch, I think it should be sufficient to add the original query id to the retried query profile, and vice versa. http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/client-request-state-map.h File be/src/service/client-request-state-map.h: http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/client-request-state-map.h@38 PS5, Line 38: bool overwrite = false > I don't think this is being used anywhere anymore. Done http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/client-request-state.h@82 PS5, Line 82: RETRYING, RETRIED, UNKNOWN > I think this approach, and the way you have to save the previous state and Yeah, thats a good suggestion. Haven't looked into adding the ClientRequestState wrapper yet, but for now just split out the the RETRYING and RETRIED states into a `enum RetryState`. It actually simplifies the state management considerably, so thanks for the suggestion. http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/impala-server.cc@632 PS5, Line 632: shared_ptr<ClientRequestState> request_state = GetClientFacingRequestState(query_id); > Unless I'm missing something in this patch, this means that only the profil hmm not sure why I did that, changed back to GetClientRequestState http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/impala-server.cc@920 PS5, Line 920: unique_ptr<TExecRequest> exec_request = make_unique<TExecRequest>(); > So I'm wondering if this patch would benefit from adding another layer of a I think that makes sense. IMO the ImpalaServer <--> ClientRequestState <--> Coordinator code needs some re-factoring in general. I wanted to some general re-factoring all this code (ImpalaServer, ClientRequestState, Coordinator) so I filed IMPALA-9370 as a follow up, but I think it makes sense to move some of the retry logic into its own class in this patch. I re-factored the BlockOnWait code so that it isn't duplicated across the hs2 and beeswax server. http://gerrit.cloudera.org:8080/#/c/14824/5/be/src/service/impala-server.cc@1025 PS5, Line 1025: query_ctx->query_id = UuidToQueryId(random_generator()()); > As mentioned before, I definitely think it would be awesome if we could cre I sort of did this in the previous patch update. I added a client_query_id_mapping_ that allows an optional mapping between query ids. It doesn't exactly create an "internal" id, but its cleaner that what I was doing before. I think I'm going to resurrect the design doc for this feature and write up some notes on how best to approach this. We can finalize the design there. The mixing of the hi/lo of the TUniqueId is an interesting idea. I'll be sure to include that. -- 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: 5 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Mon, 10 Feb 2020 16:50:40 +0000 Gerrit-HasComments: Yes
