[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status or void

2016-10-25 Thread Todd Lipcon (Code Review) 
Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status or void
..


KUDU-78. Fix pb_util functions which return bool to return Status or void

As suggested in ticket, I've refactored four pb_util functions returning bool.
As it turns out, three of them should never fail (except with CHECKS), so I've
changed these to return void and added one missing check. The return type
change was propogated to callers as well: if any of them can't fail anymore
it's also refactored to return void.
The fourth one (ParseFromSequentialFile) can actually fail in two ways:
either with IO error or with Protobuf error (Status::Corruption). To track
which one, I had to add GetStatus() method into SequentialFileFileInputStream.

Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Reviewed-on: http://gerrit.cloudera.org:8080/4800
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/util/pb_util-internal.h
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
18 files changed, 87 insertions(+), 125 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status or void

2016-10-25 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status or void
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4800/3//COMMIT_MSG
Commit Message:

PS3, Line 7: Status
> Nit: Do you still want to keep the commit header as Status or change it as 
Yes, probably should be updated


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status or void

2016-10-25 Thread Maxim Smyatkin (Code Review) 
Hello Dinesh Bhat, Kudu Jenkins,

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

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

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

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status or void
..

KUDU-78. Fix pb_util functions which return bool to return Status or void

As suggested in ticket, I've refactored four pb_util functions returning bool.
As it turns out, three of them should never fail (except with CHECKS), so I've
changed these to return void and added one missing check. The return type
change was propogated to callers as well: if any of them can't fail anymore
it's also refactored to return void.
The fourth one (ParseFromSequentialFile) can actually fail in two ways:
either with IO error or with Protobuf error (Status::Corruption). To track
which one, I had to add GetStatus() method into SequentialFileFileInputStream.

Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/util/pb_util-internal.h
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
18 files changed, 87 insertions(+), 125 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-25 Thread Dinesh Bhat (Code Review) 
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..


Patch Set 3: Code-Review+1

(1 comment)

Nice cleanups, LGTM.

http://gerrit.cloudera.org:8080/#/c/4800/3//COMMIT_MSG
Commit Message:

PS3, Line 7: Status
Nit: Do you still want to keep the commit header as Status or change it as 
'Status or void' ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-24 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4800/2/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

Line 1019:   return Status::OK();
nit: now this function always returns Status::OK. Mind either propagating the 
change so this is void, or at least add a TODO here to do so?


http://gerrit.cloudera.org:8080/#/c/4800/2/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:

Line 520: Status SysCatalogTable::ReqAddTablets(WriteRequestPB* req,
same here, this now always returns OK


http://gerrit.cloudera.org:8080/#/c/4800/2/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:

Line 165:   return Status::OK();
and here


http://gerrit.cloudera.org:8080/#/c/4800/2/src/kudu/util/pb_util-internal.h
File src/kudu/util/pb_util-internal.h:

Line 59:   Status GetStatus() const {
style: should just be called 'status()'


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-24 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4800/1//COMMIT_MSG
Commit Message:

Line 9: As suggested in ticket, I've refactored four pb_util functions to 
return Status instead of bool.
> Nit: IIRC typically we keep commit message line wrapped to 80 chars or belo
Ok, didn't know it. Will have to change default git editor from nano to 
something showing column positions :)


http://gerrit.cloudera.org:8080/#/c/4800/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

Line 431:   return Status::OK();
> Few of these routines do not seem to be returning no other value than Statu
Well, it would be easier for me to do so.
But I thought they were designed to return bools at the first place because 
they were expected to fail in future. And for such cases the error handling 
code was already added.

Maybe I've got it wrong and they can be safely changed to void?


Line 443: return Status::Corruption("Error parsing msg", 
InitializationErrorMessage("parse", *msg));
> This is the only case I don't understand; how could msg->ParseFromZeroCopyS
I got the idea that msg may be malformed or something from TODO in pb_util.h:
"TODO: change this to return Status - differentiate IO error from bad PB"

Perhaps, that's wrong. Let me check it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-23 Thread Dinesh Bhat (Code Review) 
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4800/1//COMMIT_MSG
Commit Message:

Line 9: As suggested in ticket, I've refactored four pb_util functions to 
return Status instead of bool.
Nit: IIRC typically we keep commit message line wrapped to 80 chars or below 
unless we have a crash output or some LOG output as part of commit message.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-23 Thread Dinesh Bhat (Code Review) 
Dinesh Bhat has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4800/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

Line 431:   return Status::OK();
Few of these routines do not seem to be returning no other value than 
Status::OK, I think we could as well keep these routines' return type as void. 
Otherwise LGTM.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-23 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4800/1//COMMIT_MSG
Commit Message:

PS1, Line 10: wais
Nit: ways


http://gerrit.cloudera.org:8080/#/c/4800/1/src/kudu/master/sys_catalog.cc
File src/kudu/master/sys_catalog.cc:

Line 441: table->metadata().dirty().name()));
Let's actually use the table ID here. It's more consistent with the other 
failure cases (where we prepend either the table or tablet ID).

Also, table names can be considered sensitive user information that shouldn't 
be logged (though we have a lot of work to do to fix this).


http://gerrit.cloudera.org:8080/#/c/4800/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

Line 443: return Status::Corruption("Error parsing msg", 
InitializationErrorMessage("parse", *msg));
This is the only case I don't understand; how could 
msg->ParseFromZeroCopyStream() return false, yet istream.GetStatus() return 
Status::OK()?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-22 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has uploaded a new change for review.

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

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..

KUDU-78. Fix pb_util functions which return bool to return Status

As suggested in ticket, I've refactored four pb_util functions to return Status 
instead of bool.
Only one of them can actually fail, for now: ParseFromSequentialFile. It can 
fail in two wais:
either with IO error or with Protobuf error (Status::Corruption). To track 
which one, I had to add
GetStatus() method into SequentialFileFileInputStream.

Also there were few misprints in master/sys_catalog.cc error messages 
("table"/"tablet"),
and a small bug in consensus/log.cc with wrong string::Substitute format ($1 
instead of $0).

Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log_util.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/util/pb_util-internal.h
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
9 files changed, 48 insertions(+), 51 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin