[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Reviewed-by: Mike Percy --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [util] Fix a minor bug in AssertEventually()
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
