[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 26 May 2021 02:42:12 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Reviewed-by: Andrew Wong Reviewed-by: Grant Henke --- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 26 May 2021 02:20:56 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 26 May 2021 02:20:24 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 26 May 2021 02:12:43 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 25 May 2021 23:40:16 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 25 May 2021 23:23:28 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 25 May 2021 23:11:07 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 25 May 2021 23:09:23 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 25 May 2021 23:05:34 + Gerrit-HasComments: No
[kudu-CR] [test] minor clean up on TestUnsafeChangeConfigLeaderWithPendingConfig
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 Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 25 May 2021 21:31:16 + Gerrit-HasComments: No
[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