You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2019/10/23 00:21:16 UTC
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14533
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Fixes the FindTabletFollowers() test utility function to query
each tablet server and form the list of followers instead
of simply finding the leader replica and returning supplied
tablet servers with the leader replica removed.
Earlier implementation is incorrect when one or more of the
supplied tablet servers does not host the specified tablet_id.
Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/tools/kudu-admin-test.cc
2 files changed, 97 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/14533/1
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14533
to look at the new patch set (#4).
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Fixes the FindTabletFollowers() test utility function to query
each tablet server and form the list of followers instead
of simply finding the leader replica and returning supplied
tablet servers with the leader replica removed.
Earlier implementation is incorrect when one or more of the
supplied tablet servers does not host the specified tablet_id.
Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/timestamp_advancement-itest.cc
M src/kudu/tools/kudu-admin-test.cc
3 files changed, 159 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/14533/4
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Fixes the FindTabletFollowers() test utility function to query
each tablet server and form the list of followers instead
of simply finding the leader replica and returning supplied
tablet servers with the leader replica removed.
Earlier implementation is incorrect when one or more of the
supplied tablet servers does not host the specified tablet_id.
Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Reviewed-on: http://gerrit.cloudera.org:8080/14533
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/timestamp_advancement-itest.cc
M src/kudu/tools/kudu-admin-test.cc
3 files changed, 160 insertions(+), 15 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 8
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@668
PS1, Line 668: const auto&
const auto*
?
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@694
PS1, Line 694: DCHECK(!cstate.leader_uuid().empty());
In some intermittent states Raft consensus might have no leader. I think in that case this method can return Status::IllegalState().
From the upper level, there should be a way to get eventual success from this method using ASSERT_EVENTUALLY() or alike.
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@719
PS1, Line 719: DCHECK
It would be great if this method worked the same way for both debug and release builds. So, maybe use turn this into 'return Status::InvalidArgument()'?
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@190
PS1, Line 190: TServerDetails *
nit: in Kudu source we tend to stick the asterisk to the type
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@191
PS1, Line 191: MonoDelta::FromSeconds(30)
Maybe, introduce a constant and use it here and at line 193 as well?
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@192
PS1, Line 192: TServerDetails *
ditto
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@193
PS1, Line 193: tablet_servers_
Does it make sense to add a 'negative' test case when the set of supplied tablet servers doesn't contain the whole set of servers which host tablet replicas?
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Oct 2019 02:48:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14533
to look at the new patch set (#7).
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Fixes the FindTabletFollowers() test utility function to query
each tablet server and form the list of followers instead
of simply finding the leader replica and returning supplied
tablet servers with the leader replica removed.
Earlier implementation is incorrect when one or more of the
supplied tablet servers does not host the specified tablet_id.
Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/timestamp_advancement-itest.cc
M src/kudu/tools/kudu-admin-test.cc
3 files changed, 160 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/14533/7
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 4:
> Patch Set 4: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/19286/ : FAILURE
I investigated the test failure AdminCliTest.TestUnsafeChangeConfigFollowerWithPendingConfig and from the logs it appears there was a network error. So the consensus proxy call failed due to network error but didn't fail due to time out error.
Relevant Snippet: https://gist.github.com/bbhavsar/1901e7ffc314de409d22a645068e12a2
From:
http://jenkins.kudu.apache.org/job/kudu-gerrit/19286/BUILD_TYPE=TSAN/
http://jenkins.kudu.apache.org/job/kudu-gerrit/19286/BUILD_TYPE=TSAN/artifact/build/latest/test-logs/kudu-admin-test.0.txt.gz
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 24 Oct 2019 01:29:27 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 5:
> Patch Set 5: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/19288/ : FAILURE
Looks like unrelated HmsSentryConfigurations/MasterStressTest.Test/2 failure.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 24 Oct 2019 04:15:02 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@a649
PS6, Line 649:
:
:
:
:
:
:
:
:
> Could we have done something kind of simple like have FindTabletLeader() re
Ack
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@648
PS6, Line 648: // Fills the out parameter "followers" with the tablet servers hosting the "tablet_id".
: // Non-empty "tablet_servers" is expected to include all the tablet servers and not subset
: // of tablet servers that host the "tablet_id".
> nit: could you add something to the effect of, "Returns an error if the tab
Done
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@667
PS6, Line 667: _or_peers_f
> nit: could you call this tserver_uuid instead? Tablet IDs are also UUIDs so
Done
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@671
PS6, Line 671: return Status::TimedOut(Substitute("Unable to find followers for tablet $0 after $1.",
: tablet_id, (now - start).ToString()));
: }
: const auto& tserver_uuid = entry.first;
: c
> nit: maybe move this up to the top before we do anything in this loop
Done
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@692
PS6, Line 692:
> nit: maybe "current_peer_uuids" or "peer_uuids"? We use UUIDs in a few plac
Done
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@714
PS6, Line 714:
> nit: how about calling this "leader_peer_uuids" or somesuch so it's clear t
Calling it leader_peer_uuids would suggest they are peers as reported by the leader which is not the case here. Instead using peer_uuids along with curr_peer_uuids inside the loop with the existing comment L660 explaining the purpose.
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@736
PS6, Line 736: "Not all peers reported by tablet servers are part of the supplied tablet servers."));
: }
:
: for (const auto& tserver_uuid : peer_uuids) {
: if (tserver_uuid != leader_uuid) {
: followers->emplace_back(FindOrDie(tablet_servers, tserver_uuid));
:
> nit: How about iterating through `tablet_peer_uuids` and using FindOrDie()
Good point. Done
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 24 Oct 2019 17:36:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@694
PS1, Line 694: uuids.emplace(peer.permanent_uuid())
> In some intermittent states Raft consensus might have no leader. I think in that case this method can return Status::IllegalState().
Done without removing this DCHECK. See below.
> From the upper level, there should be a way to get eventual success from this method using ASSERT_EVENTUALLY() or alike.
Done.
> There is already a check above L685 whether the tablet server returned leader uuid. Is it possible that has_leader_uuid() is true but the actual the leader_uuid() is empty?
Checked ConsensusMetadata::ToConsensusStatePB(), there can't be a case where leader is set but it's empty.
> In case of intermediate state when there is no leader for the tablet id, check on L711 would return failure.
Enhanced the error checking and reporting mechanism to differentiate between error due to no tablet server reachable or no tablet server found hosting the tablet_id and no current leader or peers as per consensus.
> So looks like then I should wrap all callers of this function under ASSERT_EVENTUALLY().
Done.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Oct 2019 21:38:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14533
to look at the new patch set (#3).
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Fixes the FindTabletFollowers() test utility function to query
each tablet server and form the list of followers instead
of simply finding the leader replica and returning supplied
tablet servers with the leader replica removed.
Earlier implementation is incorrect when one or more of the
supplied tablet servers does not host the specified tablet_id.
Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/timestamp_advancement-itest.cc
M src/kudu/tools/kudu-admin-test.cc
3 files changed, 153 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/14533/3
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 7: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@694
PS1, Line 694: std::set<string> curr_peer_uuids;
> There is already a check above L685 whether the tablet server returned lead
Indeed. Nope, there should not be the case where has_leader_uuid() is true but leader_uuid() is empty. That would be a bug.
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@694
PS1, Line 694: std::set<string> curr_peer_uuids;
> In case of intermediate state when there is no leader for the tablet id, ch
Yes, it's a good idea.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 24 Oct 2019 18:47:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 6:
(7 comments)
Looks good, mostly nits from me.
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@a649
PS6, Line 649:
:
:
:
:
:
:
:
:
Could we have done something kind of simple like have FindTabletLeader() return the leader's cstate, checked the leader's followers against 'tablet_servers', and then returned the followers? Probably not as robust as what you have here, but another option, I think.
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@648
PS6, Line 648: // Fills the out parameter "followers" with the tablet servers hosting the "tablet_id".
: // Non-empty "tablet_servers" is expected to include all the tablet servers and not subset
: // of tablet servers that host the "tablet_id".
nit: could you add something to the effect of, "Returns an error if the tablet servers do not have consensus or cannot be reached".
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@667
PS6, Line 667: tablet_uuid
nit: could you call this tserver_uuid instead? Tablet IDs are also UUIDs so tablet_uuid could easily be confused
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@671
PS6, Line 671: const auto now = MonoTime::Now();
: if (now > deadline) {
: return Status::TimedOut(Substitute("Unable to find followers for tablet $0 after $1.",
: tablet_id, (now - start).ToString()));
: }
nit: maybe move this up to the top before we do anything in this loop
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@692
PS6, Line 692: uuids
nit: maybe "current_peer_uuids" or "peer_uuids"? We use UUIDs in a few places, so picking a more specific name can really help understandability.
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@714
PS6, Line 714: tablet_peer_uuids
nit: how about calling this "leader_peer_uuids" or somesuch so it's clear that it's related to leader_uuid somehow.
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@736
PS6, Line 736:
: for (const auto& entry : tablet_servers) {
: const auto& uuid = entry.first;
: if (uuid != leader_uuid && ContainsKey(tablet_peer_uuids, uuid)) {
: followers->emplace_back(entry.second);
: }
: }
nit: How about iterating through `tablet_peer_uuids` and using FindOrDie() on `tablet_servers` (though skipping the leader)?
FindOrDie should be guaranteed since we passed the check at L732.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 24 Oct 2019 05:21:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14533/3/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/3/src/kudu/integration-tests/cluster_itest_util.cc@699
PS3, Line 699: DCHECK(!tablet_peer_uuids.empty());
: // Sanity checking that tablet servers with the specified tablet are reporting
: // the same leader and set of peers.
: if (leader_uuid != cstate.leader_uuid()) {
: return Status::IllegalState(Substitute(
: "Leader $0 reported by tablet server $1 for tablet $2 doesn't match with leader $3 "
: "reported by other tablet servers.", cstate.leader_uuid(), tablet_uuid, tablet_id,
: leader_uuid));
> Perhaps I should make these return IllegalState error instead so that it's
Done
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Oct 2019 22:14:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@694
PS1, Line 694: if (!leader_uuid.empty()) {
> There is already a check above L685 whether the tablet server returned lead
In case of intermediate state when there is no leader for the tablet id, check on L711 would return failure.
So looks like then I should wrap all callers of this function under ASSERT_EVENTUALLY().
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Oct 2019 19:16:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@668
PS1, Line 668: const auto&
> const auto*
Done
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@694
PS1, Line 694: DCHECK(!cstate.leader_uuid().empty());
> In some intermittent states Raft consensus might have no leader. I think i
There is already a check above L685 whether the tablet server returned leader uuid. Is it possible that has_leader_uuid() is true but the actual the leader_uuid() is empty?
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@719
PS1, Line 719: DCHECK
> It would be great if this method worked the same way for both debug and rel
Done
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@190
PS1, Line 190: TServerDetails *
> nit: in Kudu source we tend to stick the asterisk to the type
Done
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@191
PS1, Line 191: MonoDelta::FromSeconds(30)
> Maybe, introduce a constant and use it here and at line 193 as well?
Done
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@192
PS1, Line 192: TServerDetails *
> ditto
Done
http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@193
PS1, Line 193: tablet_servers_
> Does it make sense to add a 'negative' test case when the set of supplied t
Added a separate negative test case.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Oct 2019 18:39:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@a649
PS6, Line 649:
:
:
:
:
:
:
:
:
> Another option might be combining both FindTabletLeader() and FindTabletFol
But sure -- that better be done in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 24 Oct 2019 18:27:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@a649
PS6, Line 649:
:
:
:
:
:
:
:
:
> Ack
Another option might be combining both FindTabletLeader() and FindTabletFollowers() into something like
FindTabletReplicas(const TabletServerMap& tablet_servers, const string& tablet_id, const MonoDelta& timeout, TServerDetails* leader, vector<TServerDetails*>* followers);
where both output parameters are nullable.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 24 Oct 2019 18:25:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14533/3/src/kudu/integration-tests/cluster_itest_util.cc
File src/kudu/integration-tests/cluster_itest_util.cc:
http://gerrit.cloudera.org:8080/#/c/14533/3/src/kudu/integration-tests/cluster_itest_util.cc@699
PS3, Line 699: // Sanity checking that tablet servers with the specified tablet are reporting
: // the same leader and set of peers.
: DCHECK_EQ(leader_uuid, cstate.leader_uuid()) <<
: Substitute("Leader reported by tablet server $0 for tablet $1 doesn't match.",
: tablet_uuid, tablet_id);
: DCHECK(tablet_peer_uuids == uuids) <<
: Substitute("Peers reported by tablet server $0 for tablet $1 don't match.",
: tablet_uuid, tablet_id);
Perhaps I should make these return IllegalState error instead so that it's caught against release builds as well.
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Oct 2019 21:55:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14533
to look at the new patch set (#2).
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
......................................................................
KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function
Fixes the FindTabletFollowers() test utility function to query
each tablet server and form the list of followers instead
of simply finding the leader replica and returning supplied
tablet servers with the leader replica removed.
Earlier implementation is incorrect when one or more of the
supplied tablet servers does not host the specified tablet_id.
Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/tools/kudu-admin-test.cc
2 files changed, 118 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/14533/2
--
To view, visit http://gerrit.cloudera.org:8080/14533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8
Gerrit-Change-Number: 14533
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)