Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10158 )
Change subject: IMPALA-5384, part 2: Simplify Coordinator locking and clarify state ...................................................................... Patch Set 20: (12 comments) Thanks for taking a look Sailesh. http://gerrit.cloudera.org:8080/#/c/10158/19/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/19/be/src/runtime/coordinator.h@93 PS19, Line 93: /// 2. exec_state_lock_, backend_states_init_lock_, filter_lock_, > What about backend_states_init_lock_ ? Done http://gerrit.cloudera.org:8080/#/c/10158/19/be/src/runtime/coordinator.h@206 PS19, Line 206: > comment requires an update. referenced the class level comment. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.h@325 PS18, Line 325: rySummary() and > const ref ExecState is a scalar (enum). We pass scalar by value - no reason to have indirection. I can make them const though, if you prefer, given these are leafs routines. Generally, I'm not a huge fan of const scalar args because it leads to const creep with little benefit (compiler has to perform analysis anyway, and usually easy enough to see when non-pointers are not modified/aliased). http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.h@350 PS18, Line 350: in all cases releases resources and cal > These look like they're read only, so they should be passed as const refs. Scalar and in registers, so not going to pass by reference. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.h@357 PS18, Line 357: > const ref made it const http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.h@360 PS18, Line 360: > const ref made it const http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@127 PS14, Line 127: (); > You mean Cancel() and UpdateBackendExecStatus()? Maybe this is not worth me This comment was pre-existing but I'm fine with removing it. It's covered in the class comment. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@279 PS14, Line 279: > To be more explicit, we should say "... or when Cancel() is called." Cancel() is not called for (execution) errors. Thats part of the point of the patch is to distinguish canceled state and error state. (It's true that the impala server layer can implement an error at that layer using Cancel(), but that's not an execution error, and I'd rather not muddle the terminology). Will update this comment to talk about backend cancellation, though. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc@457 PS18, Line 457: ret_status > Preferable to get rid of the automatic variable 'ret_status' and just retur exec_status_ is protected by the lock, so the copy is necessary. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc@462 PS18, Line 462: Status ret_status; > Same here. Same locking. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc@476 PS18, Line 476: DCHECK(exec_status_.ok()) << exec_status_; > If an error happens after exec_state_ is already set to ExecState::RETURNED My thinking was that it would be good to at least log the error, which is why I wrote the code that way. Alternatively, we could only log when a state transition happens, but it seems like it might be good to know when an backend error occurs even if not failing the query, but I'm also not a fan of log spew. Are you arguing for one way or the other? http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc@486 PS18, Line 486: !status.ok() && !status.IsCancelled() && e > (!status.ok() && !status.IsCancelled() && exec_status_.IsCancelled()) This path should be rare, but sure we can avoid the copy in that case. -- To view, visit http://gerrit.cloudera.org:8080/10158 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1abdfd02163f9356c59d470fe1c64ebe012a9e8e Gerrit-Change-Number: 10158 Gerrit-PatchSet: 20 Gerrit-Owner: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 11 May 2018 16:37:02 +0000 Gerrit-HasComments: Yes
