[kudu-CR] [gutil] fix assignment operator signature in DISALLOW COPY AND ASSIGN
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16158 Change subject: [gutil] fix assignment operator signature in DISALLOW_COPY_AND_ASSIGN .. [gutil] fix assignment operator signature in DISALLOW_COPY_AND_ASSIGN Per C++ reference [1], a copy assignment operator returns the reference to the object. At least, with the standard signature of the copy assignment operator in DISALLOW_COPY_AND_ASSIGN it's easier to read error messages, if any. [1] https://en.cppreference.com/w/cpp/language/copy_assignment Change-Id: Ic33276bde2e7e35abcdc0044a8f406ffa81382af --- M src/kudu/gutil/macros.h 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16158/1 -- To view, visit http://gerrit.cloudera.org:8080/16158 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic33276bde2e7e35abcdc0044a8f406ffa81382af Gerrit-Change-Number: 16158 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] Enable arenas for RPC request and response
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16136 ) Change subject: Enable arenas for RPC request and response .. Enable arenas for RPC request and response This changes the RPC server side to allocate a protobuf Arena for each request. The request RPC and response are allocated from the Arena, ensuring that any sub-messages, strings, repeated fields, etc, use that Arena for allocation as well. Everything is deleted en-masse when the InboundCall object (which owns the Arena) is destructed. This is mostly a straight-forward change except for the change in RaftConsensus. Specifically, we used to do a dirty const_cast to mutate the inbound request and release the ReplicateMsgs, and move them into the raft subsystem. When the request is allocated from an Arena, that 'release' is now actually making a copy, which broke the code path there. Given that there's now a copy happening nonetheless, I just made the code more explicitly construct a new ReplicateMsg copying out of the leader's request. There might be a slight performance degradation here but seemed worth it for code clarity. My assumption here is that anywhere that these copies are substantially expensive we'd probably be disk-bound anyway. Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Reviewed-on: http://gerrit.cloudera.org:8080/16136 Reviewed-by: Alexey Serbin Tested-by: Todd Lipcon --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/rpc/inbound_call.h M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/service_if.cc 6 files changed, 25 insertions(+), 32 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Todd Lipcon: Verified -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Enable arenas for RPC request and response
Todd Lipcon has removed a vote on this change. Change subject: Enable arenas for RPC request and response .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Enable arenas for RPC request and response
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16136 ) Change subject: Enable arenas for RPC request and response .. Patch Set 5: Verified+1 The failure here is a pre-existing LSAN issue with openssl, seems unrelated to this patch. -- To view, visit http://gerrit.cloudera.org:8080/16136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I810931900fc2b5f1dec1265abadfb33fb41d29bf Gerrit-Change-Number: 16136 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Jul 2020 04:33:06 + Gerrit-HasComments: No
[kudu-CR] KUDU-3090: Native owner metadata in Kudu
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15841 ) Change subject: KUDU-3090: Native owner metadata in Kudu .. Patch Set 13: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/15841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67f5bfdf56d409960365fd5803913a2d3800831d Gerrit-Change-Number: 15841 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 09 Jul 2020 01:54:28 + Gerrit-HasComments: No
[kudu-CR] [master] cache for table locations
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Greg Solovyev, Bankim Bhavsar, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15971 to look at the new patch set (#11). Change subject: [master] cache for table locations .. [master] cache for table locations This patch introduces a cache for table locations in catalog manager. When running with 48 concurrent client threads, the performance of CatalogManager::GetTableLocations() method improved about 100% when the cache is enabled. A smaller 14% improvement is observed for GetTableLocations RPC when running with the same number of concurrent client threads and cache enabled. The test results are below. I'm planning to add these test scenarios into kudu/scripts/benchmarks.sh in a follow up patch. After this patch with 128MByte cache enabled: table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \ --benchmark_num_threads=48 \ --table_locations_cache_capacity_mb=128 GetTableLocations function call: 504187.6 req/sec Before this patch: table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmarkFunctionCall \ --benchmark_num_threads=48 GetTableLocations function call: 252443.4 req/sec After this patch with 128MByte cache enabled: table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \ --rpc_num_service_threads=32 \ --benchmark_num_threads=48 \ --table_locations_cache_capacity_mb=128 GetTableLocations RPC: 40033.4 req/sec Stats on GetTableLocations RPC (times in microseconds): Count: 200167 Mean: 155.144 Percentiles: 0% (min) = 45 25%= 125 50% (med) = 147 75%= 177 95%= 223 99%= 314 99.9% = 652 99.99% = 1928 100% (max) = 5096 Before this patch: table_locations-itest \ --gtest_filter=TableLocationsTest.GetTableLocationsBenchmark \ --rpc_num_service_threads=32 \ --benchmark_num_threads=48 GetTableLocations RPC: 34981 req/sec Stats on GetTableLocations RPC (times in microseconds): Count: 174905 Mean: 249.308 Percentiles: 0% (min) = 64 25%= 197 50% (med) = 231 75%= 284 95%= 386 99%= 556 99.9% = 980 99.99% = 2400 100% (max) = 6113 Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23 --- M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master_service.cc A src/kudu/master/table_locations_cache.cc A src/kudu/master/table_locations_cache.h A src/kudu/master/table_locations_cache_metrics.cc A src/kudu/master/table_locations_cache_metrics.h 9 files changed, 1,098 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15971/11 -- To view, visit http://gerrit.cloudera.org:8080/15971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23 Gerrit-Change-Number: 15971 Gerrit-PatchSet: 11 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [master] cache for table locations
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
..
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1081
PS10, Line 1081: void TearDown() override {
: if (cluster_) {
: cluster_->Shutdown();
: cluster_.reset();
: }
: KuduTest::TearDown();
: }
> nit: doesn't this happen anyway via cluster_'s destructor?
Yes, I think it does. I think this piece is a copy-paste byproduct :)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1149
PS10, Line 1149: NO_FATALS(CheckCacheMetricsResetAllMasters());
> nit: Since this is only used once, perhaps inline it?
Done
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@54
PS10, Line 54: // Copying of entry handles is explicitly prohibited.
: EntryHandle(const EntryHandle&) = delete;
: EntryHandle& operator=(const EntryHandle&) = delete;
> Is this different than DISALLOW_COPY_AND_ASSIGN(EntryHandle)?
I guess it's supposed to be the same, but DISALLOW_COPY_AND_ASSIGN() for some
reason has operator=() as void.
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: // Invoked whenever a cached entry reaches zero reference
count, i.e. it was
: // removed from the cache and there aren't any other
references
: // floating around.
: std::unique_ptr eviction_cb_;
> Can this be stack-allocated and const?
I guess it might be such a thing, but there is an issue with the signature of
the EvictionCallback class: it requires a pointer to the outer cache class.
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@42
PS10, Line 42:
> nit: a couple too many spaces
Done
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@95
PS10, Line 95: auto h(cache_->Insert(std::move(pending), eviction_cb_.get()));
> If this isn't being done under keys_by_table_id_lock_, isn't it possible th
Good catch! Fixed.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 09 Jul 2020 01:21:28 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p4: mechanism to create a transaction status table
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16124 to look at the new patch set (#3). Change subject: KUDU-2612 p4: mechanism to create a transaction status table .. KUDU-2612 p4: mechanism to create a transaction status table This plumbs table type into the master, and exercises the code paths by adding a TxnSystemClient that can set the table type when creating tables (as opposed to KuduClient, which cannot send such requests). This patch adds the ability to create a transaction status table named "kudu_system.kudu_transactions", which foregoes authorization checks (though I left TODOs indicating that only the service user should be able to create this table) and HMS synchronization. This TxnSystemClient can be used by leader masters in the future to create the initial transaction status table, but for now is useful for testing the plumbing of table type. Change-Id: I29b9009fa228a7749295b50516991613a28d58fa --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_creator-internal.h M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/master_authz-itest.cc M src/kudu/integration-tests/master_hms-itest.cc A src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/tablet/tablet_metadata.cc M src/kudu/transactions/CMakeLists.txt M src/kudu/transactions/txn_status_tablet.cc M src/kudu/transactions/txn_status_tablet.h A src/kudu/transactions/txn_system_client.cc A src/kudu/transactions/txn_system_client.h M src/kudu/tserver/tablet_service.cc 18 files changed, 430 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/16124/3 -- To view, visit http://gerrit.cloudera.org:8080/16124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I29b9009fa228a7749295b50516991613a28d58fa Gerrit-Change-Number: 16124 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p3: tserver mechanism to create txn status tablets
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16116 to look at the new patch set (#3). Change subject: KUDU-2612 p3: tserver mechanism to create txn status tablets .. KUDU-2612 p3: tserver mechanism to create txn status tablets This introduces the concept of a table type in tablets. This is used in the context of creating transaction status table partitions; if a tablet replica is created as a partition of a transaction status table, the underlying replica will initialize some state to manage and coordinate transactions -- namely, a TxnStatusManager. For the sake of decoupling submodules, to get a TabletReplica to initialize a TxnStatusManager, this patch introduces the tablet::TxnCoordinator and tablet::TxnCoordinatorFactory interfaces that TxnStatusManager and the new TxnStatusManagerFactory implement respectively. The TxnStatusManagerFactory can be created by members of the tserver and passed to TabletReplicas upon initialization -- this layer of indirection will allow us to use tserver-wide state (e.g. in the future, a system client) without muddying the tablet subdirectory too much. This approach lies in contrast to the approach used for the CatalogManager, in which the master server owns a CatalogManager that owns the underlying SysCatalogTable and TabletReplica. I went down this route because unlike the CatalogManager replicas, I expect the replicas of TxnStatusTablets to be dynamically moved around, and so it behooves us to reuse as much of the existing tserver replica management code as possible. To that end, the ownership relationship between management state and underyling tablet replica is flipped, with the TabletReplica owning the TxnStatusManager. The plumbing only extends through the tablet servers -- the ability to create transaction status tables and define partitioning is not yet plumbed into the master. Additionally, there is still currently no means to restrict calls to the TxnStatusManagers whose underlying replicas are running leaders -- that will also come in later patches. Change-Id: Ib429f055e12944fa930f3e95ec4f2504466d3d02 --- M src/kudu/common/common.proto M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/tablet-harness.h M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/tablet_replica-test-base.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h A src/kudu/tablet/txn_coordinator.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/transactions/CMakeLists.txt M src/kudu/transactions/txn_status_manager-test.cc M src/kudu/transactions/txn_status_manager.cc M src/kudu/transactions/txn_status_manager.h M src/kudu/transactions/txn_status_tablet.cc M src/kudu/transactions/txn_status_tablet.h M src/kudu/tserver/CMakeLists.txt M src/kudu/tserver/mini_tablet_server.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/tserver/tserver_admin.proto 29 files changed, 443 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/16116/3 -- To view, visit http://gerrit.cloudera.org:8080/16116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib429f055e12944fa930f3e95ec4f2504466d3d02 Gerrit-Change-Number: 16116 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p3: tserver mechanism to create txn status tablets
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16116 to look at the new patch set (#2). Change subject: KUDU-2612 p3: tserver mechanism to create txn status tablets .. KUDU-2612 p3: tserver mechanism to create txn status tablets This introduces the concept of a table type in tablets. This is used in the context of creating transaction status table partitions; if a tablet replica is created as a partition of a transaction status table, the underlying replica will initialize some state to manage and coordinate transactions -- namely, a TxnStatusManager. For the sake of decoupling submodules, to get a TabletReplica to initialize a TxnStatusManager, this patch introduces the tablet::TxnCoordinator and tablet::TxnCoordinatorFactory interfaces that TxnStatusManager and the new TxnStatusManagerFactory implement respectively. The TxnStatusManagerFactory can be created by members of the tserver and passed to TabletReplicas upon initialization -- this layer of indirection will allow us to use tserver-wide state (e.g. in the future, a system client) without muddying the tablet subdirectory too much. The plumbing only extends through the tablet servers -- the ability to create transaction status tables and define partitioning is not yet plumbed into the master. Additionally, there is still currently no means to restrict calls to the TxnStatusManagers whose underlying replicas are running leaders -- that will also come in later patches. Change-Id: Ib429f055e12944fa930f3e95ec4f2504466d3d02 --- M src/kudu/common/common.proto M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/tablet-harness.h M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/tablet_replica-test-base.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h A src/kudu/tablet/txn_coordinator.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/transactions/CMakeLists.txt M src/kudu/transactions/txn_status_manager-test.cc M src/kudu/transactions/txn_status_manager.cc M src/kudu/transactions/txn_status_manager.h M src/kudu/transactions/txn_status_tablet.cc M src/kudu/transactions/txn_status_tablet.h M src/kudu/tserver/CMakeLists.txt M src/kudu/tserver/mini_tablet_server.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/tserver/tserver_admin.proto 29 files changed, 443 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/16116/2 -- To view, visit http://gerrit.cloudera.org:8080/16116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib429f055e12944fa930f3e95ec4f2504466d3d02 Gerrit-Change-Number: 16116 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 22:59:42 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16044 to look at the new patch set (#11). Change subject: KUDU-2612 p2: introduce transaction status management .. KUDU-2612 p2: introduce transaction status management This introduces the TxnStatusManager, which is backed by the TxnStatusTablet that exposes the following APIs that will be called via RPC, and will serve as many of the building blocks for orchestrating two-phase commit: - BeginTransaction: adds a new transaction under management of the TxnStatusManager - BeginCommitTransaction: transitions the state of a transaction from OPEN to COMMIT_IN_PROGRESS - AbortTransaction: transitions the state of a transaction from OPEN or COMMIT_IN_PROGRESS to ABORTED - RegisterParticipant: adds a participant to be associated with a specific transaction ID For completeness sake w.r.t defining the transaction state's enums, the following API is also added, which will be called by the TxnStatusManager itself upon determining a transaction has been completed. - FinalizeCommitTransaction: transitions the state of a transaction from COMMIT_IN_PROGRESS to COMMITTED This new abstraction mirrors that used by the CatalogManager, which uses copy-on-write locking to protect concurrent access to metadata while writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this case, TxnStatusTablet) are being replicated. This is at least enough of a jumping off point that we can begin plumbing this into the tablet servers and defining an RPC service around it -- there are still no facilities to create a TxnStatusManager. It should be noted that end-users will not call these methods directly, but rather through some layer of indirection (e.g. clients won't request a specific transaction ID, they'll just request to begin a transaction, and some intermediary layer will be in charge of getting an appropriate transaction ID). This should give us flexibility in changing the TxnStatusManager's interface moving forward. Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 --- M src/kudu/master/catalog_manager.h M src/kudu/transactions/CMakeLists.txt A src/kudu/transactions/txn_status_entry.cc A src/kudu/transactions/txn_status_entry.h A src/kudu/transactions/txn_status_manager-test.cc A src/kudu/transactions/txn_status_manager.cc A src/kudu/transactions/txn_status_manager.h M src/kudu/util/cow_object.h 8 files changed, 1,075 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/11 -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/16044/10/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16044/10/src/kudu/transactions/txn_status_manager-test.cc@326 PS10, Line 326: AIL() << "bad up Oops, seems I clobbered over this change. -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 22:44:41 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 22:24:35 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16044 to look at the new patch set (#10). Change subject: KUDU-2612 p2: introduce transaction status management .. KUDU-2612 p2: introduce transaction status management This introduces the TxnStatusManager, which is backed by the TxnStatusTablet that exposes the following APIs that will be called via RPC, and will serve as many of the building blocks for orchestrating two-phase commit: - BeginTransaction: adds a new transaction under management of the TxnStatusManager - BeginCommitTransaction: transitions the state of a transaction from OPEN to COMMIT_IN_PROGRESS - AbortTransaction: transitions the state of a transaction from OPEN or COMMIT_IN_PROGRESS to ABORTED - RegisterParticipant: adds a participant to be associated with a specific transaction ID For completeness sake w.r.t defining the transaction state's enums, the following API is also added, which will be called by the TxnStatusManager itself upon determining a transaction has been completed. - FinalizeCommitTransaction: transitions the state of a transaction from COMMIT_IN_PROGRESS to COMMITTED This new abstraction mirrors that used by the CatalogManager, which uses copy-on-write locking to protect concurrent access to metadata while writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this case, TxnStatusTablet) are being replicated. This is at least enough of a jumping off point that we can begin plumbing this into the tablet servers and defining an RPC service around it -- there are still no facilities to create a TxnStatusManager. It should be noted that end-users will not call these methods directly, but rather through some layer of indirection (e.g. clients won't request a specific transaction ID, they'll just request to begin a transaction, and some intermediary layer will be in charge of getting an appropriate transaction ID). This should give us flexibility in changing the TxnStatusManager's interface moving forward. Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 --- M src/kudu/master/catalog_manager.h M src/kudu/transactions/CMakeLists.txt A src/kudu/transactions/txn_status_entry.cc A src/kudu/transactions/txn_status_entry.h A src/kudu/transactions/txn_status_manager-test.cc A src/kudu/transactions/txn_status_manager.cc A src/kudu/transactions/txn_status_manager.h M src/kudu/util/cow_object.h 8 files changed, 1,075 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/10 -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc@356 PS9, Line 356: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); > Whoops, I missed this last review round: the result status is not checked. Oops. Done -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 22:09:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 9: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16044/9/src/kudu/transactions/txn_status_manager-test.cc@356 PS9, Line 356: s = txn_manager_->RegisterParticipant(1, ParticipantId(1), kWrongUser); Whoops, I missed this last review round: the result status is not checked. -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 21:44:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328 PS6, Line 328: > I still see LOG(DFATAL) instead of FAIL() here. It seems the usage of thes Done http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404 PS6, Line 404: _TRUE > IIRC, in writing it's more common to avoid the contraction and use "cannot" That's true, though I think we tend to have a somewhat conversational tone in comments, rather than formal. I typically reserve such formality for written essays, whitepapers, newspapers, etc. -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 21:22:48 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16044 to look at the new patch set (#9). Change subject: KUDU-2612 p2: introduce transaction status management .. KUDU-2612 p2: introduce transaction status management This introduces the TxnStatusManager, which is backed by the TxnStatusTablet that exposes the following APIs that will be called via RPC, and will serve as many of the building blocks for orchestrating two-phase commit: - BeginTransaction: adds a new transaction under management of the TxnStatusManager - BeginCommitTransaction: transitions the state of a transaction from OPEN to COMMIT_IN_PROGRESS - AbortTransaction: transitions the state of a transaction from OPEN or COMMIT_IN_PROGRESS to ABORTED - RegisterParticipant: adds a participant to be associated with a specific transaction ID For completeness sake w.r.t defining the transaction state's enums, the following API is also added, which will be called by the TxnStatusManager itself upon determining a transaction has been completed. - FinalizeCommitTransaction: transitions the state of a transaction from COMMIT_IN_PROGRESS to COMMITTED This new abstraction mirrors that used by the CatalogManager, which uses copy-on-write locking to protect concurrent access to metadata while writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this case, TxnStatusTablet) are being replicated. This is at least enough of a jumping off point that we can begin plumbing this into the tablet servers and defining an RPC service around it -- there are still no facilities to create a TxnStatusManager. It should be noted that end-users will not call these methods directly, but rather through some layer of indirection (e.g. clients won't request a specific transaction ID, they'll just request to begin a transaction, and some intermediary layer will be in charge of getting an appropriate transaction ID). This should give us flexibility in changing the TxnStatusManager's interface moving forward. Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 --- M src/kudu/master/catalog_manager.h M src/kudu/transactions/CMakeLists.txt A src/kudu/transactions/txn_status_entry.cc A src/kudu/transactions/txn_status_entry.h A src/kudu/transactions/txn_status_manager-test.cc A src/kudu/transactions/txn_status_manager.cc A src/kudu/transactions/txn_status_manager.h M src/kudu/util/cow_object.h 8 files changed, 1,075 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/9 -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@281 PS6, Line 281: ReservoirSample > all_updates.size = 3 * 10, and kNumUpdatesInParallel = 20, so we're selecti Ah, it seems that was just a misunderstanding from my side: there are three elements per transaction ID, so all is well. Sorry for the noise. http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328 PS6, Line 328: LOG(DFATAL) << "bad update"; > Done I still see LOG(DFATAL) instead of FAIL() here. It seems the usage of these macros should be swapped for lines 327 vs 299 ? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404 PS6, Line 404: can't > This is common all over our codebase and has virtually no grammatical diffe IIRC, in writing it's more common to avoid the contraction and use "cannot" vs "can't", while in non-formal speech "can't" is used. Not sure where to put these in-line comments in the code, but I thought it would be more in the "writing" domain. Feel free to ignore this, it's just a nit. http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77 PS5, Line 77: > Thanks for summarizing the discussion Alexey! I amended the TODO to be more Thank you for updates Andrew! -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 21:14:54 + Gerrit-HasComments: Yes
[kudu-CR] [master] set RPC queue length to 100 by default
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16154 Change subject: [master] set RPC queue length to 100 by default .. [master] set RPC queue length to 100 by default There is some evidence from the field that in even in case of small (20-30 nodes) and mid-size (50-80 nodes) Kudu clusters, master's RPC queue sometimes overflows due to spikes of incoming client and TSHeartbeat requests. Since those request floods are attributed to batches of lightweight requests such as GetTableSchema, TSHeartbeatRequestPB, etc., it makes sense to increase the default setting for the RPC queue size from 50 to 100 for Kudu masters. Additionally, I did a minor code clean-up in master_runner.cc and tablet_server_runner.cc files. Change-Id: Ia90bc157d8a0d52b6d1320cf67bc533a51faf101 --- M src/kudu/master/master_runner.cc M src/kudu/tserver/tablet_server.h M src/kudu/tserver/tablet_server_runner.cc 3 files changed, 46 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/16154/1 -- To view, visit http://gerrit.cloudera.org:8080/16154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia90bc157d8a0d52b6d1320cf67bc533a51faf101 Gerrit-Change-Number: 16154 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] [Web-UI] Upgrade JQuery to 3.5.1
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16153 ) Change subject: [Web-UI] Upgrade JQuery to 3.5.1 .. [Web-UI] Upgrade JQuery to 3.5.1 Security scans of Kudu can show CVE-2020-11023 as a possible vulnerability given Kudu is using JQuery 3.2.1 for the web UI. Though that vulnerability is not an actual issue and can not be exploited in Kudu, we should still upgrade to avoid false positives in future security scans. https://nvd.nist.gov/vuln/detail/CVE-2020-11023 Change-Id: I3e5210d4d23b9b995e2011d32f245ed996c11db3 Reviewed-on: http://gerrit.cloudera.org:8080/16153 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong Reviewed-by: Greg Solovyev --- M build-support/release/rat_exclude_files.txt M src/kudu/server/webserver.cc D www/jquery-3.2.1.min.js A www/jquery-3.5.1.min.js M www/metrics.html 5 files changed, 5 insertions(+), 7 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved Greg Solovyev: Looks good to me, but someone else must approve -- To view, visit http://gerrit.cloudera.org:8080/16153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3e5210d4d23b9b995e2011d32f245ed996c11db3 Gerrit-Change-Number: 16153 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [Web-UI] Upgrade JQuery to 3.5.1
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/16153 ) Change subject: [Web-UI] Upgrade JQuery to 3.5.1 .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3e5210d4d23b9b995e2011d32f245ed996c11db3 Gerrit-Change-Number: 16153 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jul 2020 19:23:55 + Gerrit-HasComments: No
[kudu-CR] [Web-UI] Upgrade JQuery to 3.5.1
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16153 ) Change subject: [Web-UI] Upgrade JQuery to 3.5.1 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3e5210d4d23b9b995e2011d32f245ed996c11db3 Gerrit-Change-Number: 16153 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jul 2020 19:23:15 + Gerrit-HasComments: No
[kudu-CR] [Web-UI] Upgrade JQuery to 3.5.1
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16153 ) Change subject: [Web-UI] Upgrade JQuery to 3.5.1 .. Patch Set 1: I manually tested the web-ui on a local deployment to verify things work as expected. -- To view, visit http://gerrit.cloudera.org:8080/16153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3e5210d4d23b9b995e2011d32f245ed996c11db3 Gerrit-Change-Number: 16153 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jul 2020 19:19:22 + Gerrit-HasComments: No
[kudu-CR] [Web-UI] Upgrade JQuery to 3.5.1
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16153 Change subject: [Web-UI] Upgrade JQuery to 3.5.1 .. [Web-UI] Upgrade JQuery to 3.5.1 Security scans of Kudu can show CVE-2020-11023 as a possible vulnerability given Kudu is using JQuery 3.2.1 for the web UI. Though that vulnerability is not an actual issue and can not be exploited in Kudu, we should still upgrade to avoid false positives in future security scans. https://nvd.nist.gov/vuln/detail/CVE-2020-11023 Change-Id: I3e5210d4d23b9b995e2011d32f245ed996c11db3 --- M build-support/release/rat_exclude_files.txt M src/kudu/server/webserver.cc D www/jquery-3.2.1.min.js A www/jquery-3.5.1.min.js M www/metrics.html 5 files changed, 5 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/16153/1 -- To view, visit http://gerrit.cloudera.org:8080/16153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3e5210d4d23b9b995e2011d32f245ed996c11db3 Gerrit-Change-Number: 16153 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6
Bankim Bhavsar has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16114 ) Change subject: [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 .. [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 devtoolset3 package is no longer available on software collections causing build failures when building kudu from source on RHEL/CentOS 6. Using one of the only available unofficial copr repositories to install devtoolset3 since devtoolset3 is EOL already and we plan to migrate to a newer compiler version and hence a newer devtoolset for RHEL/CentOS 6 soon. Separate JIRA KUDU-3160 tracks upgrading devtoolset version for el6. Tests: - Successfully ran kudu-unit tests using devtoolset3 downloaded from the repo on an internal cloudera test job. - Verified doc instructions using ascii doc plugin in CLion IDE - Partially verified build with docker BASES=centos:6 docker/docker-build.sh on Mac OS 10.15.4 Verified from console that devtoolset3 repo was installed. However ran into issue while building kudu-python from what looks like an unrelated issue due to failure to find kudu_client library. Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe Reviewed-on: http://gerrit.cloudera.org:8080/16114 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke --- M docker/bootstrap-dev-env.sh M docs/installation.adoc 2 files changed, 9 insertions(+), 13 deletions(-) Approvals: Kudu Jenkins: Verified Grant Henke: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe Gerrit-Change-Number: 16114 Gerrit-PatchSet: 4 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR](gh-pages) site: update the prior release notes page
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16151 ) Change subject: site: update the prior release notes page .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: Id09f438a12b1c38dd011aa6fb5f22300fbff7886 Gerrit-Change-Number: 16151 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Yingchun Lai <405403...@qq.com> Gerrit-Comment-Date: Wed, 08 Jul 2020 17:40:20 + Gerrit-HasComments: No
[kudu-CR](gh-pages) site: update the prior release notes page
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16151 ) Change subject: site: update the prior release notes page .. site: update the prior release notes page Change-Id: Id09f438a12b1c38dd011aa6fb5f22300fbff7886 Reviewed-on: http://gerrit.cloudera.org:8080/16151 Reviewed-by: Yingchun Lai <405403...@qq.com> Tested-by: Andrew Wong --- M releases/1.12.0/docs/prior_release_notes.html 1 file changed, 481 insertions(+), 1 deletion(-) Approvals: Yingchun Lai: Looks good to me, approved Andrew Wong: Verified -- To view, visit http://gerrit.cloudera.org:8080/16151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: merged Gerrit-Change-Id: Id09f438a12b1c38dd011aa6fb5f22300fbff7886 Gerrit-Change-Number: 16151 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16044 to look at the new patch set (#8). Change subject: KUDU-2612 p2: introduce transaction status management .. KUDU-2612 p2: introduce transaction status management This introduces the TxnStatusManager, which is backed by the TxnStatusTablet that exposes the following APIs that will be called via RPC, and will serve as many of the building blocks for orchestrating two-phase commit: - BeginTransaction: adds a new transaction under management of the TxnStatusManager - BeginCommitTransaction: transitions the state of a transaction from OPEN to COMMIT_IN_PROGRESS - AbortTransaction: transitions the state of a transaction from OPEN or COMMIT_IN_PROGRESS to ABORTED - RegisterParticipant: adds a participant to be associated with a specific transaction ID For completeness sake w.r.t defining the transaction state's enums, the following API is also added, which will be called by the TxnStatusManager itself upon determining a transaction has been completed. - FinalizeCommitTransaction: transitions the state of a transaction from COMMIT_IN_PROGRESS to COMMITTED This new abstraction mirrors that used by the CatalogManager, which uses copy-on-write locking to protect concurrent access to metadata while writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this case, TxnStatusTablet) are being replicated. This is at least enough of a jumping off point that we can begin plumbing this into the tablet servers and defining an RPC service around it -- there are still no facilities to create a TxnStatusManager. It should be noted that end-users will not call these methods directly, but rather through some layer of indirection (e.g. clients won't request a specific transaction ID, they'll just request to begin a transaction, and some intermediary layer will be in charge of getting an appropriate transaction ID). This should give us flexibility in changing the TxnStatusManager's interface moving forward. Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 --- M src/kudu/master/catalog_manager.h M src/kudu/transactions/CMakeLists.txt A src/kudu/transactions/txn_status_entry.cc A src/kudu/transactions/txn_status_entry.h A src/kudu/transactions/txn_status_manager-test.cc A src/kudu/transactions/txn_status_manager.cc A src/kudu/transactions/txn_status_manager.h M src/kudu/util/cow_object.h 8 files changed, 1,075 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/8 -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 ) Change subject: KUDU-2612 p2: introduce transaction status management .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77 PS5, Line 77: Status LoadFromTablet(); > We discussed this with Andrew a bit last week or so. As I understand, it's Thanks for summarizing the discussion Alexey! I amended the TODO to be more inclusive of other paths forward, and I updated the design doc to mention them a bit as well. -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 17:39:59 + Gerrit-HasComments: Yes
[kudu-CR] [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16114 ) Change subject: [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe Gerrit-Change-Number: 16114 Gerrit-PatchSet: 3 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Jul 2020 17:25:14 + Gerrit-HasComments: No
[kudu-CR] [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16114 ) Change subject: [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/16114/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16114/2//COMMIT_MSG@12 PS2, Line 12: copr repository > nit: repositories Done http://gerrit.cloudera.org:8080/#/c/16114/2//COMMIT_MSG@16 PS2, Line 16: Seperate > nit: Separate Done http://gerrit.cloudera.org:8080/#/c/16114/2//COMMIT_MSG@25 PS2, Line 25: Unfortunately while building thirdparty docker engine stop/gives error : after some time. : "failed to dial gRPC: unable to upgrade to h2c, received 502" > I think we can look into this issue separately. It's definitely not related Yeah this looks like an unrelated issue. http://gerrit.cloudera.org:8080/#/c/16114/2/docker/bootstrap-dev-env.sh File docker/bootstrap-dev-env.sh: http://gerrit.cloudera.org:8080/#/c/16114/2/docker/bootstrap-dev-env.sh@103 PS2, Line 103: DTLS_REPO_URL=https://copr.fedorainfracloud.org/coprs/rhscl/devtoolset-3/repo/epel-6/rhscl-devtoolset-3-epel-6.repo > Feel free to ignore this: it's just a nit. It helps keep the yum-config-manager shorter and in case this URL needs to be updated, it's easier. -- To view, visit http://gerrit.cloudera.org:8080/16114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe Gerrit-Change-Number: 16114 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Jul 2020 17:13:45 + Gerrit-HasComments: Yes
[kudu-CR] [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16114 to look at the new patch set (#3). Change subject: [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 .. [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 devtoolset3 package is no longer available on software collections causing build failures when building kudu from source on RHEL/CentOS 6. Using one of the only available unofficial copr repositories to install devtoolset3 since devtoolset3 is EOL already and we plan to migrate to a newer compiler version and hence a newer devtoolset for RHEL/CentOS 6 soon. Separate JIRA KUDU-3160 tracks upgrading devtoolset version for el6. Tests: - Successfully ran kudu-unit tests using devtoolset3 downloaded from the repo on an internal cloudera test job. - Verified doc instructions using ascii doc plugin in CLion IDE - Partially verified build with docker BASES=centos:6 docker/docker-build.sh on Mac OS 10.15.4 Verified from console that devtoolset3 repo was installed. However ran into issue while building kudu-python from what looks like an unrelated issue due to failure to find kudu_client library. Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe --- M docker/bootstrap-dev-env.sh M docs/installation.adoc 2 files changed, 9 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/16114/3 -- To view, visit http://gerrit.cloudera.org:8080/16114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe Gerrit-Change-Number: 16114 Gerrit-PatchSet: 3 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16114 ) Change subject: [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 .. Patch Set 2: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/16114/2/docker/bootstrap-dev-env.sh File docker/bootstrap-dev-env.sh: http://gerrit.cloudera.org:8080/#/c/16114/2/docker/bootstrap-dev-env.sh@103 PS2, Line 103: DTLS_REPO_URL=https://copr.fedorainfracloud.org/coprs/rhscl/devtoolset-3/repo/epel-6/rhscl-devtoolset-3-epel-6.repo > nit: if this variable isn't using elsewhere but only at line 105, why not t Feel free to ignore this: it's just a nit. -- To view, visit http://gerrit.cloudera.org:8080/16114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe Gerrit-Change-Number: 16114 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Jul 2020 17:00:57 + Gerrit-HasComments: Yes
[kudu-CR] [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16114 ) Change subject: [docs][docker] KUDU-3159 Fix missing devtoolset3 package on el6 .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/16114/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16114/2//COMMIT_MSG@25 PS2, Line 25: Unfortunately while building thirdparty docker engine stop/gives error : after some time. : "failed to dial gRPC: unable to upgrade to h2c, received 502" > Increasing memory definitely got rid of the above error. I think we can look into this issue separately. It's definitely not related to this change. -- To view, visit http://gerrit.cloudera.org:8080/16114 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c4b9546c835df26f993ed6164e92f2d4c55c3fe Gerrit-Change-Number: 16114 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Jul 2020 14:11:01 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3090 Add delegate admin privilege
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16071 ) Change subject: KUDU-3090 Add delegate admin privilege .. Patch Set 14: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16071 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If8ba018dac568a1ab74cf2d5657221579636ac1c Gerrit-Change-Number: 16071 Gerrit-PatchSet: 14 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 14:07:19 + Gerrit-HasComments: No
[kudu-CR] KUDU-3090 Support backing up ownership info
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/16126 )
Change subject: KUDU-3090 Support backing up ownership info
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16126/7/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File
java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:
http://gerrit.cloudera.org:8080/#/c/16126/7/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@747
PS7, Line 747: def validateTablesMatch(tableA: String, tableB: String): Unit
= {
> I'm not sure what you mean
Do you mean it was changed between a full backup and a subsequent incremental
backup?
Metadata changes like that aren't super relevant to test because we always use
the metadata from the last incremental backup when restoring.
--
To view, visit http://gerrit.cloudera.org:8080/16126
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I963db0a36cd4b7f080944ed46fc4119b1e055143
Gerrit-Change-Number: 16126
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 08 Jul 2020 14:06:50 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3090 Support backing up ownership info
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16126 ) Change subject: KUDU-3090 Support backing up ownership info .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16126 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I963db0a36cd4b7f080944ed46fc4119b1e055143 Gerrit-Change-Number: 16126 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jul 2020 14:07:08 + Gerrit-HasComments: No
[kudu-CR] KUDU-3090 Ownership support in Java client
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16125 ) Change subject: KUDU-3090 Ownership support in Java client .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16125 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083ad9750ce1b3ae31bb510b700d1204fcdf291d Gerrit-Change-Number: 16125 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jul 2020 14:04:06 + Gerrit-HasComments: No
[kudu-CR] KUDU-3090: Native owner metadata in Kudu
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15841 ) Change subject: KUDU-3090: Native owner metadata in Kudu .. Patch Set 13: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/15841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67f5bfdf56d409960365fd5803913a2d3800831d Gerrit-Change-Number: 15841 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 14:02:43 + Gerrit-HasComments: No
[kudu-CR] KUDU-3090 Add ownership privileges
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16072
to look at the new patch set (#17).
Change subject: KUDU-3090 Add ownership privileges
..
KUDU-3090 Add ownership privileges
Apache Ranger supports granting privileges to resource owners by using a
special "{OWNER}" username. This patch integrates Kudu's ownership model
with Ranger.
Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109
---
M
java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/default_authz_provider.h
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/master/ranger_authz_provider.h
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
11 files changed, 304 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/16072/17
--
To view, visit http://gerrit.cloudera.org:8080/16072
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109
Gerrit-Change-Number: 16072
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [maintenance] use workload statistics to scale perf improvement
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15995 to look at the new patch set (#6). Change subject: [maintenance] use workload statistics to scale perf_improvement .. [maintenance] use workload statistics to scale perf_improvement When we consider the performance improvement brought by maintenance operations, we could use workload statistics to find how 'hot' the tablet has been in the last few minutes and perform maintenance ops for 'hot' tablets in priority. This patch use recent read/write rate of a tablet as a workload score, calculate a final perf score based on a op's raw perf_improvement, the tablet's workload score and the table's priority, so maintenance ops for a 'hotter' tablet are more likely to lauch. In our usercases, there is insert/update/delete traffic all the time, but some tables may have more read traffic at some time, so we want to dynamically adjust priorities of compaction/flush ops for different tables. We tested this on a 6-node cluster and set maintenance_manager_num_threads=1 for tservers, run three YCSB workloads in order on two tables with 64 tablets. We run almost the same workload on two tables at the same time, except for different threadcount/recordcount/operationcount settings, in order to implement different read/write rates and runtime for these two tables. worklaod_a: insert only workload. threadcount=1(table-A)/16(table-B) recordcount=50,000,000(table-A)/1,000,000,000(table-B) result: measurement Before change After change [table-A:INSERT]AverageLatency(us) 62.4835261.45316 [table-A:INSERT]95thPercentileLatency(us) 4 4 [table-B:INSERT]AverageLatency(us) 58.0531456.55963 [table-B:INSERT]95thPercentileLatency(us) 6 6 workload_b: scan mostly worklaod, scan/update ratio is 80/20. threadcount=1(table-A)/16(table-B) operationcount=500,000(table-A)/10,000,000(table-B) requestdistribution=zipfian scanlengthdistribution=zipfian maxscanlength=100 readallfields=false result: measurement Before change After change [table-A:UPDATE]AverageLatency(us) 6.73773 5.58511 [table-A:UPDATE]95thPercentileLatency(us) 15 11 [table-A:SCAN]AverageLatency(us)834.47093 481.40569(-42%) [table-A:SCAN]95thPercentileLatency(us) 639 479(-25%) [table-B:UPDATE]AverageLatency(us) 4.61783 4.58399 [table-B:UPDATE]95thPercentileLatency(us) 7 7 [table-B:SCAN]AverageLatency(us)2168.55291 1979.14102(-8%) [table-B:SCAN]95thPercentileLatency(us) 77274671(-39%) workload_c: insert heavy workload, scan/insert ratio is 20/80. threadcount=1(table-A)/16(table-B) operationcount=5,000,000(table-A)/100,000,000(table-B) insertorder=hashed requestdistribution=zipfian scanlengthdistribution=zipfian maxscanlength=100 readallfields=false result: measurementsBefore change After change [table-A:INSERT]AverageLatency(us) 7.89913 7.90852 [table-A:INSERT]95thPercentileLatency(us) 9 7 [table-A:SCAN]AverageLatency(us)1617.92456 960.75466(-40%) [table-A:SCAN]95thPercentileLatency(us) 98712073(-78%) [table-B:INSERT]AverageLatency(us) 9.11269 9.56165 [table-B:INSERT]95thPercentileLatency(us) 8 9 [table-B:SCAN]AverageLatency(us)1392.94679 1316.02913(-5.4%) [table-B:SCAN]95thPercentileLatency(us) 36653059-(16.5%) We can see that with this change we can improve scan performance in above test cases. And this path will not lead to a compaction/flush starvation of low read/write tablets because workload_score is at most 1, we can check this on tablet servers' dashboard/ maintenance-manager page. Change-Id: Ie3afcc359002d1392164ba2fda885f8930ef8696 --- M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_mm_ops.cc M src/kudu/tablet/tablet_mm_ops.h M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.h M src/kudu/util/maintenance_manager-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/maintenance_manager.h 9 files changed, 134 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/15995/6 -- To view, visit http://gerrit.cloudera.org:8080/15995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset
[kudu-CR] KUDU-3090 Add ownership privileges
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16072
to look at the new patch set (#16).
Change subject: KUDU-3090 Add ownership privileges
..
KUDU-3090 Add ownership privileges
Apache Ranger supports granting privileges to resource owners by using a
special "{OWNER}" username. This patch integrates Kudu's ownership model
with Ranger.
Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109
---
M
java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java
M src/kudu/integration-tests/master_authz-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/default_authz_provider.h
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/master/ranger_authz_provider.h
M src/kudu/ranger/ranger.proto
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
11 files changed, 291 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/16072/16
--
To view, visit http://gerrit.cloudera.org:8080/16072
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109
Gerrit-Change-Number: 16072
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3090 Ownership support in Java client
Hello Kudu Jenkins, Andrew Wong, Grant Henke, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16125 to look at the new patch set (#8). Change subject: KUDU-3090 Ownership support in Java client .. KUDU-3090 Ownership support in Java client Change-Id: I083ad9750ce1b3ae31bb510b700d1204fcdf291d --- M java/kudu-client/build.gradle M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java D java/kudu-client/src/test/java/org/apache/kudu/client/TestHiveMetastoreIntegration.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKeyEncoding.java 12 files changed, 77 insertions(+), 95 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/16125/8 -- To view, visit http://gerrit.cloudera.org:8080/16125 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I083ad9750ce1b3ae31bb510b700d1204fcdf291d Gerrit-Change-Number: 16125 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3090: Native owner metadata in Kudu
Attila Bukor has posted comments on this change. (
http://gerrit.cloudera.org:8080/15841 )
Change subject: KUDU-3090: Native owner metadata in Kudu
..
Patch Set 13:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/integration-tests/master_hms-itest.cc@303
PS12, Line 303: TEST_F(MasterHmsTest, TestAlterTableOwner) {
> Would it also be possible to test the case where we're adding an owner to a
Do you mean the empty string case in kudu-tool-test? That one was meant to test
this upgrade scenario, why do you think it's different?
http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc@2522
PS10, Line 2522: }
:
> Whoops, indeed :)
Sure, it's valid, you can modify both in a single alter.
http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/master/catalog_manager.cc@187
PS12, Line 187: n."
> nit: unrelated, but could you add a period here?
Done
http://gerrit.cloudera.org:8080/#/c/15841/12/src/kudu/master/catalog_manager.cc@2684
PS12, Line 2684:
> nit: could you move this closer to where it's used so readers don't need lo
actually it's used only once so I removed it.
http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:
http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc@385
PS10, Line 385: before_table.owner != after_table.owner
> Great, thanks for the clarification.
It should in the client side, but the server side would return an error as we
have an emptyness check (which can be turned off for tests with a hidden flag).
--
To view, visit http://gerrit.cloudera.org:8080/15841
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67f5bfdf56d409960365fd5803913a2d3800831d
Gerrit-Change-Number: 15841
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 12:16:50 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3090 Ownership support in Java client
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16125 ) Change subject: KUDU-3090 Ownership support in Java client .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16125/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/16125/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@134 PS7, Line 134:* @return this table's owner > For that matter, it might be worth adding a test for what happens if this is > called on such a table. Would it make sense to add a Master flag for that, > like --set_owner_to_user_if_none_specified or something? Do you mean the master would return a specified user for unowned tables? I'm not sure that's a good idea as users wouldn't be able to tell apart unowned tables and tables owned by that user. > Would also be useful in testing the C++ client. Which tests would it be helpful for? -- To view, visit http://gerrit.cloudera.org:8080/16125 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I083ad9750ce1b3ae31bb510b700d1204fcdf291d Gerrit-Change-Number: 16125 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jul 2020 12:16:56 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3090: Native owner metadata in Kudu
Attila Bukor has uploaded a new patch set (#13) to the change originally created by Grant Henke. ( http://gerrit.cloudera.org:8080/15841 ) Change subject: KUDU-3090: Native owner metadata in Kudu .. KUDU-3090: Native owner metadata in Kudu Apache Sentry and Apache Ranger both support permissions granted to table owners, but as Sentry integrates with Apache Hive Metastore (HMS) and stores its metadata in it, Kudu didn't need to store table ownership to support granting permissions to owners. Apache Ranger on the other hand doesn't depend on HMS and needs Kudu to tell it if the owner is attempting to authorize an action, so to enable users to grant privileges to owners we need to support ownership natively. This patch adds the basic plumbing for table ownership, synchronizing ownership metadata with HMS both using the notification log listener and via tooling, and setting the owner on CREATE TABLE and ALTER TABLE requests in the C++ client. The maximum owner length is 128 characters by default which aligns with HMS/Apache Impala maximum owner lengths, but it's configurable with the max_owner_length flag. Supporting this in the Java and Python clients, authorizing these requests, and support for ownership in authorization will come in follow-up patches. Credit goes to Grant Henke for the initial version of this patch. Design doc: https://s.apache.org/kudu-ownership-design Change-Id: I67f5bfdf56d409960365fd5803913a2d3800831d --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/client.proto M src/kudu/client/scan_token-internal.cc M src/kudu/client/table-internal.cc M src/kudu/client/table-internal.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/client/table_creator-internal.h M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/hms_itest-base.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 26 files changed, 410 insertions(+), 91 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/15841/13 -- To view, visit http://gerrit.cloudera.org:8080/15841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I67f5bfdf56d409960365fd5803913a2d3800831d Gerrit-Change-Number: 15841 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3090 Support backing up ownership info
Attila Bukor has posted comments on this change. (
http://gerrit.cloudera.org:8080/16126 )
Change subject: KUDU-3090 Support backing up ownership info
..
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16126/7/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File
java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:
http://gerrit.cloudera.org:8080/#/c/16126/7/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@747
PS7, Line 747: def validateTablesMatch(tableA: String, tableB: String): Unit
= {
> Does it also make sense to test restoring a table that has had its owner ch
I'm not sure what you mean
http://gerrit.cloudera.org:8080/#/c/16126/7/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@750
PS7, Line 750: assertEquals(tA.getOwner, tB.getOwner)
> nit: maybe also check that the owner isn't empty?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16126
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I963db0a36cd4b7f080944ed46fc4119b1e055143
Gerrit-Change-Number: 16126
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 08 Jul 2020 12:17:00 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3090 Restrict changing ownership of a table
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16113 to look at the new patch set (#11). Change subject: KUDU-3090 Restrict changing ownership of a table .. KUDU-3090 Restrict changing ownership of a table Before this patch changing the ownership required ALTER permissions on a table. This patch adds a new method to authorization provider to authorize ownership changes and makes catalog manager call it if the owner is changed on an alter. Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1 --- M src/kudu/integration-tests/master_authz-itest.cc M src/kudu/master/authz_provider.h M src/kudu/master/catalog_manager.cc M src/kudu/master/default_authz_provider.h M src/kudu/master/ranger_authz_provider.cc M src/kudu/master/ranger_authz_provider.h 6 files changed, 74 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/16113/11 -- To view, visit http://gerrit.cloudera.org:8080/16113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1 Gerrit-Change-Number: 16113 Gerrit-PatchSet: 11 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3090 Restrict changing ownership of a table
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16113 ) Change subject: KUDU-3090 Restrict changing ownership of a table .. Patch Set 11: (2 comments) http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/authz_provider.h@32 PS10, Line 32: lass TablePrivilegePB; : > nit: drop the extra line? Done http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16113/10/src/kudu/master/catalog_manager.cc@2598 PS10, Line 2598: // Change owner requires higher level of privilege (ALL WITH GRANT OPTION, : // or ALL + delegate admin) than other types of alter operations, so if a : // single alter contains an owner change as well as other changes, it's : // sufficient to authorize only the owner change. : i > It seems easy to misconstrue this with a privilege escalation, since we're Done -- To view, visit http://gerrit.cloudera.org:8080/16113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1 Gerrit-Change-Number: 16113 Gerrit-PatchSet: 11 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 08 Jul 2020 12:17:11 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3090 Add ownership privileges
Attila Bukor has posted comments on this change. (
http://gerrit.cloudera.org:8080/16072 )
Change subject: KUDU-3090 Add ownership privileges
..
Patch Set 16:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16072/15/src/kudu/integration-tests/master_authz-itest.cc
File src/kudu/integration-tests/master_authz-itest.cc:
PS15:
> Can you also add a test for the case where a user gives its ownership away
Done
http://gerrit.cloudera.org:8080/#/c/16072/15/src/kudu/integration-tests/master_authz-itest.cc@935
PS15, Line 935: &
> nit: capitalize and add a period
Done
http://gerrit.cloudera.org:8080/#/c/16072/15/src/kudu/integration-tests/master_authz-itest.cc@1063
PS15, Line 1063:
: {
: {
: ::IsAlterTableDone,
: ::GrantGetMetadataTablePrivilege,
: "IsAlterTableDone",
> nit: maybe be simpler as
Unfortunately that doesn't work, kTestUser contains a hyphen which is an
invalid character. Another option would be to change the value of kTestUser but
I wasn't sure if I should touch it.
--
To view, visit http://gerrit.cloudera.org:8080/16072
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109
Gerrit-Change-Number: 16072
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 12:17:08 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3090 Support backing up ownership info
Hello Kudu Jenkins, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16126 to look at the new patch set (#8). Change subject: KUDU-3090 Support backing up ownership info .. KUDU-3090 Support backing up ownership info Change-Id: I963db0a36cd4b7f080944ed46fc4119b1e055143 --- M java/kudu-backup-common/src/main/protobuf/backup.proto M java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/TableMetadata.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java 4 files changed, 10 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/16126/8 -- To view, visit http://gerrit.cloudera.org:8080/16126 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I963db0a36cd4b7f080944ed46fc4119b1e055143 Gerrit-Change-Number: 16126 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management
..
Patch Set 7:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc
File src/kudu/transactions/txn_status_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@57
PS6, Line 57: using std::string;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@281
PS6, Line 281: vector
all_updates.size = 3 * 10, and kNumUpdatesInParallel = 20, so we're selecting a
random subset to do in parallel.
> in this case not a single transaction will change its state in a concurrent
> manner, no?
I'm not sure I understand why this wouldn't be considered concurrent,
regardless of count.
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328
PS6, Line 328:
> Why LOG(DFATAL) is better than simply failing the test with FAIL() ?
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@348
PS6, Line 348: ASSERT_OK(txn_manager_->RegisterParticipant(1, Partic
> Do you mind adding a scenario to call BeginTransaction() after this, but ca
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@357
PS6, Line 357: s = txn_manager_->RegisterParticipant(1, ParticipantId(2),
kWrongUser);
> What is the significance of the result? In other words, what is expected t
Whoops, done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: s.IsIlle
> register
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404
PS6, Line 404: _TRUE
> nit: cannot
This is common all over our codebase and has virtually no grammatical
difference. Any particular reason?
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@409
PS6, Line 409: ST_F(TxnStatusManagerTest, TestRegisterParticipantsWithStates) {
> Do you mind adding a scenario to show that register the same participant wi
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h
File src/kudu/transactions/txn_status_manager.h:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h@114
PS6, Line 114:
> What's the expectation on a per-transaction set of IDs? I see the implemen
Done
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc
File src/kudu/transactions/txn_status_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@67
PS6, Line 67: // locking.
> Is it guaranteed we don't have a race here while setting the highest_txn_id
Done
This isn't a thread-safe method. Updated the method comments and added a note
here.
http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@94
PS6, Line 94: :loc
> nit for here and elsewhere: wrap these unlikely error conditions into PREDI
Done
--
To view, visit http://gerrit.cloudera.org:8080/16044
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636
Gerrit-Change-Number: 16044
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 08 Jul 2020 07:19:39 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2: introduce transaction status management
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16044 to look at the new patch set (#7). Change subject: KUDU-2612 p2: introduce transaction status management .. KUDU-2612 p2: introduce transaction status management This introduces the TxnStatusManager, which is backed by the TxnStatusTablet that exposes the following APIs that will be called via RPC, and will serve as many of the building blocks for orchestrating two-phase commit: - BeginTransaction: adds a new transaction under management of the TxnStatusManager - BeginCommitTransaction: transitions the state of a transaction from OPEN to COMMIT_IN_PROGRESS - AbortTransaction: transitions the state of a transaction from OPEN or COMMIT_IN_PROGRESS to ABORTED - RegisterParticipant: adds a participant to be associated with a specific transaction ID For completeness sake w.r.t defining the transaction state's enums, the following API is also added, which will be called by the TxnStatusManager itself upon determining a transaction has been completed. - FinalizeCommitTransaction: transitions the state of a transaction from COMMIT_IN_PROGRESS to COMMITTED This new abstraction mirrors that used by the CatalogManager, which uses copy-on-write locking to protect concurrent access to metadata while writes to the underlying TabletReplica (i.e. SysCatalogTable, or in this case, TxnStatusTablet) are being replicated. This is at least enough of a jumping off point that we can begin plumbing this into the tablet servers and defining an RPC service around it -- there are still no facilities to create a TxnStatusManager. It should be noted that end-users will not call these methods directly, but rather through some layer of indirection (e.g. clients won't request a specific transaction ID, they'll just request to begin a transaction, and some intermediary layer will be in charge of getting an appropriate transaction ID). This should give us flexibility in changing the TxnStatusManager's interface moving forward. Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 --- M src/kudu/master/catalog_manager.h M src/kudu/transactions/CMakeLists.txt A src/kudu/transactions/txn_status_entry.cc A src/kudu/transactions/txn_status_entry.h A src/kudu/transactions/txn_status_manager-test.cc A src/kudu/transactions/txn_status_manager.cc A src/kudu/transactions/txn_status_manager.h M src/kudu/util/cow_object.h 8 files changed, 1,074 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16044/7 -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [master] cache for table locations
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations
..
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1081
PS10, Line 1081: void TearDown() override {
: if (cluster_) {
: cluster_->Shutdown();
: cluster_.reset();
: }
: KuduTest::TearDown();
: }
nit: doesn't this happen anyway via cluster_'s destructor?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1149
PS10, Line 1149: NO_FATALS(CheckCacheMetricsResetAllMasters());
nit: Since this is only used once, perhaps inline it?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h
File src/kudu/master/table_locations_cache.h:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@54
PS10, Line 54: // Copying of entry handles is explicitly prohibited.
: EntryHandle(const EntryHandle&) = delete;
: EntryHandle& operator=(const EntryHandle&) = delete;
Is this different than DISALLOW_COPY_AND_ASSIGN(EntryHandle)?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145
PS10, Line 145: // Invoked whenever a cached entry reaches zero reference
count, i.e. it was
: // removed from the cache and there aren't any other
references
: // floating around.
: std::unique_ptr eviction_cb_;
Can this be stack-allocated and const?
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc
File src/kudu/master/table_locations_cache.cc:
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@42
PS10, Line 42:
nit: a couple too many spaces
http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@95
PS10, Line 95: auto h(cache_->Insert(std::move(pending), eviction_cb_.get()));
If this isn't being done under keys_by_table_id_lock_, isn't it possible that
we Put() and immediately Remove(), but end up with the entry added into the
cache, and nothing in keys_by_table_id_? I think that'd mean we're leaving
memory on the table in the cache since we may never evict those handles.
--
To view, visit http://gerrit.cloudera.org:8080/15971
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23
Gerrit-Change-Number: 15971
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 08 Jul 2020 06:31:57 +
Gerrit-HasComments: Yes
[kudu-CR] Use protobuf arenas for CommitMsgs
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16147 ) Change subject: Use protobuf arenas for CommitMsgs .. Patch Set 2: > (3 comments) > > Overall looks good to me, but it seems many tests need to be > updated: t hey fail with errors like > > F0707 23:05:28.555228 1482 write_op.cc:410] Check failed: > op->result->GetArena() == result->GetArena() (0x7f705ca0acf0 vs. 0) Ah, it seems that's not about updating individual tests, but rather allocating LocalTabletWriter::result_ with state_->pb_arena() or something. The stacks look like the following: F0707 23:09:05.798643 4615 write_op.cc:410] Check failed: op->result->GetArena() == result->GetArena() (0x2a10e30 vs. 0) *** Check failure stack trace: *** *** Aborted at 1594163345 (unix time) try "date -d @1594163345" if you are using GNU date *** PC: @ 0x7fea1c3f7c37 gsignal *** SIGABRT (@0x3e81207) received by PID 4615 (TID 0x7fea214e48c0) from PID 4615; stack trace: *** @ 0x7fea1c3f7cb0 (unknown) at ??:0 @ 0x7fea1c3f7c37 gsignal at ??:0 @ 0x7fea1c3fb028 abort at ??:0 @ 0x7fea1d4b9919 google::logging_fail() at ??:0 @ 0x7fea1d4bb13d google::LogMessage::Fail() at ??:0 @ 0x7fea1d4bd15c google::LogMessage::SendToLog() at ??:0 @ 0x7fea1d4bac99 google::LogMessage::Flush() at ??:0 @ 0x7fea1d4bdaef google::LogMessageFatal::~LogMessageFatal() at ??:0 @ 0x7fea2083eee8 kudu::tablet::WriteOpState::ReleaseTxResultPB() at ??:0 @ 0x4783f4 kudu::tablet::LocalTabletWriter::WriteBatch() at /home/jenkins-slave/workspace/kudu-master/2/src/kudu/tablet/local_tablet_writer.h:112 @ 0x477fac kudu::tablet::LocalTabletWriter::Write() at /home/jenkins-slave/workspace/kudu-master/2/src/kudu/tablet/local_tablet_writer.h:90 (discriminator 1) @ 0x477f4c kudu::tablet::LocalTabletWriter::Insert() at /home/jenkins-slave/workspace/kudu-master/2/src/kudu/tablet/local_tablet_writer.h:66 -- To view, visit http://gerrit.cloudera.org:8080/16147 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78698d4cb4944bddd8dabd6cbbf1e3a064056199 Gerrit-Change-Number: 16147 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 08 Jul 2020 06:07:43 + Gerrit-HasComments: No
