[kudu-CR] KUDU-2612 p11: persist txn metadata in superblock
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16492 )
Change subject: KUDU-2612 p11: persist txn metadata in superblock
..
Patch Set 3:
(15 comments)
I did a first quick pass. So far looks reasonable, but I'm not sure I have a
clear big picture here, so my comments are just some nits.
http://gerrit.cloudera.org:8080/#/c/16492/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16492/3//COMMIT_MSG@17
PS3, Line 17: this can be used to store the owner of
: the transaction
Not related to this changelist directly, but I'm curios: what's the benefit of
storing the owner of a transaction in the superblock? As of now, we have that
information stored in the transaction status table and we use it to restrict
access to transaction management operations. Where we would use the
information on the owner stored in the superblock?
http://gerrit.cloudera.org:8080/#/c/16492/3//COMMIT_MSG@19
PS3, Line 19: commit timestamp
It would be nice if you could added more colors on the intended use of the txn
commit timestamp stored in the superblock.
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/metadata.proto@167
PS3, Line 167: Metadata associated with each transaction that this tablet is a
: // participant in
It would be nice to add more colors: what they key of the map is (transaction
id?) and how often this field is updated to reflect the changes in runtime?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.h
File src/kudu/tablet/ops/participant_op.h:
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.h@76
PS3, Line 76: tablet
Is it worth documenting the role of the 'tablet' parameter here?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.cc@232
PS3, Line 232: txn_participant->ClearIfInitFailed(txn_id);
nit: any reason not to move this after the 'if (PREDICT_FALSE(result ==
Op::ABORTED))' clause?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet.h@181
PS3, Line 181: Begins
nit here and below: it seems the majority of methods here are documented using
a different tense, e.g. here it would be 'Begin the transactions, ...'. Maybe,
it's worth keeping it consistent across this file?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@68
PS3, Line 68: RefCounted
nit: why not to use std::shared_ptr instead of scoped_refptr and use
make_shared to create related instances? Why scoped_refptr is better?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@70
PS3, Line 70: bool aborted,
: boost::optional commit_ts
Maybe, make these parameters to be 'false' and 'boost::none' by default?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@73
PS3, Line 73: commit_ts
nit: add std::move() ?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@95
PS3, Line 95: ~TxnMetadata() {}
nit: would the following be more idiomatic?
~TxnMetadata() = default;
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@446
PS3, Line 446: std::vector>
Is it possible to switch to std::vector and use move
semantics for passing parameters instead using std::unique_ptr?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@511
PS3, Line 511: txn_metadata_by_txn_id_.swap(txn_metas)
nit: why 'swap' is preferred instead of 'move' here?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@624
PS3, Line 624: anchors_needing_flush.clear();
Do you mind adding a comment to explain why it's necessary to explicitly empty
the container?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@839
PS3, Line 839: in_flight_txn_ids->swap(in_flights);
nit: IIRC, we tend to uniformly use std::move() in cases like this. Why 'swap'
is better for here?
http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@841
PS3, Line 841: swap
ditto
--
To view, visit http://gerrit.cloudera.org:8080/16492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f32808
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16494 ) Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_catalog.h@66 PS1, Line 66: // Creates a new table entry in the HMS. : // : // If 'owner' is omitted the table will be created without an owner. This is : // useful in circumstances where the owner is not known, for example when : // creating an HMS table entry for an existing Kudu table. : // : // Fails the HMS is unreachable, or a table with the same name is already present. nit: update the comment with the cluster_id? The same for other similar methods. http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/master/hms_notification_log_listener.cc File src/kudu/master/hms_notification_log_listener.cc: http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/master/hms_notification_log_listener.cc@356 PS1, Line 356: This is safe because we still validate the table ID : // which is universally unique Does it mean the table ID will never be the same even in across different clusters? http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/tools/tool_action_hms.cc@245 PS1, Line 245: row.resize nit: samer here. http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/tools/tool_action_hms.cc@256 PS1, Line 256: adjust row.resize below nit: adjust 'row.resize' below. -- To view, visit http://gerrit.cloudera.org:8080/16494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 24 Sep 2020 22:11:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster > I didn't test this explicitly, but because I am using the credentials from Good to know that HMS service takes care of that. Do we know that it works as expected for Kudu client? BTW, if it possible to add such a test as a part of MiniHms in ExternalMiniCluster? There it will be easy to play with Kerberos-related settings using MiniKdc properties. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 19:33:54 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p6 (c): add GetTransactionStatus() to TxnSystemClient
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16501 ) Change subject: KUDU-2612 p6 (c): add GetTransactionStatus() to TxnSystemClient .. Patch Set 1: Verified+1 -Code-Review -- To view, visit http://gerrit.cloudera.org:8080/16501 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7fac7158df307d03db6a48087e7c5a16269a3bc6 Gerrit-Change-Number: 16501 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 19:30:04 + Gerrit-HasComments: No
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@23 PS8, Line 23: Additonally > Additionally Done http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster > For the Kerberos-enabled case I'm curious how the expiration of authn crede I didn't test this explicitly, but because I am using the credentials from the HMS service itself, I would expect that the HMS would be renewing and updating the subject. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 19:09:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster For the Kerberos-enabled case I'm curious how the expiration of authn credentials is handled there. IIRC, Kudu client itself doesn't have provisions to renew Kerberos tickets nor it accepts a keytab. It's assumed that a Kudu client re-uses creds from the Kerberos cache (which is supposed to be initialized externally by other actors). So, the concern here is that when Kerberos creds in the cache expire, this unexpectedly stops working. And if so, would it create some problems in the consistency of catalog data on its own. For example, something that would require a manual intervention to restore the consistency. Did you try to run this if using very short-lived kerberos tickets? I guess that's something about setting 'ticket_lifetime' and 'renew_lifetime' to few minutes in krb5.conf -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 18:58:54 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#9). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additionally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f I also manually verified the following behavior on a real cluster: - `KUDU_HMS_SYNC_ENABLED` environment variable - CREATE/ALTER/DROP on non-sync cluster works - - With plugin jar used by the HMS - CREATE/ALTER/DROP on sync cluster fails with clear message - Authn via Kerberos Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 239 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/9 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@33 PS8, Line 33: - > nit: drop the extra dash ? I meant this as a sub-item. http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@34 PS8, Line 34: CREATE/ALTER/DROP on sync cluster > Does it work in case of using Ranger along for fine-grained authz? It does, though I didn't explicitly test because the superuser configs bypass ranger authz. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 18:23:43 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@23 PS8, Line 23: Additonally Additionally http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@33 PS8, Line 33: - nit: drop the extra dash ? http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@34 PS8, Line 34: CREATE/ALTER/DROP on sync cluster Does it work in case of using Ranger along for fine-grained authz? -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 18:18:18 + Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to add a master
Bankim Bhavsar has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16321 ) Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master .. [master] KUDU-2181 Raft ChangeConfig request to add a master This change: - Adds hidden feature flag "--master_support_change_config" off by default. - RPC changes to add a master that initiates Raft config change and responds asynchronously. - RPC changes to report back member type(VOTER/NON_VOTER) of masters - Removes the cached master_addresses in catalog manager as it's no longer static. - Updates and adds comments in MasterOptions such that it's used to fetch master addresses only during master init time as masters can be added/removed dynamically with this change. - Updates ListMasters() to look at local Raft config instead of MasterOptions as the masters can be added/removed dynamically. If the new master can be caught up from the WAL then the master gets promoted to VOTER else it remains as NON_VOTER without master tablet copying support. Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c Reviewed-on: http://gerrit.cloudera.org:8080/16321 Reviewed-by: Alexey Serbin Tested-by: Bankim Bhavsar --- M src/kudu/common/wire_protocol.proto M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_options.cc M src/kudu/master/master_options.h M src/kudu/master/master_path_handlers.cc M src/kudu/master/master_service.cc M src/kudu/master/master_service.h M src/kudu/master/mini_master.cc M src/kudu/master/sys_catalog.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/tools/tool_action_master.cc 20 files changed, 987 insertions(+), 56 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Bankim Bhavsar: Verified -- To view, visit http://gerrit.cloudera.org:8080/16321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c Gerrit-Change-Number: 16321 Gerrit-PatchSet: 9 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
