You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/07/11 21:57:18 UTC
[kudu-CR] util: add Status::AndThen combinator
Dan Burkert has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7399
Change subject: util: add Status::AndThen combinator
......................................................................
util: add Status::AndThen combinator
The AndThen combinator makes it easier to chain together
Status-returning operations when failure recovery is something other
than returning early. RETURN_NOT_OK already handles the return-early
case elegantly.
Also tweaks Status::CloneAndPrepend and Status::CloneAndAppend to be
usable with OK status values.
Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
---
M src/kudu/util/status-test.cc
M src/kudu/util/status.cc
M src/kudu/util/status.h
3 files changed, 60 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7399/1
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: util: add Status::AndThen combinator
......................................................................
util: add Status::AndThen combinator
The AndThen combinator makes it easier to chain together
Status-returning operations when failure recovery is something other
than returning early. RETURN_NOT_OK already handles the return-early
case elegantly.
Also tweaks Status::CloneAndPrepend and Status::CloneAndAppend to be
usable with OK status values.
Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Reviewed-on: http://gerrit.cloudera.org:8080/7399
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/util/status-test.cc
M src/kudu/util/status.cc
M src/kudu/util/status.h
3 files changed, 60 insertions(+), 2 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7399/3//COMMIT_MSG
Commit Message:
PS3, Line 9: The AndThen combinator makes it easier to chain together
: Status-returning operations when failure recovery is something other
: than returning early. RETURN_NOT_OK already handles the return-early
: case elegantly.
> I think my confusion stemmed from expecting failure recovery to take more o
I'm not entirely sure, but I'd be surprised if it made a measurable difference in compile times.
http://gerrit.cloudera.org:8080/#/c/7399/3/src/kudu/util/status.h
File src/kudu/util/status.h:
PS3, Line 187: ok()
> In case of very long chains, should we PREDICT_TRUE? And in CloneAndPrepend
I wasn't really sure about this. I didn't want to add it to CloneAndPrepend because that method is currently called only when the status is failed. In the case of AndThen, I think the case could be made for it, but I opted to keep it simple for now. If you feel strongly about it I can add it.
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3:
Here's an example from a patch which builds off this, in CatalogManager::CreateTable
// a. Validate the user request.
Schema client_schema;
s = SchemaFromPB(req.schema(), &client_schema)
.AndThen([&] () {
return ValidateIdentifier(req.database_name()).CloneAndPrepend("invalid database name");
}).AndThen([&] () {
return ValidateIdentifier(req.table_name()).CloneAndPrepend("invalid table name");
}).AndThen([&] () {
return ValidateClientSchema(client_schema);
}).AndThen([&] () {
if (client_schema.has_column_ids()) {
return Status::InvalidArgument("user requests should not have Column IDs");
} else {
return Status::OK();
}
});
if (!s.ok()) {
return SetError(MasterErrorPB::INVALID_SCHEMA, s);
}
For what it's worth, this is a common method to have on status-like types in functional languages, for instance Scala's Try.flatMap[1] or Rust's Result.and_then [2].
[1]: http://www.scala-lang.org/api/2.12.x/scala/util/Try.html#flatMap[U](f:T=>scala.util.Try[U]):scala.util.Try[U]
[2]: https://doc.rust-lang.org/std/result/enum.Result.html#method.and_then
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] util: add Status::AndThen combinator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7399/2//COMMIT_MSG
Commit Message:
PS2, Line 11: RETURN_NOT_OK
As an alternative, could Status::AndThen's functionality be implemented as macro as well?
Like RETURN_NOT_OK_CLEANUP(TryDoSomething(), Rollback()) ?
http://gerrit.cloudera.org:8080/#/c/7399/2/src/kudu/util/status.h
File src/kudu/util/status.h:
PS2, Line 172: {.cpp}
This is redundant: it's inferred from the file this sample is located, so it's 'cpp' already.
PS2, Line 172: \
nit: I think it would be nice to keep the @-like notation here:
@code
PS2, Line 180: \
nit: ditto the comment style -- it would be better to have @endcode here
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7399/1/src/kudu/util/status.h
File src/kudu/util/status.h:
Line 178: /// .CloneAndPrepend("failed to write to example file");
> this pattern seems a little suspect because it always eager-evaluates the a
Yes. In this case it's more-or-less zero cost since it's a static Slice, but if it were a Substitute or similar it may be expensive.
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] util: add Status::AndThen combinator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] util: add Status::AndThen combinator
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7399/1/src/kudu/util/status.h
File src/kudu/util/status.h:
Line 162: Status& operator=(Status&& s);
> ignoring.
I think you can add // NOLINT if you want to get rid of it
Line 178: /// .CloneAndPrepend("failed to write to example file");
this pattern seems a little suspect because it always eager-evaluates the argument of CloneAndPrepend, but I guess we can just keep an eye out for that?
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7399
to look at the new patch set (#3).
Change subject: util: add Status::AndThen combinator
......................................................................
util: add Status::AndThen combinator
The AndThen combinator makes it easier to chain together
Status-returning operations when failure recovery is something other
than returning early. RETURN_NOT_OK already handles the return-early
case elegantly.
Also tweaks Status::CloneAndPrepend and Status::CloneAndAppend to be
usable with OK status values.
Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
---
M src/kudu/util/status-test.cc
M src/kudu/util/status.cc
M src/kudu/util/status.h
3 files changed, 60 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7399/3
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] util: add Status::AndThen combinator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3: Code-Review+1
> Here's an example from a patch which builds off this, in
> CatalogManager::CreateTable
>
> // a. Validate the user request.
> Schema client_schema;
> s = SchemaFromPB(req.schema(), &client_schema)
> .AndThen([&] () {
> return ValidateIdentifier(req.database_name()).CloneAndPrepend("invalid
> database name");
> }).AndThen([&] () {
> return ValidateIdentifier(req.table_name()).CloneAndPrepend("invalid
> table name");
> }).AndThen([&] () {
> return ValidateClientSchema(client_schema);
> }).AndThen([&] () {
> if (client_schema.has_column_ids()) {
> return Status::InvalidArgument("user requests should not have
> Column IDs");
> } else {
> return Status::OK();
> }
> });
>
> if (!s.ok()) {
> return SetError(MasterErrorPB::INVALID_SCHEMA, s);
> }
>
>
> For what it's worth, this is a common method to have on status-like
> types in functional languages, for instance Scala's Try.flatMap[1]
> or Rust's Result.and_then [2].
>
> [1]: http://www.scala-lang.org/api/2.12.x/scala/util/Try.html#flatMap[U](f:T=>scala.util.Try[U]):scala.util.Try[U]
> [2]: https://doc.rust-lang.org/std/result/enum.Result.html#method.and_then
Thanks! I appreciate this.
For some reason I thought the rollback method somehow could be specified in the AndThen() -- that was my misunderstanding.
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7399
to look at the new patch set (#2).
Change subject: util: add Status::AndThen combinator
......................................................................
util: add Status::AndThen combinator
The AndThen combinator makes it easier to chain together
Status-returning operations when failure recovery is something other
than returning early. RETURN_NOT_OK already handles the return-early
case elegantly.
Also tweaks Status::CloneAndPrepend and Status::CloneAndAppend to be
usable with OK status values.
Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
---
M src/kudu/util/status-test.cc
M src/kudu/util/status.cc
M src/kudu/util/status.h
3 files changed, 60 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7399/2
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7399/1/src/kudu/util/status.h
File src/kudu/util/status.h:
Line 162: Status& operator=(Status&& s);
> warning: move assignment operators should be marked noexcept [misc-noexcept
ignoring.
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] util: add Status::AndThen combinator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3:
> (1 comment)
I agree. I just wanted to understand what's the intended use case (didn't see the chaining of calls right away).
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7399/2//COMMIT_MSG
Commit Message:
PS2, Line 11: RETURN_NOT_OK
> As an alternative, could Status::AndThen's functionality be implemented as
AndThen is nice for scenarios where you have a sequence of operations which can fail, but you want to handle all of the failures the same. For instance, we do this a lot in the server where a sequence of possibly failing operations occurs, and then if any of them fail, we mark the RPC return specially. I don't think such a macro would help reduce the rollback boilerplate.
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] util: add Status::AndThen combinator
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7399/2//COMMIT_MSG
Commit Message:
PS2, Line 11: RETURN_NOT_OK
> AndThen is nice for scenarios where you have a sequence of operations which
Also, for what it's worth, I think anything that can be implemented as a function instead of a macro probably should be. RETURN_NOT_OK is special because of the control flow requirements.
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] util: add Status::AndThen combinator
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3:
> (1 comment)
I'm not sure I understand -- how does Status::AndThen() allow to specify a common rollback method if any of the chained operations (mor than 1) in the AndThen() fail? Could you add some sort of example for that into the test?
Maybe, I just misread the description or just missing something essential.
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] util: add Status::AndThen combinator
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: util: add Status::AndThen combinator
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7399/3//COMMIT_MSG
Commit Message:
PS3, Line 9: The AndThen combinator makes it easier to chain together
: Status-returning operations when failure recovery is something other
: than returning early. RETURN_NOT_OK already handles the return-early
: case elegantly.
I think my confusion stemmed from expecting failure recovery to take more of an or_else shape. Maybe note somewhere that the intent to replace this:
Status SequenceOfStatuses() {
RETURN_NOT_OK(S1)
RETURN_NOT_OK(S2)
RETURN_NOT_OK(S3)
return Status::OK()
}
do_stuff(SequenceOfStatuses())
with this:
Status s = S1->AndThen(S2)->AndThen(S3)
do_stuff(s)
Although I suppose we get the bonus of not having to compile a new SequenceOfStatuses() function.
Actually, is chaining a bunch of small lambdas more costly to compile than one large one (e.g. if SequenceOfStatuses were a lambda)?
http://gerrit.cloudera.org:8080/#/c/7399/3/src/kudu/util/status.h
File src/kudu/util/status.h:
PS3, Line 187: ok()
In case of very long chains, should we PREDICT_TRUE? And in CloneAndPrepend?
--
To view, visit http://gerrit.cloudera.org:8080/7399
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6e1a79ff95406825d4238a8157d242252562805
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes