[kudu-CR] [location awareness] Add ts location to TSInfoPB
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11727 to look at the new patch set (#5). Change subject: [location_awareness] Add ts location to TSInfoPB .. [location_awareness] Add ts location to TSInfoPB I added the 'location' field into TSInfoPB so that the tool client can get the ts location info. Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259 --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/scripts/first_argument.sh M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 5 files changed, 130 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/11727/5 -- To view, visit http://gerrit.cloudera.org:8080/11727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259 Gerrit-Change-Number: 11727 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add ts location to both client and internal client
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: [location_awareness] Add ts location to both client and internal client > Thanks, Adar and Alexey for the suggestions! One thing I'm not very clear, Ah, it seems a typo in my last comment was the source of the confusion. I meant '[client] expose location in KuduTabletServer via private API'. Maybe, a better wording might be: '[client] expose location via internal API' And then add the detailed description. And yes, since the location is needed for both KuduTabletServer and RemoteTabletServer, it's needed for both, I think. -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 24 Oct 2018 05:23:14 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] SentryAuthzProvider
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc
File src/kudu/sentry/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@121
PS3, Line 121: SentryAuthzProviderTest
Is it worth adding a few scenarios to verify how the provider works if Sentry
server is unreachable?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc
File src/kudu/sentry/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@121
PS3, Line 121: Status SentryAuthzProvider::AuthorizeAlterTable(const string&
table_name,
: const string&
user,
: bool* authorized)
nit: consider either setting '*authorized' to 'false' in the beginning or add
WARN_UNUSED_RESULT attribute for those methods; that's to avoid mistakes of
using the result '*authorized' if methods like this return not-OK prematurely.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@222
PS3, Line 222: Status SentryAuthzProvider::ParseAddresses
nit: it would be nice if the order of methods in the .cc file matched the order
of those in the .h file.
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@218
PS3, Line 218: request.__set_principalName("test-user");
I'm curious what would be the response if not setting (or clearing/resetting)
this field?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 24 Oct 2018 05:13:20 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to TSInfoPB
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11727 to look at the new patch set (#4). Change subject: [location_awareness] Add ts location to TSInfoPB .. [location_awareness] Add ts location to TSInfoPB I added the 'location' field into TSInfoPB so that the tool client can get the ts location info. Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259 --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/scripts/first_argument.sh M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 5 files changed, 130 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/11727/4 -- To view, visit http://gerrit.cloudera.org:8080/11727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259 Gerrit-Change-Number: 11727 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add ts location to both client and internal client
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > Thank you for the clarification, Adar! Thanks, Adar and Alexey for the suggestions! One thing I'm not very clear, so do I expose location via private API for both KuduTabletServer and RemoteTabletServer? -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 24 Oct 2018 04:36:46 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to TSInfoPB
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11727 to look at the new patch set (#3). Change subject: [location_awareness] Add ts location to TSInfoPB .. [location_awareness] Add ts location to TSInfoPB I added the 'location' field into TSInfoPB so that the tool client can get the ts location info. Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259 --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/scripts/first_argument.sh M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 5 files changed, 130 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/11727/3 -- To view, visit http://gerrit.cloudera.org:8080/11727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259 Gerrit-Change-Number: 11727 Gerrit-PatchSet: 3 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add ts location to TSInfoPB
Fengling Wang has posted comments on this change. (
http://gerrit.cloudera.org:8080/11727 )
Change subject: [location_awareness] Add ts location to TSInfoPB
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@127
PS2, Line 127: properply
> Nit: properly
Done
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@128
PS2, Line 128: class TableLocationsWithTSLocationTest : public KuduTest {
> You should be able to extend the existing test fixture; just add a virtual
Done
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@138
PS2, Line 138:
"scripts/first_argument.sh");
> Nit: indentation
Done
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@301
PS2, Line 301: req.set_max_returned_locations(3);
> Is there any significance of how many returned locations are there?
Done
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@307
PS2, Line 307: EXPECT_EQ("",
resp.tablet_locations(0).replicas(0).ts_info().location());
> What about checking additional 2 locations?
Done
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@315
PS2, Line 315:
: { // Check that the master doesn't give back partial results
while the table is being created.
: GetTableLocationsRequestPB req;
: GetTableLocationsResponsePB resp;
: RpcController controller;
: req.mutable_table()->set_table_name(table_name);
:
: for (int i = 1; ; i++) {
: if (i > 10) {
: FAIL() << "Create table timed out";
: }
:
: controller.Reset();
: ASSERT_OK(proxy_->GetTableLocations(req, ,
));
: SCOPED_TRACE(SecureDebugString(resp));
:
: if (resp.has_error()) {
: ASSERT_EQ(MasterErrorPB::TABLET_NOT_RUNNING,
resp.error().code());
: SleepFor(MonoDelta::FromMilliseconds(i * i * 100));
: } else {
: ASSERT_EQ(1, resp.tablet_locations().size());
: break;
: }
: }
: }
> OK, then please encapsulate it into a common helper method. And maybe combi
Done
http://gerrit.cloudera.org:8080/#/c/11727/2/src/kudu/integration-tests/table_locations-itest.cc@350
PS2, Line 350: ASSERT_EQ("/foo",
resp.tablet_locations(0).replicas(0).ts_info().location());
> Does it make sense to add an additional assert on expected size of the tabl
Done
--
To view, visit http://gerrit.cloudera.org:8080/11727
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5865cb47698817097a7f62c968c5cfd80e07259
Gerrit-Change-Number: 11727
Gerrit-PatchSet: 2
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Wed, 24 Oct 2018 04:18:26 +
Gerrit-HasComments: Yes
[kudu-CR] [master] update placement logic for RF % 2 == 0
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11763 to look at the new patch set (#2). Change subject: [master] update placement logic for RF % 2 == 0 .. [master] update placement logic for RF % 2 == 0 Updated the logic of the replica placement in master to properly handle even replication factors. Added corresponding unit tests as well. It's possible to configure Kudu to allow creation of tables with an even replication factor. I think it's easier to implement the handling of those cases instead of handling possible inconsistencies in the rebalancer and other tools and adding extra paragraphs into release notes. Change-Id: I4259f805090dd350f31bf3bf2b6477898214ece4 --- M src/kudu/master/placement_policy-test.cc M src/kudu/master/placement_policy.cc M src/kudu/master/placement_policy.h 3 files changed, 137 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/11763/2 -- To view, visit http://gerrit.cloudera.org:8080/11763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4259f805090dd350f31bf3bf2b6477898214ece4 Gerrit-Change-Number: 11763 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [master] update placement logic for RF % 2 == 0
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11763 Change subject: [master] update placement logic for RF % 2 == 0 .. [master] update placement logic for RF % 2 == 0 Updated replica placement logic to properly handle even replication factors. Added corresponding unit tests as well. It's possible to configure Kudu to allow creation of tables with even replication factor. I think it's easier to implement the handling of those cases instead of adding paragraphs into relase notes and handling possible inconsistencies in the rebalancer and other tools. Change-Id: I4259f805090dd350f31bf3bf2b6477898214ece4 --- M src/kudu/master/placement_policy-test.cc M src/kudu/master/placement_policy.cc M src/kudu/master/placement_policy.h 3 files changed, 137 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/11763/1 -- To view, visit http://gerrit.cloudera.org:8080/11763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4259f805090dd350f31bf3bf2b6477898214ece4 Gerrit-Change-Number: 11763 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] [rebalancer] location-aware rebalancer (part 8/n)
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11761 Change subject: [rebalancer] location-aware rebalancer (part 8/n) .. [rebalancer] location-aware rebalancer (part 8/n) Updated FindBestReplicaToReplace() to handle common and edge cases of even replication factors. Added corresponding unit tests as well. Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de --- M src/kudu/tools/placement_policy_util-test.cc M src/kudu/tools/placement_policy_util.cc 2 files changed, 203 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/11761/1 -- To view, visit http://gerrit.cloudera.org:8080/11761 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de Gerrit-Change-Number: 11761 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. KUDU-1678: Race during abort of pending operations during raft shutdown When a tablet replica is shutting down, the following race can occur: 0: The replica receives an ALTER_SCHEMA op adding the column 'foo'. 1: The replica receives a WRITE_OP inserting a row with column 'foo' present. 2: The replica starts to abort its pending operations because it is shutting down. The ALTER_SCHEMA is aborted. 3: Before the WRITE_OP can be aborted, it replicates. 4: The tablet server crashes as a result, with a message like: F1023 20:49:18.088703 1409 transaction_driver.cc:382] T 3fe5651e31d8486780d0e480f8748ead P 12e38846d8c648df8a11b9d8da2ad407 S R-NP Ts 6309182492379934720: Cannot cancel transactions that have already replicated: Invalid argument: Client provided column c587 INT32 NOT NULL not present in tablet The tablet server will crash with this same message every time it tries to bootstrap. Other tablet servers hosting replicas may crash if they replicated the bad write. The problem is that transactions need to be aborted in reverse order of their index, since later transactions may depend on earlier ones. This bug reproduced about 1% of the time in alter_table-randomized-test when run in DEBUG mode with 16 stress threads: I saw it 10 times in 1000 runs. With this change, I saw 0 failures in 1000 runs. Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Reviewed-on: http://gerrit.cloudera.org:8080/11759 Reviewed-by: Alexey Serbin Tested-by: Will Berkeley --- M src/kudu/consensus/pending_rounds.cc M src/kudu/integration-tests/alter_table-randomized-test.cc 2 files changed, 23 insertions(+), 13 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Will Berkeley: Verified -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 23:25:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Will Berkeley has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 22:45:47 + Gerrit-HasComments: No
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/11759/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11759/1//COMMIT_MSG@9 PS1, Line 9: When a tablet replica is shutting down, the following race can occur: > I'm kind of surprised that we don't "quiesce" a tablet during shutdown so t Yeah, that would work too. http://gerrit.cloudera.org:8080/#/c/11759/1//COMMIT_MSG@17 PS1, Line 17: 4: The tablet server crashes as a result, with a message like: > Specifically, a different tserver crashes, right? The one hosting the FOLLO I think it can cause multiple to crash-- which servers end up with the bad write in their log. http://gerrit.cloudera.org:8080/#/c/11759/1/src/kudu/consensus/pending_rounds.cc File src/kudu/consensus/pending_rounds.cc: http://gerrit.cloudera.org:8080/#/c/11759/1/src/kudu/consensus/pending_rounds.cc@66 PS1, Line 66: tions in reverse index order. See KUDU-1678. > nit: why not to use crbegin() and crend() if the elements of the container Done http://gerrit.cloudera.org:8080/#/c/11759/1/src/kudu/consensus/pending_rounds.cc@67 PS1, Line 67: pending_txns > nit: does txn->second look any better? Done -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 22:41:55 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Hello Mike Percy, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11759 to look at the new patch set (#2). Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. KUDU-1678: Race during abort of pending operations during raft shutdown When a tablet replica is shutting down, the following race can occur: 0: The replica receives an ALTER_SCHEMA op adding the column 'foo'. 1: The replica receives a WRITE_OP inserting a row with column 'foo' present. 2: The replica starts to abort its pending operations because it is shutting down. The ALTER_SCHEMA is aborted. 3: Before the WRITE_OP can be aborted, it replicates. 4: The tablet server crashes as a result, with a message like: F1023 20:49:18.088703 1409 transaction_driver.cc:382] T 3fe5651e31d8486780d0e480f8748ead P 12e38846d8c648df8a11b9d8da2ad407 S R-NP Ts 6309182492379934720: Cannot cancel transactions that have already replicated: Invalid argument: Client provided column c587 INT32 NOT NULL not present in tablet The tablet server will crash with this same message every time it tries to bootstrap. Other tablet servers hosting replicas may crash if they replicated the bad write. The problem is that transactions need to be aborted in reverse order of their index, since later transactions may depend on earlier ones. This bug reproduced about 1% of the time in alter_table-randomized-test when run in DEBUG mode with 16 stress threads: I saw it 10 times in 1000 runs. With this change, I saw 0 failures in 1000 runs. Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 --- M src/kudu/consensus/pending_rounds.cc M src/kudu/integration-tests/alter_table-randomized-test.cc 2 files changed, 23 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11759/2 -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 23 Oct 2018 22:40:34 + Gerrit-HasComments: No
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11759/1/src/kudu/consensus/pending_rounds.cc File src/kudu/consensus/pending_rounds.cc: http://gerrit.cloudera.org:8080/#/c/11759/1/src/kudu/consensus/pending_rounds.cc@66 PS1, Line 66: pending_txns_.rbegin(); txn != pending_txns_.rend() nit: why not to use crbegin() and crend() if the elements of the container are not changed anyway? http://gerrit.cloudera.org:8080/#/c/11759/1/src/kudu/consensus/pending_rounds.cc@67 PS1, Line 67: (*txn).second nit: does txn->second look any better? -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 23 Oct 2018 22:35:38 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11759 ) Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. Patch Set 1: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/11759/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11759/1//COMMIT_MSG@9 PS1, Line 9: When a tablet replica is shutting down, the following race can occur: I'm kind of surprised that we don't "quiesce" a tablet during shutdown so that its pending transactions can't make forward progress as we cancel them. http://gerrit.cloudera.org:8080/#/c/11759/1//COMMIT_MSG@17 PS1, Line 17: 4: The tablet server crashes as a result, with a message like: Specifically, a different tserver crashes, right? The one hosting the FOLLOWER of this tablet, the recipient of the replication? Although the example in the bug report doesn't involve replication; just scheduling of the uncanceled write on the local tserver's prepare threadpool. -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 23 Oct 2018 22:33:25 + Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-2542: add initial authorization token impl
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11750 ) Change subject: WIP KUDU-2542: add initial authorization token impl .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto File src/kudu/security/token.proto: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@49 PS1, Line 49: optional bool update_privilege = 5; We don't support authz on per-column basis for update, right? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@64 PS1, Line 64: optional > I think it is one table per AuthzToken. Add a comment here to clarify that? That was a generic question regarding current authz token design and whether it's about to evolve in the future. I understand that current implementation assumes a single table privilege per token. The question is whether it's anticipated to change in the future, and if yes, how we are about to expand this message. Probably, it's not worth thinking about that now, and if needed, we can always update the message and introduce new fields and obsolete old ones when it's needed. -- To view, visit http://gerrit.cloudera.org:8080/11750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id28747ec38675abdf50dce1e7c176d29213e370f Gerrit-Change-Number: 11750 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Oct 2018 22:24:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1678: Race during abort of pending operations during raft shutdown
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11759 Change subject: KUDU-1678: Race during abort of pending operations during raft shutdown .. KUDU-1678: Race during abort of pending operations during raft shutdown When a tablet replica is shutting down, the following race can occur: 0: The replica receives an ALTER_SCHEMA op adding the column 'foo'. 1: The replica receives a WRITE_OP inserting a row with column 'foo' present. 2: The replica starts to abort its pending operations because it is shutting down. The ALTER_SCHEMA is aborted. 3: Before the WRITE_OP can be aborted, it replicates. 4: The tablet server crashes as a result, with a message like: F1023 20:49:18.088703 1409 transaction_driver.cc:382] T 3fe5651e31d8486780d0e480f8748ead P 12e38846d8c648df8a11b9d8da2ad407 S R-NP Ts 6309182492379934720: Cannot cancel transactions that have already replicated: Invalid argument: Client provided column c587 INT32 NOT NULL not present in tablet The tablet server will crash with this same message every time it tries to bootstrap. The problem is that transactions need to be aborted in reverse order of their index, since later transactions may depend on earlier ones. This bug reproduced about 1% of the time in alter_table-randomized-test when run in DEBUG mode with 16 stress threads: I saw it 10 times in 1000 runs. With this change, I saw 0 failures in 1000 runs. Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 --- M src/kudu/consensus/pending_rounds.cc M src/kudu/integration-tests/alter_table-randomized-test.cc 2 files changed, 22 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11759/1 -- To view, visit http://gerrit.cloudera.org:8080/11759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Idde75bd1fe966a1a3d53aa1e5de6a01a48ff1103 Gerrit-Change-Number: 11759 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
[kudu-CR] WIP KUDU-2543: pass around default authz tokens
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 ) Change subject: WIP KUDU-2543: pass around default authz tokens .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h File src/kudu/rpc/rpc_verification_util.h: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h@40 PS1, Line 40: with a new token This comment looks very specific to me since there is no 'token' concept defined in the scope of this method. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h@41 PS1, Line 41: ErrorForVerification nit: name it ParseVerificationResult to be more clear? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@388 PS1, Line 388: FATAL_INVALID_AUTHORIZATION_TOKEN > I would expect it to be FATAL_UNAUTHORIZED instead. Could you add the expl Does FATAL_UNAUTHORIZED intent to cover both authentication and authorization failure? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tserver.proto@307 PS1, Line 307: ScanRequestPB Add SignedTokenPB for ChecksumRequestPB in tserver.proto as well? -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Oct 2018 22:12:36 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > I wasn't aware of the RemoteKsckCluster use case. Given that, I agree that Thank you for the clarification, Adar! I agree it's crucial to have the right wording in the commit message regarding the change. Fengling, could you address that? I think something like '[client] expose location in RemoteTabletServer via private API' could be good enough summary (i.e. the first line of the commit message). -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 21:34:56 + Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-2542: add initial authorization token impl
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11750 ) Change subject: WIP KUDU-2542: add initial authorization token impl .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto File src/kudu/security/token.proto: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token.proto@64 PS1, Line 64: optional > Do we ever need to have privileges for multiple tables in one authz token? I think it is one table per AuthzToken. Add a comment here to clarify that? -- To view, visit http://gerrit.cloudera.org:8080/11750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id28747ec38675abdf50dce1e7c176d29213e370f Gerrit-Change-Number: 11750 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Oct 2018 20:55:40 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: [location_awareness] Add ts location to both client and internal client > Probably, that has already been discussed, and if so, I just want to have i I wasn't aware of the RemoteKsckCluster use case. Given that, I agree that exposing location in KuduTabletServer with KUDU_NO_EXPORT makes the most sense. That said, this commit message should avoid saying that the ts location is being added to the public API. -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 20:35:20 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 3/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11744 ) Change subject: [rebalancer] location-aware rebalancer (part 3/n) .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie89f1958238dbc28b44111038d4b82f49f824ca9 Gerrit-Change-Number: 11744 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 20:33:36 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 2/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11743 ) Change subject: [rebalancer] location-aware rebalancer (part 2/n) .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Gerrit-Change-Number: 11743 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 20:33:24 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add ts location to both client and internal client
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > That make sense, but then I don't think you need to expose location() in cl Probably, that has already been discussed, and if so, I just want to have it in the review as well. It seems the idea was to use it in https://gerrit.cloudera.org/#/c/11422/11/src/kudu/tools/ksck_remote.cc@487 Is that acceptable to have it exposed in this patch or we want to have a separate patch with exposing the location via kudu::client::KuduTabletServer API (not exported, though)? Or another alternative would be not exposing the location via a public method of KuduTabletServer (but in internal-only API); instead, befriend RemoteKsckCluster and access the location field in src/kudu/tools/ksck_remote.cc via the KuduTabletServer::Data class? Adar, any preference on the items above? -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 20:24:10 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add location info in ksck report
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 ) Change subject: [location_awareness] Add location info in ksck report .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/11422/11/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/11422/11/src/kudu/tools/ksck.h@388 PS11, Line 388: std::string location_ nit: it seems this field is set only in the constructor and is not changing during lifetime of the objects, so it makes sense to add 'const' specifier. -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 11 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 20:24:19 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11748 ) Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. Patch Set 2: Verified+1 Unrelated flake in AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown (ASAN). -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 19:36:09 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11748 ) Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] catalog manager: adjust maximum tablet count at table creation time
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11716 ) Change subject: catalog_manager: adjust maximum tablet count at table creation time .. catalog_manager: adjust maximum tablet count at table creation time The maximum tablet check introduced in commit 64983c454 was incomplete in that it only considered the number of _tablets_ rather than the number of _replicas_, even though replica creation itself places load on a tserver during table creation. This patch clarifies the intended behavior and incorporates the replication factor of the table into the check. Note: users who override --max_create_tablets_per_ts are in for a rude surprise as the maximum size of their tables will be effectively cut in three. Given that the parameter isn't tagged as "stable" though, I think communicating this change in behavior via release note is sufficient. Change-Id: I00d91baddb1591476a7be27cba043e6354558208 Reviewed-on: http://gerrit.cloudera.org:8080/11716 Reviewed-by: Dan Burkert Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M docs/known_issues.adoc M src/kudu/client/client-test.cc M src/kudu/integration-tests/raft_consensus_stress-itest.cc M src/kudu/master/catalog_manager.cc 4 files changed, 40 insertions(+), 26 deletions(-) Approvals: Dan Burkert: Looks good to me, but someone else must approve Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11716 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I00d91baddb1591476a7be27cba043e6354558208 Gerrit-Change-Number: 11716 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] catalog manager: adjust maximum tablet count at table creation time
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11716 ) Change subject: catalog_manager: adjust maximum tablet count at table creation time .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11716 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00d91baddb1591476a7be27cba043e6354558208 Gerrit-Change-Number: 11716 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 19:08:35 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 6/n)
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11747 )
Change subject: [rebalancer] location-aware rebalancer (part 6/n)
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.h
File src/kudu/tools/tool_replica_util.h:
http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.h@121
PS1, Line 121: is
> Remove.
Done
http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.cc
File src/kudu/tools/tool_replica_util.cc:
http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.cc@415
PS1, Line 415: if (is_ts_uuid_replace_attr_set) {
: // Nothing to do: the replica is already marked with the
REPLACE attribute.
: return Status::OK();
: }
> Why not return from the loop body?
That's a good idea.
http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.cc@435
PS1, Line 435: if (resp.error().code() ==
tserver::TabletServerErrorPB::CAS_FAILED &&
: cas_failed) {
: *cas_failed = true;
: }
> I think we should explicitly set cas_failed=false if cas_failed is non-null
Yup, that's a good catch.
http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.cc@460
PS1, Line 460: // Check if the replica with UUID 'to_ts_uuid' is in the
config, and if it has
: // been promoted to voter.
It's a stale comment, I removed it.
http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.cc@494
PS1, Line 494: is_all_voters
> Why do we need to check this?
I thought making current leader step-down before the non-voter is promoted in
some cases might needlessly delay the promotion of already caught-up replica.
Also, the former leader is not going to be evicted anyway before the non-voter
become a voter. In addition, de-moting the former leader at this point
slightly increases chances of the former leader to be re-elected in case of
network issues and RPC queue overflows.
>From the other side, I don't see a lot of benefits demoting current leader
>early.
I added a comment. If you think there is something wrong with those arguments,
I can remove that logic.
--
To view, visit http://gerrit.cloudera.org:8080/11747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31
Gerrit-Change-Number: 11747
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 23 Oct 2018 19:05:32 +
Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11748 ) Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/rebalancer.h File src/kudu/tools/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/rebalancer.h@326 PS1, Line 326: If 'location' is boost::none, rebalance across : // locations. > The idea is that this balances within a location if 'location' is not boost Woops, that's just a stale comment. I updated it. http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/rebalancer.cc File src/kudu/tools/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/rebalancer.cc@257 PS1, Line 257: > in Done http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/rebalancer.cc@289 PS1, Line 289: // TODO(aserbin): it would be nice to run these rebalancers in parallel > Yeah, I think that's a good idea. But also worth saving for a followup-once Ack. http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/rebalancer_tool-test.cc File src/kudu/tools/rebalancer_tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/rebalancer_tool-test.cc@1170 PS1, Line 1170: This tests > These tests are Done http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11748/1/src/kudu/tools/tool_action_cluster.cc@107 PS1, Line 107: envolves > involves Done -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 19:05:28 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 6/n)
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11747 to look at the new patch set (#2). Change subject: [rebalancer] location-aware rebalancer (part 6/n) .. [rebalancer] location-aware rebalancer (part 6/n) Added SetReplace() and CheckCompleteReplace() auxiliary fuctions. A follow-up patch will start using those. Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 --- M src/kudu/tools/tool_replica_util.cc M src/kudu/tools/tool_replica_util.h 2 files changed, 157 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/11747/2 -- To view, visit http://gerrit.cloudera.org:8080/11747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 Gerrit-Change-Number: 11747 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Hello Will Berkeley, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11748 to look at the new patch set (#2). Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. [rebalancer] location-aware rebalancer (part 7/n) Added PolicyFixer and integrated the cross-location balancing algorithm. Added one integration test as well, but it's disabled for a now since it requires a tablet server's location to be reported in KsckServerHealthSummary. If that piece is in place, the test passes. More integration tests will be added in a follow-up commit. Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 --- M src/kudu/tools/rebalancer.cc M src/kudu/tools/rebalancer.h M src/kudu/tools/rebalancer_tool-test.cc M src/kudu/tools/tool_action_cluster.cc 4 files changed, 709 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/11748/2 -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] WIP: Add mini Sentry to the external mini cluster
Hello Andrew Wong, Hao Hao, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11756 to review the following change. Change subject: WIP: Add mini Sentry to the external mini cluster .. WIP: Add mini Sentry to the external mini cluster This includes some changes to turn on remote debugging of the Sentry and HMS processes marked with TODOs; these should be removed before committing. Change-Id: I312e7008983c4505ef0db270ee4427c3a236b50b --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry_client-test.cc M src/kudu/thrift/client.h M src/kudu/util/test_util.cc 11 files changed, 227 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11756/1 -- To view, visit http://gerrit.cloudera.org:8080/11756 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I312e7008983c4505ef0db270ee4427c3a236b50b Gerrit-Change-Number: 11756 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao
[kudu-CR] catalog manager: adjust maximum tablet count at table creation time
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11716 ) Change subject: catalog_manager: adjust maximum tablet count at table creation time .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11716 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00d91baddb1591476a7be27cba043e6354558208 Gerrit-Change-Number: 11716 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 18:41:40 + Gerrit-HasComments: No
[kudu-CR] catalog manager: adjust maximum tablet count at table creation time
Hello Will Berkeley, Alexey Serbin, Dan Burkert, Jean-Daniel Cryans, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11716 to look at the new patch set (#5). Change subject: catalog_manager: adjust maximum tablet count at table creation time .. catalog_manager: adjust maximum tablet count at table creation time The maximum tablet check introduced in commit 64983c454 was incomplete in that it only considered the number of _tablets_ rather than the number of _replicas_, even though replica creation itself places load on a tserver during table creation. This patch clarifies the intended behavior and incorporates the replication factor of the table into the check. Note: users who override --max_create_tablets_per_ts are in for a rude surprise as the maximum size of their tables will be effectively cut in three. Given that the parameter isn't tagged as "stable" though, I think communicating this change in behavior via release note is sufficient. Change-Id: I00d91baddb1591476a7be27cba043e6354558208 --- M docs/known_issues.adoc M src/kudu/client/client-test.cc M src/kudu/integration-tests/raft_consensus_stress-itest.cc M src/kudu/master/catalog_manager.cc 4 files changed, 40 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/11716/5 -- To view, visit http://gerrit.cloudera.org:8080/11716 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00d91baddb1591476a7be27cba043e6354558208 Gerrit-Change-Number: 11716 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11746 ) Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11746 ) Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. Patch Set 2: Verified+1 Unrelated flake in AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown (ASAN) -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 18:26:51 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 4/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11745 ) Change subject: [rebalancer] location-aware rebalancer (part 4/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Gerrit-Change-Number: 11745 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 4/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11745 ) Change subject: [rebalancer] location-aware rebalancer (part 4/n) .. Patch Set 2: Verified+1 Unrelated flake in org.apache.kudu.spark.kudu.DefaultSourceTest -- To view, visit http://gerrit.cloudera.org:8080/11745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Gerrit-Change-Number: 11745 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 18:26:22 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 3/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11744 ) Change subject: [rebalancer] location-aware rebalancer (part 3/n) .. Patch Set 2: Verified+1 Unrelated flake in AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown (ASAN). -- To view, visit http://gerrit.cloudera.org:8080/11744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie89f1958238dbc28b44111038d4b82f49f824ca9 Gerrit-Change-Number: 11744 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 18:25:38 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 1/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11549 ) Change subject: [rebalancer] location-aware rebalancer (part 1/n) .. Patch Set 7: Verified+1 Unrelated flake in AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown (ASAN) -- To view, visit http://gerrit.cloudera.org:8080/11549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 Gerrit-Change-Number: 11549 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 18:25:02 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 2/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11743 ) Change subject: [rebalancer] location-aware rebalancer (part 2/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Gerrit-Change-Number: 11743 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 1/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11549 ) Change subject: [rebalancer] location-aware rebalancer (part 1/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 Gerrit-Change-Number: 11549 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 2/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11743 ) Change subject: [rebalancer] location-aware rebalancer (part 2/n) .. Patch Set 2: Verified+1 Unrelated flake in AdminCliTest.TestSimultaneousLeaderTransferAndAbruptStepdown -- To view, visit http://gerrit.cloudera.org:8080/11743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Gerrit-Change-Number: 11743 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 23 Oct 2018 18:24:32 + Gerrit-HasComments: No
[kudu-CR] WIP KUDU-2542: add initial authorization token impl
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11750 ) Change subject: WIP KUDU-2542: add initial authorization token impl .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h File src/kudu/security/token_signer.h: http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@84 PS1, Line 84: tsk_propagation_period I don't quite follow the need to add this sentence. Is this a term mentioned/defined in other places? Do you mean 'tsk_propagation_interval'? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@124 PS1, Line 124: Note that the Activity Interval is identically the rotation : // interval I am not sure why is that necessary? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@211 PS1, Line 211: max(authn_token_validity, authz_token_validity) I know there is a lot of detailed description of why we end it up with this equation but maybe add a few sentences to concisely state the reason again, so that readers are not lost in the details? http://gerrit.cloudera.org:8080/#/c/11750/1/src/kudu/security/token_signer.h@289 PS1, Line 289: GenerateAuthzToken nit: add a comment? -- To view, visit http://gerrit.cloudera.org:8080/11750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id28747ec38675abdf50dce1e7c176d29213e370f Gerrit-Change-Number: 11750 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Oct 2018 18:10:12 + Gerrit-HasComments: Yes
[kudu-CR] WIP KUDU-2543: pass around default authz tokens
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11751 )
Change subject: WIP KUDU-2543: pass around default authz tokens
..
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc@483
PS1, Line 483: n
z ?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master_service.cc@422
PS1, Line 422: LOG(FATAL)
This behavior assumes other error cases (like empty remote_user().username())
are filtered out at an earlier phase. However, in the new code I don't see it
verified.
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/retriable_rpc.h@175
PS1, Line 175: template
: void RetriableRpc::GetNewAuthzTokenAndRetryCb(
: const Status& status) {
: if (status.ok()) {
: // Perform the RPC call with the newly fetched authz token.
: mutable_retrier()->mutable_controller()->Reset();
: SendRpc();
: } else {
: // Back to the retry sequence, hoping for better conditions
after some time.
: VLOG(1) << "Failed to get new authz token: " <<
status.ToString();
: mutable_retrier()->DelayedRetry(this, status);
: }
: }
This looks identical to GetNewAuthnTokenAndRetryCb() with exception of VLOG()
message. Does it make sense to unify those two methods?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h
File src/kudu/rpc/rpc_verification_util.h:
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h@37
PS1, Line 37: RPC
RPC error code?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc
File src/kudu/rpc/rpc_verification_util.cc:
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@45
PS1, Line 45: Status s
nit here and below: why this variable is necessary? Could it be just
*error = ...;
return Status::NotAuthorized(...);
?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@46
PS1, Line 46: *error = retry_error;
std::move ?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@388
PS1, Line 388: FATAL_INVALID_AUTHORIZATION_TOKEN
I would expect it to be FATAL_UNAUTHORIZED instead. Could you add the
explanation into a comment regarding this choice?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@402
PS1, Line 402: ErrorStatusPB::FATAL_UNAUTHORIZED
Why is FATAL_UNAUTHORIZED code chosen here instead of INVALID_AUTHZ_TOKEN? It
would be nice to explain that in a comment. Is that related to the authz token
re-acquisition logic?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1390
PS1, Line 1390: req->has_new_scan_request()
It seems already requests to existing scanners are not going to be examined
regarding the access permissions. Does it sound like a security issue (e.g.,
someone could grab already existing scan knowing its ID even if they don't have
proper scan privileges)?
If it does not sound like an issue, why not to move the authz_token into the
NewScanRequestPB instead of keeping it in ScanRequestPB?
http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1489
PS1, Line 1489: TabletServiceImpl::SplitKeyRange
Maybe, add a TODO into this method regarding verification of authz token in the
request?
--
To view, visit http://gerrit.cloudera.org:8080/11751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4
Gerrit-Change-Number: 11751
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Oct 2018 17:59:16 +
Gerrit-HasComments: Yes
