You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2018/06/26 20:09:25 UTC
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10827
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
IMPALA-7215: Implement a templatized CountingBarrier
Currently, our CountingBarrier util only notifies a 'bool' value
and uses an underlying Promise<bool>.
We're seeing cases in code where we might want to be notified of a
different kind of Promise (other than bool). We convert this utility
code to be templatized to be able to reuse the CountingBarrier for
these new use cases as well.
This was identified while working on IMPALA-7163.
Testing: Ran 'core' tests.
Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
---
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/util/counting-barrier.h
M be/src/util/hdfs-bulk-ops.cc
M be/src/util/hdfs-bulk-ops.h
10 files changed, 43 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10827/1
--
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: newchange
Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Gerrit-Change-Number: 10827
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
Patch Set 5: Code-Review+2
(4 comments)
Thanks for the review!
Rebase, carry +2.
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h
File be/src/util/counting-barrier.h:
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@36
PS4, Line 36: ks Wait() with
> Not clear which "returned value" this is talking about - sounds like Notify
Done
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@43
PS4, Line 43:
: /// Sets the number of pending notificatio
> that's clearer. you could say it this way for Notify() comment.
Done
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@56
PS4, Line 56:
> public comments shouldn't talk about private fields (the client of this cla
Done
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@61
PS4, Line 61: ms' passes, in which
: /// case '*timed_out' will be true. If
> same
Done
--
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: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 17:49:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h
File be/src/util/counting-barrier.h:
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@34
PS3, Line 34: /// Sends one notification, decrementing the number of pending notifications by one.
> Add something like:
Done
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@36
PS3, Line 36: /// If
> looks like no caller actually uses this return value and so you've annotate
We just don't use it anywhere now, but I think it is useful to have as a Util API, since in some cases we may want to know if we were the last one to Notify() or not. Otherwise, it's also useful to return the number of pending counts.
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@42
PS3, Line 42:
> similarly, add comment about 'promise_value'
Done
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@54
PS3, Line 54: }
> comment the return value
Done
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@55
PS3, Line 55:
> would be good to return "const T&" here and below (to match Promise.Get())
Done
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@58
PS3, Line 58: const T& Wait() { return promise_.Get(); }
> comment return value
Done
--
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: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 17:24:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 04:31:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
IMPALA-7215: Implement a templatized CountingBarrier
Currently, our CountingBarrier util only notifies a 'bool' value
and uses an underlying Promise<bool>.
We're seeing cases in code where we might want to be notified of a
different kind of Promise (other than bool). We add a templatized
class TypedCountingBarrier<T> and convert CountingBarrier to use the
TypedCountingBarrier<T> internally.
This was identified while working on IMPALA-7163.
Testing: Ran 'core' tests.
Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Reviewed-on: http://gerrit.cloudera.org:8080/10827
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/runtime/coordinator.cc
M be/src/util/counting-barrier.h
M be/src/util/hdfs-bulk-ops.cc
5 files changed, 62 insertions(+), 25 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Gerrit-Change-Number: 10827
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
Patch Set 3:
(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 parame
Done
http://gerrit.cloudera.org:8080/#/c/10827/1/be/src/util/counting-barrier.h@46
PS1, Line 46: eturn; // count_ can legitimately drop below 0
: if (count_.CompareAndSwap(value, 0)) {
: promise_.Set(promise_value);
> this would be simpler if we just require the promise value for the non-bool
Good point. I implemented a TypedCountingBarrier<T> and made CountingBarrier a user of TypedCountingBarrier<T>.
--
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: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 02:49:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10827
to look at the new patch set (#4).
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
IMPALA-7215: Implement a templatized CountingBarrier
Currently, our CountingBarrier util only notifies a 'bool' value
and uses an underlying Promise<bool>.
We're seeing cases in code where we might want to be notified of a
different kind of Promise (other than bool). We add a templatized
class TypedCountingBarrier<T> and convert CountingBarrier to use the
TypedCountingBarrier<T> internally.
This was identified while working on IMPALA-7163.
Testing: Ran 'core' tests.
Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
---
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/runtime/coordinator.cc
M be/src/util/counting-barrier.h
M be/src/util/hdfs-bulk-ops.cc
5 files changed, 61 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10827/4
--
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: newpatchset
Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Gerrit-Change-Number: 10827
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10827
to look at the new patch set (#5).
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
IMPALA-7215: Implement a templatized CountingBarrier
Currently, our CountingBarrier util only notifies a 'bool' value
and uses an underlying Promise<bool>.
We're seeing cases in code where we might want to be notified of a
different kind of Promise (other than bool). We add a templatized
class TypedCountingBarrier<T> and convert CountingBarrier to use the
TypedCountingBarrier<T> internally.
This was identified while working on IMPALA-7163.
Testing: Ran 'core' tests.
Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
---
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/runtime/coordinator.cc
M be/src/util/counting-barrier.h
M be/src/util/hdfs-bulk-ops.cc
5 files changed, 62 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10827/5
--
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: newpatchset
Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Gerrit-Change-Number: 10827
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
Patch Set 5: Verified+1
--
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: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 21:18:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10827
to look at the new patch set (#3).
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
IMPALA-7215: Implement a templatized CountingBarrier
Currently, our CountingBarrier util only notifies a 'bool' value
and uses an underlying Promise<bool>.
We're seeing cases in code where we might want to be notified of a
different kind of Promise (other than bool). We add a templatized
class TypedCountingBarrier<T> and convert CountingBarrier to use the
TypedCountingBarrier<T> internally.
This was identified while working on IMPALA-7163.
Testing: Ran 'core' tests.
Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
---
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/runtime/coordinator.cc
M be/src/util/counting-barrier.h
M be/src/util/hdfs-bulk-ops.cc
5 files changed, 54 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10827/3
--
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: newpatchset
Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Gerrit-Change-Number: 10827
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2753/ DRY_RUN=false
--
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: 5
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 17:49:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
Patch Set 4: Code-Review+2
(4 comments)
Just a few more comment cleanups for the public interface.
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h
File be/src/util/counting-barrier.h:
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@36
PS4, Line 36: returned value
Not clear which "returned value" this is talking about - sounds like Notify()'s returned value. The comment for NotifyRemaining is clearer
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@43
PS4, Line 43: unblocks Wait() with
: /// the returned value as 'promise_value'.
that's clearer. you could say it this way for Notify() comment.
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@56
PS4, Line 56: 'promise_'
public comments shouldn't talk about private fields (the client of this class shouldn't care that there is a promise in the implementation). Something like:
Returns the value passed to the final Notify() or NotifyRemaining() call.
http://gerrit.cloudera.org:8080/#/c/10827/4/be/src/util/counting-barrier.h@61
PS4, Line 61: returns the value set
: /// on 'promise_' in Notify() or Notif
same
--
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: 4
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 17:35:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10827
to look at the new patch set (#2).
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
IMPALA-7215: Implement a templatized CountingBarrier
Currently, our CountingBarrier util only notifies a 'bool' value
and uses an underlying Promise<bool>.
We're seeing cases in code where we might want to be notified of a
different kind of Promise (other than bool). We add a templatized
class TypedCountingBarrier<T> and convert CountingBarrier to use the
TypedCountingBarrier<T> internally.
This was identified while working on IMPALA-7163.
Testing: Ran 'core' tests.
Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
---
M be/src/exec/hdfs-scan-node.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/runtime/coordinator.cc
M be/src/util/counting-barrier.h
M be/src/util/hdfs-bulk-ops.cc
5 files changed, 59 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/10827/2
--
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: newpatchset
Gerrit-Change-Id: I05fc79228250408ae16481ae7ff3491a90d26b8e
Gerrit-Change-Number: 10827
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-7215: Implement a templatized CountingBarrier
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10827 )
Change subject: IMPALA-7215: Implement a templatized CountingBarrier
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h
File be/src/util/counting-barrier.h:
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@34
PS3, Line 34: /// Sends one notification, decrementing the number of pending notifications by one.
Add something like:
If this is the final notifier, sets the value returned to the waiter to 'promise_value'
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@36
PS3, Line 36: int32_t
looks like no caller actually uses this return value and so you've annotated them with discard_result(). You could also just make this return void since no one wants the value. I'm fine either way.
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@42
PS3, Line 42: /// Sets the number of pending notifications to 0 and unblocks Wait().
similarly, add comment about 'promise_value'
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@54
PS3, Line 54: /// Blocks until all notifications are received.
comment the return value
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@55
PS3, Line 55: T
would be good to return "const T&" here and below (to match Promise.Get())
http://gerrit.cloudera.org:8080/#/c/10827/3/be/src/util/counting-barrier.h@58
PS3, Line 58: /// case '*timed_out' will be true.
comment return value
--
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: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 04:32:41 +0000
Gerrit-HasComments: Yes