[kudu-CR] KUDU-2612: add PartitionLock and ScopedPartitionLock
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17097 ) Change subject: KUDU-2612: add PartitionLock and ScopedPartitionLock .. Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/17097/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17097/3//COMMIT_MSG@12 PS3, Line 12: sact > nit: acquire? Done http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager-test.cc File src/kudu/tablet/lock_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager-test.cc@195 PS3, Line 195: CountDownLatch latch(kNumStartAtOnce); > nit: since lock acquisition is quite fast operation, maybe it's worth synch Done http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager-test.cc@208 PS3, Line 208: ASSERT_EQ(0, lock_manager_.partition_lock_refs()); > nit: since lock acquisition is quite fast operation, maybe it's worth synch Done http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager.h File src/kudu/tablet/lock_manager.h: http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager.h@19 PS3, Line 19: #include > Nit: Can use the C++ compatible instead. Done http://gerrit.cloudera.org:8080/#/c/17097/3/src/kudu/tablet/lock_manager.h@157 PS3, Line 157: multi-rows t > I find it weird that a class named *Lock doesn't have any lock/unlock/relea I found it is more clear to have a specific entity to represent the lock. Even though it is just a wrapper around TxnId. But it can be further extended to contain more required field if needed in future. -- To view, visit http://gerrit.cloudera.org:8080/17097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I158115739ce3e7cfb77bbcb854e834336c1256b1 Gerrit-Change-Number: 17097 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 11 Mar 2021 07:21:28 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add PartitionLock and ScopedPartitionLock
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17097 to look at the new patch set (#5). Change subject: KUDU-2612: add PartitionLock and ScopedPartitionLock .. KUDU-2612: add PartitionLock and ScopedPartitionLock This patch introduces a coarse grained partition level lock, PartitionLock to prevent dirty writes for multi-rows transaction. A partition lock can only be held by a single transaction (or write op) at a time, the same transaction can acquire the lock for multiple times. To prevent deadlock, 'wait-die' scheme is used, which if the transaction requires a lock held by another transaction, 1) abort the transaction immediately if it has a higher txn ID than the other one. 2) Otherwise, retry the write op (or participant op) of the transaction. A ScopedPartitionLock is also introduced to automatically manage the lifecycle of a PartitionLock. Change-Id: I158115739ce3e7cfb77bbcb854e834336c1256b1 --- M src/kudu/tablet/lock_manager-test.cc M src/kudu/tablet/lock_manager.cc M src/kudu/tablet/lock_manager.h M src/kudu/tserver/tserver.proto 4 files changed, 396 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/17097/5 -- To view, visit http://gerrit.cloudera.org:8080/17097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I158115739ce3e7cfb77bbcb854e834336c1256b1 Gerrit-Change-Number: 17097 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: acquire and release partition lock
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17159 to look at the new patch set (#3). Change subject: KUDU-2612: acquire and release partition lock .. KUDU-2612: acquire and release partition lock This patch introduces the logic for actual acquiring and releasing the partition lock for transactions and non-transactional operations. For each transaction, we try to acquire the partition lock in the ParticipantOp prepare phase of BEGIN_TXN and release the lock when COMMIT_TXN or ABORT_TXN is applied. Moreover, we take the parition lock for non-transactional operations as well to ensure we don’t have duplicate keys. If the partition lock cannot be acquired, the transaction (or write op) will be aborted or retried. Change-Id: If26733cae16810f3b3afd1fd05dcb984e6366939 --- M src/kudu/client/batcher.cc M src/kudu/integration-tests/txn_participant-itest.cc M src/kudu/integration-tests/txn_write_ops-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/ops/participant_op.h M src/kudu/tablet/ops/write_op.cc M src/kudu/tablet/ops/write_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/txn_participant-test.cc M src/kudu/tablet/txn_participant.h M src/kudu/transactions/participant_rpc.cc 12 files changed, 376 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/17159/3 -- To view, visit http://gerrit.cloudera.org:8080/17159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If26733cae16810f3b3afd1fd05dcb984e6366939 Gerrit-Change-Number: 17159 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [test][master] KUDU-3249 Recover dead master at the same HostPort
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17126 to look at the new patch set (#2). Change subject: [test][master] KUDU-3249 Recover dead master at the same HostPort .. [test][master] KUDU-3249 Recover dead master at the same HostPort This change uses the Add/Remove master primitives added earlier to simulate recovery for a dead master at the same HostPort but different uuid. Test only code change but involves bunch of refactoring to re-use parts of existing test. Change-Id: Iac65201fd39ede5e918025ca7cf6a852a92d6eec --- M src/kudu/master/dynamic_multi_master-test.cc 1 file changed, 328 insertions(+), 163 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/17126/2 -- To view, visit http://gerrit.cloudera.org:8080/17126 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iac65201fd39ede5e918025ca7cf6a852a92d6eec Gerrit-Change-Number: 17126 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [test][master] KUDU-3249 Recover dead master at the same HostPort
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17126 ) Change subject: [test][master] KUDU-3249 Recover dead master at the same HostPort .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@643 PS1, Line 643: ASSERT_OK(CreateTable(cluster_.get(), kTableName)); > nit: this was surprising to see. Is it supposed to be here? It's not mentio Done http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@662 PS1, Line 662: Before removing the master set the output parameters as the master being removed : // will not be tracked by ExternalMiniCluster. > nit: these values don't change though, so what's the importance of setting Applies only to src_master_hp. Moving the comment there. http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@701 PS1, Line 701: consensus:: > nit: given how frequently this is used, add a "using" declaration Done http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@1079 PS1, Line 1079: Please follow the next steps which includes system catalog " : "tablet copy" > nit: missing a $0 somewhere? good catch! -- To view, visit http://gerrit.cloudera.org:8080/17126 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac65201fd39ede5e918025ca7cf6a852a92d6eec Gerrit-Change-Number: 17126 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 11 Mar 2021 02:55:21 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#6). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 43 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/6 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 6 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h@140 PS4, Line 140: 'client_col_id > nit: following suit with other comments, could you wrap variable/arg names Done -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 5 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 11 Mar 2021 01:14:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#5). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 43 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/5 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 5 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 ) Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/17161/4/src/kudu/common/row_operations.h@140 PS4, Line 140: client_col_idx nit: following suit with other comments, could you wrap variable/arg names with ''s? That'd make it a bit more obvious that you're referring them specifically. -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 4 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 11 Mar 2021 00:59:04 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/17141 )
Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster
ksck
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@657
PS5, Line 657: if (master.empty())
: continue;
Nit: I'm not sure whether it's part of style guide but for "if" if there is a
single stmt and without any braces then the stmt should be on the same single
line.
if (master.empty()) continue;
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@660
PS5, Line 660: er)+"
> That said, maybe instead keep an unordered_set of duplicated masters and
> output that via JoinStrings()? (see gutil/strings/join.h)
+1 to above.
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@665
PS5, Line 665: if (!duplicate_masters.empty())
: return Status::InvalidArgument("Duplicate master addresses
specified: " + duplicate_masters);
: *master_addresses = std::move(master_addresses_local);
Same as above about either using braces {} for "if" if the stmt is on next line.
Else it can cause confusion whether the L667 is part of the if stmt or no.
--
To view, visit http://gerrit.cloudera.org:8080/17141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f
Gerrit-Change-Number: 17141
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 10 Mar 2021 21:54:54 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671: No-op ClientServerMapping if only one schema exists.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17161 to look at the new patch set (#4). Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. .. KUDU-2671: No-op ClientServerMapping if only one schema exists. For the new field of PartitionSchema, a client schema is necessary for RowOperationsPBDecoder to populate the new field. Not all call sites for PartitionSchema::FromPB() have access to an explicit client schema. This patch allows the client schema to have column IDs if it's equal to the tablet schema and no-ops the ClientServerMapping in this case. If we don't have access to a client schema, we don't need a mapping since we only have one schema. Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h 3 files changed, 43 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/17161/4 -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 4 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] [KUDU-3255] fix failure on CentOS7 docker build
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/17169 ) Change subject: [KUDU-3255] fix failure on CentOS7 docker build .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I20048060e0ef68b8cb6de366b19aa97e7903f1a4 Gerrit-Change-Number: 17169 Gerrit-PatchSet: 1 Gerrit-Owner: Hongjiang Zhang Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 10 Mar 2021 15:25:11 + Gerrit-HasComments: No
[kudu-CR] [KUDU-3255] fix failure on CentOS7 docker build
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17169 ) Change subject: [KUDU-3255] fix failure on CentOS7 docker build .. [KUDU-3255] fix failure on CentOS7 docker build "https://bootstrap.pypa.io/2.7/get-pip.py; is replaced by "https://bootstrap.pypa.io/pip/2.7/get-pip.py; Change-Id: I20048060e0ef68b8cb6de366b19aa97e7903f1a4 Reviewed-on: http://gerrit.cloudera.org:8080/17169 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke --- M docker/bootstrap-python-env.sh 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Grant Henke: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I20048060e0ef68b8cb6de366b19aa97e7903f1a4 Gerrit-Change-Number: 17169 Gerrit-PatchSet: 2 Gerrit-Owner: Hongjiang Zhang Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
