You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/11/03 11:12:21 UTC

[kudu-CR] Add a macro to LOG and return on a non-OK status 

David Ribeiro Alves has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4927

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................

Add a macro to LOG and return on a non-OK status

We often see the following pattern:

Status s = someOperation();

if (!s.ok()) {
  LOG(ERROR) << "A critical error occurred at blah. Status: " << s.ToString();
  return s;
}

This is cumbersome. This macro allows to do the same the following way:

RETURN_NOT_OK_LOG(ERROR, someOperation(), "A critical error occurred at blah.");

The macro also prints the status (though at the beggining of the message and not
at the end so that we can have arbirarily long debugging information but the
status is easy to find).

Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
---
M src/kudu/util/status.h
1 file changed, 10 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/4927/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4927

to look at the new patch set (#6).

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................

Add a macro to LOG and return on a non-OK status

We often see the following pattern:

Status s = someOperation();

if (!s.ok()) {
  LOG(ERROR) << "A critical error occurred at blah. Status: " << s.ToString();
  return s;
}

This is cumbersome. This macro allows to do the same the following way:

RETURN_NOT_OK_LOG(someOperation(), ERROR, "A critical error occurred at blah.");

The macro also prints the status (though at the beginning of the
message and not at the end so that we can have arbirarily long
debugging information but the status is easy to find).

Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
---
M src/kudu/util/status.h
1 file changed, 10 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/4927/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4927

to look at the new patch set (#5).

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................

Add a macro to LOG and return on a non-OK status

We often see the following pattern:

Status s = someOperation();

if (!s.ok()) {
  LOG(ERROR) << "A critical error occurred at blah. Status: " << s.ToString();
  return s;
}

This is cumbersome. This macro allows to do the same the following way:

RETURN_NOT_OK_LOG(ERROR, someOperation(), "A critical error occurred at blah.");

The macro also prints the status (though at the beginning of the
message and not at the end so that we can have arbirarily long
debugging information but the status is easy to find).

Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
---
M src/kudu/util/status.h
1 file changed, 10 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/4927/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4927

to look at the new patch set (#3).

Change subject:  Add a macro to LOG and return on a non-OK status
......................................................................

Add a macro to LOG and return on a non-OK status

We often see the following pattern:

Status s = someOperation();

if (!s.ok()) {
  LOG(ERROR) << "A critical error occurred at blah. Status: " << s.ToString();
  return s;
}

This is cumbersome. This macro allows to do the same the following way:

RETURN_NOT_OK_LOG(ERROR, someOperation(), "A critical error occurred at blah.");

The macro also prints the status (though at the beginning of the
message and not at the end so that we can have arbirarily long
debugging information but the status is easy to find).

Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
---
M src/kudu/util/status.h
1 file changed, 10 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/4927/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4927

to look at the new patch set (#4).

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................

Add a macro to LOG and return on a non-OK status

We often see the following pattern:

Status s = someOperation();

if (!s.ok()) {
  LOG(ERROR) << "A critical error occurred at blah. Status: " << s.ToString();
  return s;
}

This is cumbersome. This macro allows to do the same the following way:

RETURN_NOT_OK_LOG(ERROR, someOperation(), "A critical error occurred at blah.");

The macro also prints the status (though at the beginning of the
message and not at the end so that we can have arbirarily long
debugging information but the status is easy to find).

Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
---
M src/kudu/util/status.h
1 file changed, 10 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/4927/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Add a macro to LOG and return on a non-OK status

We often see the following pattern:

Status s = someOperation();

if (!s.ok()) {
  LOG(ERROR) << "A critical error occurred at blah. Status: " << s.ToString();
  return s;
}

This is cumbersome. This macro allows to do the same the following way:

RETURN_NOT_OK_LOG(someOperation(), ERROR, "A critical error occurred at blah.");

The macro also prints the status (though at the beginning of the
message and not at the end so that we can have arbirarily long
debugging information but the status is easy to find).

Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Reviewed-on: http://gerrit.cloudera.org:8080/4927
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/util/status.h
1 file changed, 10 insertions(+), 0 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4927/2//COMMIT_MSG
Commit Message:

Nit: if possible, please keep the commit message line length under 72 characters:

https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines


PS2, Line 22: beggining
nit: typo, should have been beginning


http://gerrit.cloudera.org:8080/#/c/4927/2/src/kudu/util/status.h
File src/kudu/util/status.h:

PS2, Line 65: and
nit: log it as 'msg' at 'level' and return the status


PS2, Line 66: KUDU_RETURN_NOT_OK_LOG
May be, name it KUDU_LOG_AND_RETURN_MSG() since it's very similar to KUDU_LOG_AND_RETURN()?

And re-implement the KUDU_LOG_AND_RETURN() macro via this new macro, dropping the additional 'Status: ' prefix?


PS2, Line 69: msg
since it's a macro parameter, it's better to enclose it into parenthesis.


-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4927/2//COMMIT_MSG
Commit Message:

> Nit: if possible, please keep the commit message line length under 72 chara
ah, neat. didn't know that one. did it for the most part, but didn't wrap code examples


PS2, Line 22: beggining
> nit: typo, should have been beginning
Done


http://gerrit.cloudera.org:8080/#/c/4927/2/src/kudu/util/status.h
File src/kudu/util/status.h:

PS2, Line 65: and
> nit: log it as 'msg' at 'level' and return the status
Done


PS2, Line 66: KUDU_RETURN_NOT_OK_LOG
> May be, name it KUDU_LOG_AND_RETURN_MSG() since it's very similar to KUDU_L
That one reads slightly weird to me as it seems that it returns the message and not the status. Notice that KUDU_LOG_AND_RETURN returns on any status not just not ok and that this one additionally takes 'msg' which makes it hard to reuse the macro.

I first thought of LOG_RETURN_NOT_OK() but this one also seems to hint that we log all the time so the current one was the best that I could think of. Finally I didn't want to include ERROR (like LOG_ERROR_RETURN_NOT_OK) since the user might confuse ERROR with the passed 'level'.


PS2, Line 69: msg
> since it's a macro parameter, it's better to enclose it into parenthesis.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Patch Set 6: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4927/5//COMMIT_MSG
Commit Message:

PS5, Line 20: ERROR, someOperation()
nit: please update the order of the arguments here to match the updated signature of the RETURN_NOT_OK_LOG macro.


http://gerrit.cloudera.org:8080/#/c/4927/5/src/kudu/util/status.h
File src/kudu/util/status.h:

PS5, Line 69: << "Status: "
nit: do you think the "Status: " prefix is really necessary here?


-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4927/5//COMMIT_MSG
Commit Message:

PS5, Line 20: ERROR, someOperation()
> nit: please update the order of the arguments here to match the updated sig
Done


http://gerrit.cloudera.org:8080/#/c/4927/5/src/kudu/util/status.h
File src/kudu/util/status.h:

PS5, Line 69: << "Status: "
> nit: do you think the "Status: " prefix is really necessary here?
Yeah, I think is makes is obvious which part of the message is the resulting status of the operation.


-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4927/4/src/kudu/util/status.h
File src/kudu/util/status.h:

PS4, Line 66: level, s, msg
> nit: it would read better for me to put the log level before the message, n
was just trying to keep this consistent with the one above, but I prefer your suggestion. Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Add a macro to LOG and return on a non-OK status

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Add a macro to LOG and return on a non-OK status
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4927/4/src/kudu/util/status.h
File src/kudu/util/status.h:

PS4, Line 66: level, s, msg
nit: it would read better for me to put the log level before the message, no?

KUDU_RETURN_NOT_OK(Foo(), WARNING, "blah")?


-- 
To view, visit http://gerrit.cloudera.org:8080/4927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb906e7240d7a289a7822444bc9cbf748a555efb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes