You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dinesh Bhat (Code Review)" <ge...@cloudera.org> on 2016/09/29 20:03:26 UTC
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Hello Mike Percy, Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4566
to review the following change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
[util] Fix a minor bug in AssertEventually()
We want to be sleeping with a monotonically increasing timeout
value in this routine. After 10 attempts, we sleep
for 1000 ms(constant value). But this routine was never
increasing the 'attempts' thus falling back to sleep always at 1ms
intervals making it inefficient.
Also snuck in a change to remove a warning stemming from fresh builds.
Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
---
M src/kudu/common/column_predicate.cc
M src/kudu/util/test_util.cc
2 files changed, 2 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/4566/1
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4566/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 590: default: LOG(FATAL) << "unknown predicate type";
Out of curiosity, does ToString() emit a warning too? If not, why not?
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4566
to look at the new patch set (#2).
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
[util] Fix a minor bug in AssertEventually()
We want to be sleeping with a monotonically increasing timeout
value in this routine. After 10 attempts, we sleep
for 1000 ms(constant value). But this routine was never
increasing the 'attempts' thus falling back to sleep always at 1ms
intervals making it inefficient.
Also snuck in a change to remove a warning stemming from fresh builds.
Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
---
M src/kudu/common/column_predicate.cc
M src/kudu/util/test_util.cc
2 files changed, 3 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/4566/2
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4566/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 590: default: LOG(FATAL) << "unknown predicate type";
> Hah, I had the same Qn when you suggested me to take a look at this routine
Looking bit more (if my code navigation hasn't cheated me) it turns out LOG() returns a std::ostream&, so that makes sense why we are not seeing the warning there. Also confirmed that removing that LOG introduces a warning.
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4566/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 590: default: LOG(FATAL) << "unknown predicate type";
> Looking bit more (if my code navigation hasn't cheated me) it turns out LOG
The purpose of not putting a default here is so that you get a compiler warning/error if you're missing a case. I think we should leave out the 'default' and put the LOG(FATAL) at the end of the function.
It also looks like we may need a 'return true' for the InList case?
Either way I don't like sneaking this change into an unrelated one.
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4566/1/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 591: return false;
> Hmm. If we add a new PredicateType but forget to update this function, coul
Sure, added FATAL under default case, I guess can't get around without a return bool value here to avoid this warning.
http://gerrit.cloudera.org:8080/#/c/4566/1/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
Line 189: while (MonoTime::Now() < deadline) {
> Might be simpler as a for loop that initializes attempts to 0, checks the d
Done
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4566/2/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 590: default: LOG(FATAL) << "unknown predicate type";
> Out of curiosity, does ToString() emit a warning too? If not, why not?
Hah, I had the same Qn when you suggested me to take a look at this routine, my wild guess was that LOG(FATAL) has some sorta exit code which probably ends up with an implicit typecast to string :). In theory, the warning should be caught on all non-void returns:
warning: control reaches end of non-void function [-Wreturn-type]
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Adar Dembo, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4566
to look at the new patch set (#3).
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
[util] Fix a minor bug in AssertEventually()
We want to be sleeping with a monotonically increasing timeout
value in this routine. After 10 attempts, we sleep
for 1000 ms(constant value). But this routine was never
increasing the 'attempts' thus falling back to sleep always at 1ms
intervals making it inefficient.
Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
---
M src/kudu/util/test_util.cc
1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/4566/3
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4566/1/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:
Line 591: return false;
Hmm. If we add a new PredicateType but forget to update this function, couldn't that lead to really strange errors at runtime? Can we stifle the warning in some other way, perhaps by adding a catch-all LOG(FATAL) as was done in ToString()?
http://gerrit.cloudera.org:8080/#/c/4566/1/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
Line 189: while (MonoTime::Now() < deadline) {
Might be simpler as a for loop that initializes attempts to 0, checks the deadline, and increments attempts.
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
[util] Fix a minor bug in AssertEventually()
We want to be sleeping with a monotonically increasing timeout
value in this routine. After 10 attempts, we sleep
for 1000 ms(constant value). But this routine was never
increasing the 'attempts' thus falling back to sleep always at 1ms
intervals making it inefficient.
Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Reviewed-on: http://gerrit.cloudera.org:8080/4566
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/util/test_util.cc
1 file changed, 1 insertion(+), 2 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 2: Verified-1
Please split out the column_predicate change to a separate patch, since it's unrelated and I think problematic.
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [util] Fix a minor bug in AssertEventually()
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [util] Fix a minor bug in AssertEventually()
......................................................................
Patch Set 3:
> Please split out the column_predicate change to a separate patch,
> since it's unrelated and I think problematic.
Sounds good Todd, will split that out in another patch. Updated this diff/commit_message.
--
To view, visit http://gerrit.cloudera.org:8080/4566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id34b3461164d96261e4a2d1657244332eb33ad0c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No