Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10060 )
Change subject: IMPALA-5216: Make admission control queuing async ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.h@172 PS7, Line 172: Coordinator* coord() const { : return coord_exec_started_.Load() ? coord_.get() : nullptr; : } > Renamed to GetCoord(). If you find that there are too many entry points to CRS due to the various uses of coord(), and it doesn't feel like things are improving, then let me know and we can defer this until we do a cleanup of CRS. I'll hold off on doing a detailed review round until you decide whether or no to go forward with this part, since it will cause a fair amount of change. If we do continue with the current approach, once we convert to a more state-machine centric implementation, can we get rid of coord_exec_started_ and instead just check for FINISHED state (or whatever the internal state is, if we have internal states)? http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc@481 PS7, Line 481: query-exec-state > I dont see any reason not to. Since this only needs to be changed in 2 othe I think it would be okay. The only reason to be hesitant is if there might be some external tool depending on this name (I'm not aware of that though), in which case it would be easier to do it separately so it can be reverted separately if need be. http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/client-request-state.cc@1155 PS7, Line 1155: // set it to CANCELLED. > I totally agree about cleanup. Also, please see previous comment for why I Out of curiosity, which test expects FINISHED/OK after cancellation? http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/service/impala-server.cc@663 PS7, Line 663: return Status::OK(); > This method is only called by the hs2 and beeswax's GetExecSummary api, so Right, depends on the client. Maybe check to see if/how CM and Hue and any other management layer use it? http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/util/promise.h File be/src/util/promise.h: http://gerrit.cloudera.org:8080/#/c/10060/7/be/src/util/promise.h@77 PS7, Line 77: return val_; > hmmm, you are right but this would be similar to how its implemented right Sure. I was thinking of something simpler just to refactor the code. But I'm okay with your proposal as well. -- To view, visit http://gerrit.cloudera.org:8080/10060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I989cf5b259afb8f5bc5c35590c94961c81ce88bf Gerrit-Change-Number: 10060 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 14 May 2018 17:42:42 +0000 Gerrit-HasComments: Yes
