You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2021/05/25 07:49:15 UTC
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17504
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
[test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
When looking at one of recent pre-commit test failure [1][2], I found
that AdminCliTest.TestUnsafeChangeConfigLeaderWithPendingConfig failed
but it was hard to tell what was the actual status code returned:
src/kudu/tools/kudu-admin-test.cc:881: Failure
Value of: s.IsTimedOut()
Actual: false
Expected: true
I reran the scenario multiple times with TSAN-enabled binaries, but
was not able to reproduce the test failure yet. Anyways, I added more
diagnostics about the actual status code returned and cleaned up the
code of the test scenario a bit. Hopefully, next time it fails it
will be clearer what's going on.
[1] http://jenkins.kudu.apache.org/job/kudu-gerrit/23918/BUILD_TYPE=TSAN
[2] http://dist-test.cloudera.org/job?job_id=jenkins-slave.1621893605.3746155
Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 7 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/17504/1
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1: Code-Review+1
LGTM, don't have much to add to the discussion about removing LOG statements, if they're duplicated info I'm ok with removing them.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 May 2021 23:40:16 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 May 2021 02:20:24 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1:
> > Patch Set 1:
> >
> > > Why remove those info log lines?
> >
> > Because they are useless in an automated tests. They might make
> sense during developing of the test, but there isn't much sense
> keeping them since nobody looks at them during regular runs. If a
> test scenario fails, the information in the assert messages should
> be enough to start troubleshooting.
>
> They could help debug in case of a test failure. I'm not convinced
> removing log lines is the right thing to do unless it's causing log
> spew and making debugging even more difficult.
Nope, they could not. As you can see, many of those duplicates what's output with assertions, and for the case I was looking at they are completely useless.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 May 2021 23:11:07 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1:
> > Patch Set 1:
> >
> > > > Patch Set 1:
> > > >
> > > > > Why remove those info log lines?
> > > >
> > > > Because they are useless in an automated tests. They might
> make
> > > sense during developing of the test, but there isn't much
> sense
> > > keeping them since nobody looks at them during regular runs.
> If a
> > > test scenario fails, the information in the assert messages
> should
> > > be enough to start troubleshooting.
> > >
> > > They could help debug in case of a test failure. I'm not
> convinced
> > > removing log lines is the right thing to do unless it's
> causing log
> > > spew and making debugging even more difficult.
> >
> > Nope, they could not. As you can see, many of those duplicates
> what's output with assertions, and for the case I was looking at
> they are completely useless.
>
> It may not help in this case but consider an example of someone new
> trying to debug a test issue, instead of trying to co-relate with
> the code such log messages can serve as marker points.
>
> I won't try to argue further. One of those cases where we can agree
> to disagree and move on :).
OK, this makes sense to me.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 May 2021 02:12:43 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > > Patch Set 1:
> > >
> > > > Why remove those info log lines?
> > >
> > > Because they are useless in an automated tests. They might make
> > sense during developing of the test, but there isn't much sense
> > keeping them since nobody looks at them during regular runs. If a
> > test scenario fails, the information in the assert messages should
> > be enough to start troubleshooting.
> >
> > They could help debug in case of a test failure. I'm not convinced
> > removing log lines is the right thing to do unless it's causing log
> > spew and making debugging even more difficult.
>
> Nope, they could not. As you can see, many of those duplicates what's output with assertions, and for the case I was looking at they are completely useless.
It may not help in this case but consider an example of someone new trying to debug a test issue, instead of trying to co-relate with the code such log messages can serve as marker points.
I won't try to argue further. One of those cases where we can agree to disagree and move on :).
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 May 2021 23:23:28 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1: Code-Review+2
I am not against logging in tests in general as I do often find it a useful way to track/debug what's happening. That said if this info has proven unhelpful I am okay with removing it.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 May 2021 02:20:56 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1:
> I am not against logging in tests in general as I do often find it
> a useful way to track/debug what's happening. That said if this
> info has proven unhelpful I am okay with removing it.
Thank you for the review.
I removed those INFO logs because I've seen many times they are useless when trying to debug a test scenaio which fails. This is one of those, and here I saw the same pattern. At least in this test case, those logs either duplicate information which might later appear in the output of an assertion, or that's some information that's irrelevant when some other assertion fires. Yes, those might serve as markers as pointed out by Bankim, but the logging from masters and tablet servers are good enough markers already, IMO.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 May 2021 02:42:12 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
[test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
When looking at one of recent pre-commit test failure [1][2], I found
that AdminCliTest.TestUnsafeChangeConfigLeaderWithPendingConfig failed
but it was hard to tell what was the actual status code returned:
src/kudu/tools/kudu-admin-test.cc:881: Failure
Value of: s.IsTimedOut()
Actual: false
Expected: true
I reran the scenario multiple times with TSAN-enabled binaries, but
was not able to reproduce the test failure yet. Anyways, I added more
diagnostics about the actual status code returned and cleaned up the
code of the test scenario a bit. Hopefully, next time it fails it
will be clearer what's going on.
[1] http://jenkins.kudu.apache.org/job/kudu-gerrit/23918/BUILD_TYPE=TSAN
[2] http://dist-test.cloudera.org/job?job_id=jenkins-slave.1621893605.3746155
Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Reviewed-on: http://gerrit.cloudera.org:8080/17504
Tested-by: Kudu Jenkins
Reviewed-by: Mahesh Reddy <mr...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
---
M src/kudu/tools/kudu-admin-test.cc
1 file changed, 7 insertions(+), 13 deletions(-)
Approvals:
Kudu Jenkins: Verified
Mahesh Reddy: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > Why remove those info log lines?
>
> Because they are useless in an automated tests. They might make sense during developing of the test, but there isn't much sense keeping them since nobody looks at them during regular runs. If a test scenario fails, the information in the assert messages should be enough to start troubleshooting.
They could help debug in case of a test failure. I'm not convinced removing log lines is the right thing to do unless it's causing log spew and making debugging even more difficult.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 May 2021 23:09:23 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1:
Why remove those info log lines?
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 May 2021 21:31:16 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Patch Set 1:
> Why remove those info log lines?
Because they are useless in an automated tests. They might make sense during developing of the test, but there isn't much sense keeping them since nobody looks at them during regular runs. If a test scenario fails, the information in the assert messages should be enough to start troubleshooting.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 May 2021 23:05:34 +0000
Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Bankim Bhavsar from this change. ( http://gerrit.cloudera.org:8080/17504 )
Change subject: [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
......................................................................
Removed reviewer Bankim Bhavsar.
--
To view, visit http://gerrit.cloudera.org:8080/17504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Id2bf354764f3ddb371de19910c5f879c1a06c78b
Gerrit-Change-Number: 17504
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>