Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9718 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/9718/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9718/1//COMMIT_MSG@7 PS1, Line 7: Fix flaky test_profile_fragment_instances This subject makes it sound like you're fixing the flaky test. But you're actually making an impala change, so could you reword this to summarize the change? Otherwise, when looking at git commits, it's hard to know that this change is making a subtle coordinator behavior change. http://gerrit.cloudera.org:8080/#/c/9718/1//COMMIT_MSG@43 PS1, Line 43: Returning results for queries that are cancelled by the user is : unaffected as the manual cancel path causes WaitForBackendCompletion(). I don't follow that. What does this have to do with query results? Also, what do you mean by the "path causes WaitForBackendCompletion()". Which callpath are you referring to? http://gerrit.cloudera.org:8080/#/c/9718/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/9718/1/be/src/runtime/coordinator-backend-state.h@119 PS1, Line 119: boost::mutex* lock() { return &lock_; } > I don't think it will help with the iteration part, but it'll prevent mista I think we should try hard to not expose the lock out of a class. That's a big problem with ClientRequestState. The fact that it's exposed usually means some abstraction is missing or wrong. (If we really do end up needing to take the series of locks, then it'd be good to follow the pattern here: https://github.com/apache/impala/blob/dc2f69e5a0b36ca721eeadeec661a251c957410b/be/src/runtime/bufferpool/buffer-allocator.cc#L341) http://gerrit.cloudera.org:8080/#/c/9718/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/9718/1/be/src/runtime/coordinator.cc@1028 PS1, Line 1028: // ensure the profile isn't being updated when we call Add(Split|Exec)Stats below. What are the backend locks protecting in Add*Stats()? Which profile is the comment referring to? -- To view, visit http://gerrit.cloudera.org:8080/9718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f8295e4ec580b476f7668c77aec1348fb2e868c Gerrit-Change-Number: 9718 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Mon, 26 Mar 2018 23:36:03 +0000 Gerrit-HasComments: Yes
