Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/10827/1/be/src/util/counting-barrier.h File be/src/util/counting-barrier.h: http://gerrit.cloudera.org:8080/#/c/10827/1/be/src/util/counting-barrier.h@39 PS1, Line 39: ) rather than have a default_promise_value_, why not just pass it as a parameter to Notify()? We'll inevitably need that for the typed Promise case. http://gerrit.cloudera.org:8080/#/c/10827/1/be/src/util/counting-barrier.h@46 PS1, Line 46: T* optional_promise_override = nullptr) { : T promise_value = (optional_promise_override != nullptr) ? : *optional_promise_override : default_promise_value_ this would be simpler if we just require the promise value for the non-bool case because otherwise why would you specify the type if you don't want to specify the value? Also, rather than making all the callers deal with the meaningless bool arg and discarding the return value, it seems best to hide the bool case from the callers (so in the standard case, arg and return values aren't needed since they are just implementation artifacts). That is, in the places we currently have a CountingBarrier, the fact that it's implemented with a bool type is just an implementation artifact. it could have just as well been int or whatever type, so the users of the plain CountingBarrier shouldn't be aware of it. One option is to rename this class to TypedCounterBarrier<T> (or whatever name) and then define CountingBarrier to be a thin wrapper class containing a TypedCounterBarrier<bool> that removes the args/return values to get the interface back to the status quo for plain CountingBarrier. -- To view, visit http://gerrit.cloudera.org:8080/10827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e Gerrit-Change-Number: 10827 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Comment-Date: Wed, 27 Jun 2018 04:31:29 +0000 Gerrit-HasComments: Yes
