[kudu-CR] KUDU-2514 Part 1: Support extra config for table.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12468 ) Change subject: KUDU-2514 Part 1: Support extra config for table. .. Patch Set 19: (4 comments) For the most part looks good. Mostly nits. http://gerrit.cloudera.org:8080/#/c/12468/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java: http://gerrit.cloudera.org:8080/#/c/12468/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@32 PS19, Line 32: import java.util.HashMap; nit: could you stick this before java.util.List so it's alphabetical? http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc@656 PS19, Line 656: kudu. Why "table" and why "kudu"? The config maps are *Table*ExtraConfigPB. Do we expect these configs to ever _not_ be table configs or Kudu configs? Since these will be stored on disk, even though it's a small amount of space, could we do without the "kudu.table"? Or does it provide value via uniqueness of keys in some way? http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc@668 PS19, Line 668: config.first nit: It would be more straightforward if you pulled out config.first and config.second into their own variables, e.g. const string& config_name = config.first; const string& config_value = config.second; http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc@672 PS19, Line 672: history_max_age_sec nit: Should this be "kudu.table.history_max_age_sec"? Maybe use Substitute() and pass in config_name too (if you decide to take my suggestion). -- To view, visit http://gerrit.cloudera.org:8080/12468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0514507dca95602a97e954d1db464b907e073aae Gerrit-Change-Number: 12468 Gerrit-PatchSet: 19 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Fri, 07 Jun 2019 06:19:11 + Gerrit-HasComments: Yes
[kudu-CR] sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13552 ) Change subject: sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/13552/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13552/2//COMMIT_MSG@26 PS2, Line 26: where Nit: when http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_authz_provider.h@125 PS2, Line 125:bool require_grant_option = false, :bool cache_table_column_privileges = true); Consider enum-ifying these. Two bools is rough, and although annotations help they also make the code more verbose. http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_authz_provider.cc@192 PS2, Line 192: tables columns? http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_privileges_fetcher.h@174 PS2, Line 174: bool cache_table_column_privileges, Doc (and consider enum-ifying). http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13552/2/src/kudu/master/sentry_privileges_fetcher.cc@595 PS2, Line 595: Status SentryPrivilegesFetcher::GetSentryPrivileges( May want to consider decomposing this; it's getting pretty tough to follow all the things that it does. -- To view, visit http://gerrit.cloudera.org:8080/13552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 Gerrit-Change-Number: 13552 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 07 Jun 2019 06:16:54 + Gerrit-HasComments: Yes
[kudu-CR] sentry: avoid authorizing every table in ListTables
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13549 ) Change subject: sentry: avoid authorizing every table in ListTables .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@210 PS3, Line 210: TableMetadataLock ltm(table_info.get(), LockMode::READ); > It feels really sketchy to see this abstraction leak out of CatalogManager. Another thing to consider: there's an invariant that we follow in the catalog manager where we only authorize tables while they're locked. That's a brutal proposition for any sort of grouping in ListTables though. Even if we get the plumbing right, it'll just be bad for concurrency to lock a bunch of tables while authorizing their database. -- To view, visit http://gerrit.cloudera.org:8080/13549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 Gerrit-Change-Number: 13549 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 07 Jun 2019 06:11:43 + Gerrit-HasComments: Yes
[kudu-CR] sentry: avoid authorizing every table in ListTables
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13549 )
Change subject: sentry: avoid authorizing every table in ListTables
..
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/authz_provider.h@28
PS3, Line 28: #include "kudu/master/catalog_manager.h"
Why do we need this? Can't we forward declare TableInfo?
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc@400
PS3, Line 400: .
Nit: extra period here.
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc@401
PS3, Line 401: const string dummy_name = Substitute("$0_$1", "foo", d);
: if (FLAGS_has_db_privileges) {
:
ASSERT_OK(AlterRoleGrantPrivilege(GetDatabasePrivilege(db_name, "METADATA")));
: } else {
:
ASSERT_OK(AlterRoleGrantPrivilege(GetDatabasePrivilege(dummy_name,
"METADATA")));
: }
Rewrite:
ASSERT_OK(AlterRoleGrantPrivilege(GetDatabasePrivilege(FLAGS_has_db_privileges
? db_name : Substitute("foo_$0", d), "METADATA")));
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider-test.cc@409
PS3, Line 409: ,d
Nit: , d
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@204
PS3, Line 204: map>> tables_by_db;
Could be unordered_map, unless you want alphanumeric ordering for the METADATA
ON DATABASE authz checks below.
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@210
PS3, Line 210: TableMetadataLock ltm(table_info.get(), LockMode::READ);
It feels really sketchy to see this abstraction leak out of CatalogManager.
Could we just pass a vector of pair into this function
(after having locked and filtered the tables one by one), then parse and group
by database here? Why do we need to have a table locked during the parse?
Alternatively, we can pass a pre-filled ListTablesResponsePB into this function
and make the contract something like "the authz provider gets to filter out
entries". Then there's less additional marshalling/unmarshalling of data, and
you don't need the duplicated code in default_authz_provider.cc.
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@233
PS3, Line 233: const string& first_table_name_in_db = tables_in_db[0].first;
Does this need to be deterministic? Like, should we sort tables_in_db or
something?
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@234
PS3, Line 234: if (user) {
Is this check correct? Previously we would include all tables in the response
if user was unset.
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@235
PS3, Line 235: scoped_refptr table_info;
Unused.
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@239
PS3, Line 239: first_table_name_in_db
I think I know why you're providing the table name and not database name here
(to avoid getting privileges for non-Kudu tables sharing the database), but
will it be obvious to anyone reading this?
http://gerrit.cloudera.org:8080/#/c/13549/3/src/kudu/master/sentry_authz_provider.cc@253
PS3, Line 253: ListTablesResponsePB::TableInfo* table =
pb->add_tables();
: table->set_name(name_and_id.first);
: table->set_id(name_and_id.second);
May be able to save a few LOC with a lambda.
--
To view, visit http://gerrit.cloudera.org:8080/13549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527
Gerrit-Change-Number: 13549
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 07 Jun 2019 06:05:28 +
Gerrit-HasComments: Yes
[kudu-CR] [java] small fixes for diff scan setup logic
Hello Will Berkeley, Mike Percy, Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/13553 to review the following change. Change subject: [java] small fixes for diff scan setup logic .. [java] small fixes for diff scan setup logic Change-Id: Ie6775221115b078d4b2eb640a15d34ee4f315e27 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java 1 file changed, 9 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/13553/1 -- To view, visit http://gerrit.cloudera.org:8080/13553 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie6775221115b078d4b2eb640a15d34ee4f315e27 Gerrit-Change-Number: 13553 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2809 (5/6): add diff scan support to fuzz-itest
Hello Will Berkeley, Tidy Bot, Mike Percy, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13537 to look at the new patch set (#4). Change subject: KUDU-2809 (5/6): add diff scan support to fuzz-itest .. KUDU-2809 (5/6): add diff scan support to fuzz-itest This patch adds diff scans to the set of operations supported by fuzz-itest. The existing saved_values_ map is quite difficult to reuse for verifying diff scan results, so instead I added a saved_redos_ map that tracks every insertion/mutation as a discrete "redo". For coverage, there are two new tests for KUDU-2809. I also ran fuzz-itest in slow mode a few thousands times on this patch series without failure. Change-Id: I3f7dae20ef1b903dba80e90d5f491e4322815fbb --- M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/key_value_test_schema.h 2 files changed, 262 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/13537/4 -- To view, visit http://gerrit.cloudera.org:8080/13537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f7dae20ef1b903dba80e90d5f491e4322815fbb Gerrit-Change-Number: 13537 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] fuzz-itest: assorted cleanup
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13532 ) Change subject: fuzz-itest: assorted cleanup .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/13532/2/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: http://gerrit.cloudera.org:8080/#/c/13532/2/src/kudu/integration-tests/fuzz-itest.cc@118 PS2, Line 118: TEST_DIFF_SCAN, > accidental inclusion? it's missing in the string names and the switch Indeed. I'll move it to the appropriate patch. -- To view, visit http://gerrit.cloudera.org:8080/13532 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1792f4f2d83403c9ae57acb29190246d5f036e69 Gerrit-Change-Number: 13532 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 07 Jun 2019 05:52:07 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2809 (3/6): C++ private API for diff scans
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13535 )
Change subject: KUDU-2809 (3/6): C++ private API for diff scans
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1337
PS2, Line 1337: Status KuduScanner::SetDiffScan(uint64_t start_timestamp,
uint64_t end_timestamp) {
> Yeah, validating (start <= end) would be a good fit for config->SetDiffScan
That's not done in the Java client, but I'll add it here (and to the Java
client in a separate patch).
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1737
PS2, Line 1737:
data_->mutable_configuration()->SetSnapshotRaw(snapshot_timestamp);
: return Status::OK();
: }
> Maybe == kNoTimestamp is better (also equal to 0)
Done
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1740
PS2, Line 1740:
> maybe just validate that start <= end ... also consider moving all of this
Done
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/scan_batch.h@189
PS2, Line 189: /// @return Operation result status. Return a bad Status if at
least one
> The user won't know the name/index here right? Can this be handled like Row
Sure, though it'll require more complicated schema surgery.
--
To view, visit http://gerrit.cloudera.org:8080/13535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I03bd5e51c553b239e36fbdfc4a94131c9599851e
Gerrit-Change-Number: 13535
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:51:57 +
Gerrit-HasComments: Yes
[kudu-CR] schema: cache first is deleted virtual column index
Hello Will Berkeley, Mike Percy, Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/13554 to review the following change. Change subject: schema: cache first is_deleted virtual column index .. schema: cache first is_deleted virtual column index This mirrors what we do in Java, and will make it easier to implement KuduScanBatch::RowPtr::IsDeleted. Change-Id: I018daa0c432f909942d5107df1acab3477788683 --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/memrowset.cc M src/kudu/tablet/tablet-test-util.h 8 files changed, 40 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/13554/1 -- To view, visit http://gerrit.cloudera.org:8080/13554 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I018daa0c432f909942d5107df1acab3477788683 Gerrit-Change-Number: 13554 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2809 (2/6): skip unobservable rows when iterating an MRS
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13534 ) Change subject: KUDU-2809 (2/6): skip unobservable rows when iterating an MRS .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/13534/2/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/13534/2/src/kudu/tablet/memrowset.h@586 PS2, Line 586: row : // row > nit: s/row row/row/ Done http://gerrit.cloudera.org:8080/#/c/13534/2/src/kudu/tablet/memrowset.h@593 PS2, Line 593: bool insert_excluded, > nit: maybe doc this param I did; see L569-L570. It's a quirky style (bunching up the enum definition with the function) but I like that it "scopes" the enum to just this one usage. -- To view, visit http://gerrit.cloudera.org:8080/13534 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I801991260749d1f810540cb32ec84c3ea6a02160 Gerrit-Change-Number: 13534 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 07 Jun 2019 05:52:04 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2809 (3/6): C++ private API for diff scans
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13535 to look at the new patch set (#3). Change subject: KUDU-2809 (3/6): C++ private API for diff scans .. KUDU-2809 (3/6): C++ private API for diff scans This patch adds just enough of a private diff scan API so that fuzz-itest can use it. I modeled it after Grant's Java work in commit 5e78e4a5c. No tests here, but this gets plenty of test coverage in an upcoming fuzz-itest patch, and I think that's fine given fuzz-itest is the only thing motivating the C++ API in the first place. Change-Id: I03bd5e51c553b239e36fbdfc4a94131c9599851e --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/scan_batch.cc M src/kudu/client/scan_batch.h M src/kudu/client/scan_configuration.cc M src/kudu/client/scan_configuration.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/scanner-internal.cc 8 files changed, 146 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/13535/3 -- To view, visit http://gerrit.cloudera.org:8080/13535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I03bd5e51c553b239e36fbdfc4a94131c9599851e Gerrit-Change-Number: 13535 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2809 (2/6): skip unobservable rows when iterating an MRS
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13534 to look at the new patch set (#3). Change subject: KUDU-2809 (2/6): skip unobservable rows when iterating an MRS .. KUDU-2809 (2/6): skip unobservable rows when iterating an MRS An unobservable row is one whose entire lifespan is contained within the time range of a diff scan. Such rows should be hidden from the diff scan results as they represent "changes" that the client wouldn't expect to see. This patch adds such a heuristic to the MRS iterator. It's relatively straight-forward because the MRS only stores REDOs and always remembers its rows' insertion timestamps. Delta stores are more complicated; a patch for handling unobservable rows for them will follow. Change-Id: I801991260749d1f810540cb32ec84c3ea6a02160 --- M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test.cc 4 files changed, 132 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/13534/3 -- To view, visit http://gerrit.cloudera.org:8080/13534 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I801991260749d1f810540cb32ec84c3ea6a02160 Gerrit-Change-Number: 13534 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] fuzz-itest: assorted cleanup
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13532 to look at the new patch set (#3). Change subject: fuzz-itest: assorted cleanup .. fuzz-itest: assorted cleanup No functional changes here, though this does change the textual representation of a fuzz-itest op. Change-Id: I1792f4f2d83403c9ae57acb29190246d5f036e69 --- M src/kudu/integration-tests/fuzz-itest.cc 1 file changed, 142 insertions(+), 116 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/13532/3 -- To view, visit http://gerrit.cloudera.org:8080/13532 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1792f4f2d83403c9ae57acb29190246d5f036e69 Gerrit-Change-Number: 13532 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] Support SPNEGO for web server
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13341 ) Change subject: Support SPNEGO for web server .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/13341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7 Gerrit-Change-Number: 13341 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 07 Jun 2019 05:49:55 + Gerrit-HasComments: No
[kudu-CR] Support SPNEGO for web server
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13341 ) Change subject: Support SPNEGO for web server .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc File src/kudu/security/gssapi.cc: http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@144 PS3, Line 144: *complete = s.ok(); > interesting. The manpage indicates it does need to be freed. Not sure why L Added by yours truly: // KUDU-2653: Memory leak in libgssapi_krb5 [1]. Exists in certain patched // versions of krb5-1.12 (such as krb5 in Debian 8). // // Unfortunately there's no narrower match without resorting to // fast_unwind_on_malloc=0; the best alternative is to match on glob, but that // seems like overkill too. // // 1. http://krbdev.mit.edu/rt/Ticket/Display.html?id=7981 "leak:libgssapi_krb5.so.2\n"; -- To view, visit http://gerrit.cloudera.org:8080/13341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7 Gerrit-Change-Number: 13341 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 07 Jun 2019 05:27:08 + Gerrit-HasComments: Yes
[kudu-CR] Support SPNEGO for web server
Hello Thomas Marshall, Tidy Bot, Lars Volker, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13341 to look at the new patch set (#5). Change subject: Support SPNEGO for web server .. Support SPNEGO for web server SPNEGO is a protocol for securing HTTP requests with Kerberos by passing negotiation through HTTP headers. It's supported by most major browsers and also by most of the Java-based Hadoop components. Notably, it's also the typical way in which Apache Knox authenticates itself to Hadoop components in the "trusted proxy" mode, allowing them to be secured behind Knox's SSO and other policies. This patch implements the SPNEGO protocol by driving GSSAPI, and integrates it into the webserver when configured by a new --webserver_require_spnego flag. The new test verifies this end-to-end using curl's SPNEGO authentication support. Along the way I had to cross-port a small change to the Base64 functions in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of the same file. Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7 --- M src/kudu/gutil/strings/escaping.cc M src/kudu/rpc/client_negotiation.cc M src/kudu/security/CMakeLists.txt A src/kudu/security/gssapi.cc A src/kudu/security/gssapi.h M src/kudu/security/test/mini_kdc.cc M src/kudu/security/test/mini_kdc.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h M src/kudu/util/test_macros.h M thirdparty/build-definitions.sh 16 files changed, 625 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/5 -- To view, visit http://gerrit.cloudera.org:8080/13341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7 Gerrit-Change-Number: 13341 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Support SPNEGO for web server
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/13341 )
Change subject: Support SPNEGO for web server
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@144
PS3, Line 144: wlil
> nit: will
Done
http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc@287
PS4, Line 287: if (opts_.require_spnego) {
> IIRC, there is some sequence of searching for the server-side keytab. Coul
I believe at this point the --keytab_file is already propagated into
$KRB5_KTNAME by security::InitKerberosForServer. I think we could customize the
gss_accept_* calls to use some specific credential store/keytab if we wanted,
but for SASL purposes I remember it was basically impossible to do without just
setting this env var. So, here we're just relying on the same. I'll add a
comment.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h@95
PS3, Line 95: spnego_
> nit: 'use_spnego_' might be a better name from readability perspective
Done
--
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:17:46 +
Gerrit-HasComments: Yes
[kudu-CR] Support SPNEGO for web server
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13341 )
Change subject: Support SPNEGO for web server
..
Patch Set 4: Code-Review+1
(4 comments)
I didn't look deep at this patch, but overall looks good just a few nits.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@20
PS3, Line 20: #include
nit: #include
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@144
PS3, Line 144: wlil
nit: will
http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc@287
PS4, Line 287: if (!kt_file || !Env::Default()->FileExists(kt_file)) {
IIRC, there is some sequence of searching for the server-side keytab. Could
you add a comment explaining why to rely only on the highest-level override
here?
Also, what is the relation between data in file pointed by --keytab_file gflag
(i.e. client-side keytab) and this variable? Could they be the same or they
will always be different?
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h@95
PS3, Line 95: spnego_
nit: 'use_spnego_' might be a better name from readability perspective
--
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:11:59 +
Gerrit-HasComments: Yes
[kudu-CR] Add seek before mode for CBTree to accelerate CheckRowDeleted in dms.
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/13516 )
Change subject: Add seek before mode for CBTree to accelerate CheckRowDeleted
in dms.
..
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc
File src/kudu/tablet/cbtree-test.cc:
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@797
PS2, Line 797: string key = "key" + std::to_string(i * 2);
nit: use Substitute("key$0", i * 2 + 1) here and below in a number of places
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@802
PS2, Line 802: exact = false;
you mean exact = true, right?
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@805
PS2, Line 805: gscoped_ptr > iter(
: t.NewIterator());
nit: no need to wrap here
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@817
PS2, Line 817: faild
typo
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@819
PS2, Line 819: gscoped_ptr > iter(
same style nit on wrapping (also below a few places)
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@828
PS2, Line 828: inexistent
non-existent
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/concurrent_btree.h
File src/kudu/tablet/concurrent_btree.h:
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/concurrent_btree.h@1615
PS2, Line 1615: enum SeekMode {
this can be in a private: block, right?
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/concurrent_btree.h@1631
PS2, Line 1631: // Find out the first idx in the very leafnode with key <=
given key.
agree with Adar this is a little confusing. I don't think we really need the
comment since the function is pretty straightforward and follows the one above.
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/deltamemstore.cc
File src/kudu/tablet/deltamemstore.cc:
http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/deltamemstore.cc@171
PS2, Line 171: }
can we add DCHECK(!exact) here? we never expect to hit an entry with
Timestamp::kMax
--
To view, visit http://gerrit.cloudera.org:8080/13516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icda5585c7226a075ffebdb22c7fc7728edf85feb
Gerrit-Change-Number: 13516
Gerrit-PatchSet: 2
Gerrit-Owner: ZhangYao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Yao Xu
Gerrit-Reviewer: ZhangYao
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:01:17 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13552 ) Change subject: sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes .. Patch Set 2: Verified+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/13552/1/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/13552/1/src/kudu/master/sentry_authz_provider.cc@195 PS1, Line 195:/*require_grant_option=*/false, > warning: argument name 'cache_table_column_privileges' in comment does not Done -- To view, visit http://gerrit.cloudera.org:8080/13552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 Gerrit-Change-Number: 13552 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 07 Jun 2019 04:55:11 + Gerrit-HasComments: Yes
[kudu-CR] sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13552 ) Change subject: sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes .. Patch Set 2: Unrelated flake. -- To view, visit http://gerrit.cloudera.org:8080/13552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 Gerrit-Change-Number: 13552 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 07 Jun 2019 04:55:18 + Gerrit-HasComments: No
[kudu-CR] sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes
Andrew Wong has removed a vote on this change. Change subject: sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 Gerrit-Change-Number: 13552 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [rebalance] Add '--known unhealthy tservers' flag to rebalancer
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13539 ) Change subject: [rebalance] Add '--known_unhealthy_tservers' flag to rebalancer .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/13539/1//COMMIT_MSG Commit Message: PS1: > Curious why you went with this approach vs. a boolean flag that just allowe Agreed with that. Or, can we just ignore any server which has been dead for more than N seconds? -- To view, visit http://gerrit.cloudera.org:8080/13539 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie83a7d2497b778833f3f96fa8aa20e7486c5ebbb Gerrit-Change-Number: 13539 Gerrit-PatchSet: 1 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 07 Jun 2019 04:53:21 + Gerrit-HasComments: Yes
[kudu-CR] Support SPNEGO for web server
Hello Thomas Marshall, Tidy Bot, Lars Volker, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13341 to look at the new patch set (#4). Change subject: Support SPNEGO for web server .. Support SPNEGO for web server SPNEGO is a protocol for securing HTTP requests with Kerberos by passing negotiation through HTTP headers. It's supported by most major browsers and also by most of the Java-based Hadoop components. Notably, it's also the typical way in which Apache Knox authenticates itself to Hadoop components in the "trusted proxy" mode, allowing them to be secured behind Knox's SSO and other policies. This patch implements the SPNEGO protocol by driving GSSAPI, and integrates it into the webserver when configured by a new --webserver_require_spnego flag. The new test verifies this end-to-end using curl's SPNEGO authentication support. Along the way I had to cross-port a small change to the Base64 functions in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of the same file. Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7 --- M src/kudu/gutil/strings/escaping.cc M src/kudu/rpc/client_negotiation.cc M src/kudu/security/CMakeLists.txt A src/kudu/security/gssapi.cc A src/kudu/security/gssapi.h M src/kudu/security/test/mini_kdc.cc M src/kudu/security/test/mini_kdc.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h M src/kudu/util/test_macros.h M thirdparty/build-definitions.sh 16 files changed, 622 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/4 -- To view, visit http://gerrit.cloudera.org:8080/13341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7 Gerrit-Change-Number: 13341 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Support SPNEGO for web server
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/13341 )
Change subject: Support SPNEGO for web server
..
Patch Set 3:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt@85
PS3, Line 85: set(SECURITY_LIBS
: gutil
: kudu_util
: token_proto
:
: krb5
: gssapi_krb5
: openssl_crypto
: openssl_ssl)
> Nit: while you're here, could you resort this alphabetically?
done, but left the kudu libs separate from the thirdparty ones.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h
File src/kudu/security/gssapi.h:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@35
PS3, Line 35: header
> Nit: did you mean 'token' here? Otherwise there's not enough context to und
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@42
PS3, Line 42: '*complete' indicates whether any further rounds are required. On
: // completion of negotiation,
> I assume that 'complete' is only touched in the event of an OK status?
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc
File src/kudu/security/gssapi.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@54
PS3, Line 54: gss_release_buffer(&minor, &buf);
> Maybe also add a comment saying that we can't use the more natural ReleaseB
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@144
PS3, Line 144: gss_buffer_desc name_buf {0, nullptr};
> Does this need to be freed after the call to std::string::assign? If not, c
interesting. The manpage indicates it does need to be freed. Not sure why LSAN
didn't catch it -- perhaps because we have some LSAN suppressions for other
known leaks in gssapi. Good catch.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@20
PS3, Line 20: #include
> Nit: cstdlib instead
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@166
PS3, Line 166: return Status::OK();
> Do you also want coverage for the case where the callback returns a bad Sta
the current use case for this thing is just for tests, so I'll make it a void
callback instead of returning Status. I'll extend it to return Status later if
we find an authorization use case for it.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@204
PS3, Line 204:
> Nit: extra blank line here.
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@238
PS3, Line 238: well-formed token
> Should probably prove that you can authenticate using this token first. Oth
well, the token here is well-formed but invalid, since each run of the test is
a different KDC, principal, etc. But it still turned up overflow bugs just
during parsing. I'll add a note indicating this.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@143
PS3, Line 143: ,
> Nit: misplaced comma
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@153
PS3, Line 153: neg_token = "";
> This is neg_token's default value; you can invert the condition and combine
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@459
PS3, Line 459: s = Status::RuntimeError("SPNEGO indicated complete, but
got empty principal");
> Any particular reason the status message shouldn't also include the address
the status here would just get propagated back to the requester as part of the
HTTP response, and the requester already knows their own IP.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h
File src/kudu/server/webserver_options.h:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h@23
PS3, Line 23: #include
> Nit: since you're here already, could you convert this into cstdint?
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc
File src/kudu/server/webserver_options.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc@111
PS3, Line 111:
> Nit: extra blank line here.
Done
http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh@653
PS3, Line 653: # Configure for a very minimal install - basically only
HTTP(S), since we only
: # use this
[kudu-CR] [backup] Make KuduBackupCLI executable
Hello Will Berkeley, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13551 to look at the new patch set (#2). Change subject: [backup] Make KuduBackupCLI executable .. [backup] Make KuduBackupCLI executable This patch changes the KuduBackupCLI to be the defacto main class of the kudu-backup-tools. It does this by combining both the GC tool and the list tool into a single CLI parser. I tested this by running: `java -jar build/libs/kudu-backup-tools-1.10.0-SNAPSHOT.jar` Change-Id: I15e0270a94844ffb7cb804a182dcf9699dd12ec7 --- M java/kudu-backup-tools/build.gradle M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCLI.scala M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCleaner.scala A java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupLister.scala M java/kudu-backup-tools/src/test/scala/org/apache/kudu/backup/TestKuduBackupCleaner.scala R java/kudu-backup-tools/src/test/scala/org/apache/kudu/backup/TestKuduBackupLister.scala 6 files changed, 275 insertions(+), 229 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/13551/2 -- To view, visit http://gerrit.cloudera.org:8080/13551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I15e0270a94844ffb7cb804a182dcf9699dd12ec7 Gerrit-Change-Number: 13551 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] Support SPNEGO for web server
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13341 )
Change subject: Support SPNEGO for web server
..
Patch Set 3:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt@85
PS3, Line 85: set(SECURITY_LIBS
: gutil
: kudu_util
: token_proto
:
: krb5
: gssapi_krb5
: openssl_crypto
: openssl_ssl)
Nit: while you're here, could you resort this alphabetically?
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h
File src/kudu/security/gssapi.h:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@35
PS3, Line 35: header
Nit: did you mean 'token' here? Otherwise there's not enough context to
understand what 'header' means.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@42
PS3, Line 42: '*complete' indicates whether any further rounds are required. On
: // completion of negotiation,
I assume that 'complete' is only touched in the event of an OK status?
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc
File src/kudu/security/gssapi.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@54
PS3, Line 54: gss_release_buffer(&minor, &buf);
Maybe also add a comment saying that we can't use the more natural
ReleaseBufferOrWarn here because it'd call right back into ErrorToString.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@144
PS3, Line 144: gss_buffer_desc name_buf {0, nullptr};
Does this need to be freed after the call to std::string::assign? If not, could
you add a comment explaining?
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@20
PS3, Line 20: #include
Nit: cstdlib instead
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@166
PS3, Line 166: return Status::OK();
Do you also want coverage for the case where the callback returns a bad Status
and causes authentication to fail?
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@204
PS3, Line 204:
Nit: extra blank line here.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@238
PS3, Line 238: well-formed token
Should probably prove that you can authenticate using this token first.
Otherwise the various token failure tests below are less credible because the
token was never going to work anyway.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@143
PS3, Line 143: ,
Nit: misplaced comma
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@153
PS3, Line 153: neg_token = "";
This is neg_token's default value; you can invert the condition and combine it
with the condition in the else if.
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@459
PS3, Line 459: s = Status::RuntimeError("SPNEGO indicated complete, but
got empty principal");
Any particular reason the status message shouldn't also include the address as
per the DFATAL just below?
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h
File src/kudu/server/webserver_options.h:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h@23
PS3, Line 23: #include
Nit: since you're here already, could you convert this into cstdint?
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc
File src/kudu/server/webserver_options.cc:
http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc@111
PS3, Line 111:
Nit: extra blank line here.
http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh@653
PS3, Line 653: # Configure for a very minimal install - basically only
HTTP(S), since we only
: # use this for testing our own HTTP/HTTPS endpoints at this
point in time.
May want to update this.
--
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Ku
[kudu-CR] [backup] Make KuduBackupCLI format optional
Grant Henke has removed a vote on this change. Change subject: [backup] Make KuduBackupCLI format optional .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I15e0270a94844ffb7cb804a182dcf9699dd12ec7 Gerrit-Change-Number: 13551 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] [backup] Make KuduBackupCLI format optional
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13551 ) Change subject: [backup] Make KuduBackupCLI format optional .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15e0270a94844ffb7cb804a182dcf9699dd12ec7 Gerrit-Change-Number: 13551 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 07 Jun 2019 02:15:38 + Gerrit-HasComments: No
[kudu-CR] [backup] Fix backup CLI dependencies
Grant Henke has removed a vote on this change. Change subject: [backup] Fix backup CLI dependencies .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I6bee1f467d5652ced8412eef0315057e5b0d684e Gerrit-Change-Number: 13547 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] [backup] Fix backup CLI dependencies
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13547 ) Change subject: [backup] Fix backup CLI dependencies .. Patch Set 2: Verified+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/13547/1/java/kudu-backup-tools/build.gradle File java/kudu-backup-tools/build.gradle: http://gerrit.cloudera.org:8080/#/c/13547/1/java/kudu-backup-tools/build.gradle@52 PS1, Line 52: > Can we make this an executable JAR? Something to the effect of: I will do that in a follow up patch. The way it's structured now there are two CLIs. KuduBackupCLI and KuduBackupCleaner. http://gerrit.cloudera.org:8080/#/c/13547/1/java/kudu-backup-tools/build.gradle@53 PS1, Line 53: > ? oops. -- To view, visit http://gerrit.cloudera.org:8080/13547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6bee1f467d5652ced8412eef0315057e5b0d684e Gerrit-Change-Number: 13547 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 07 Jun 2019 02:11:23 + Gerrit-HasComments: Yes
[kudu-CR] sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13552 to look at the new patch set (#2). Change subject: sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes .. sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes Currently, Kudu will not cache COLUMN/TABLE when checking for DATABASE/SERVER Sentry privileges. This is because, when written, Kudu would send requests that would yield significantly more privileges than required. Commit 0040a731e4741bde6d8c9d81e796cb000cd24817 changed this behavior, and as such, this patch gates the optimization behind a boolean in the Sentry provider's Authorize() signature. The effect of this is that when checking for DATABASE/SERVER privileges, two entries will be added to the privilege cache instead of one: one for the DATABASE/SERVER privileges, and one for the TABLE/COLUMN privileges. This is optional because in some cases, it isn't necessarily desirable to cache table-level privileges. E.g. when creating a table, we shouldn't cache table-level privileges because Sentry may create OWNER privileges for the new table that caching may miss out on when first authorizing the create request. This improves the worst case performance of ListTables where there are many databases on which the user has no privileges, and a single table per database. In this scenario, without this patch, there would be 2 * (# tables) calls to Sentry, and with this patch, there would be 1 * (# tables) calls. With this patch: ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=false sentry_authz_provider-test.cc:424] Time spent Listing tables: real 23.203s user 0.028s sys 0.004s Without this patch: ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=false sentry_authz_provider-test.cc:420] Time spent Listing tables: real 44.448s user 0.037s sys 0.018s Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 --- M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/master/sentry_authz_provider.h M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 5 files changed, 65 insertions(+), 45 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/13552/2 -- To view, visit http://gerrit.cloudera.org:8080/13552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 Gerrit-Change-Number: 13552 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2809 (3/6): C++ private API for diff scans
Mike Percy has posted comments on this change. (
http://gerrit.cloudera.org:8080/13535 )
Change subject: KUDU-2809 (3/6): C++ private API for diff scans
..
Patch Set 2: Code-Review+1
(3 comments)
looks good modulo a couple nits and grant's suggestions
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1337
PS2, Line 1337: Status KuduScanner::SetDiffScan(uint64_t start_timestamp,
uint64_t end_timestamp) {
> Should we verify start is before end?
Yeah, validating (start <= end) would be a good fit for config->SetDiffScan()
along with the value validation done below
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1737
PS2, Line 1737: if (start_timestamp == 0) {
: return Status::IllegalState("Start timestamp must be set
bigger than 0");
: }
Maybe == kNoTimestamp is better (also equal to 0)
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1740
PS2, Line 1740: if (end_timestamp == 0) {
maybe just validate that start <= end ... also consider moving all of this into
config->SetDiffScan() as suggested in another comment
--
To view, visit http://gerrit.cloudera.org:8080/13535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I03bd5e51c553b239e36fbdfc4a94131c9599851e
Gerrit-Change-Number: 13535
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 07 Jun 2019 01:51:41 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2809 (2/6): skip unobservable rows when iterating an MRS
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13534 ) Change subject: KUDU-2809 (2/6): skip unobservable rows when iterating an MRS .. Patch Set 2: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/13534/2/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/13534/2/src/kudu/tablet/memrowset.h@586 PS2, Line 586: row : // row nit: s/row row/row/ http://gerrit.cloudera.org:8080/#/c/13534/2/src/kudu/tablet/memrowset.h@593 PS2, Line 593: bool insert_excluded, nit: maybe doc this param -- To view, visit http://gerrit.cloudera.org:8080/13534 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I801991260749d1f810540cb32ec84c3ea6a02160 Gerrit-Change-Number: 13534 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 07 Jun 2019 00:55:13 + Gerrit-HasComments: Yes
[kudu-CR] sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes
Hello Alexey Serbin, Hao Hao, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/13552 to review the following change. Change subject: sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes .. sentry: allow caching of COLUMN/TABLE privileges when checking higher scopes Currently, Kudu will not cache COLUMN/TABLE when checking for DATABASE/SERVER Sentry privileges. This is because, when written, Kudu would send requests that would yield significantly more privileges than required. Commit 0040a731e4741bde6d8c9d81e796cb000cd24817 changed this behavior, and as such, this patch gates the optimization behind a boolean in the Sentry provider's Authorize() signature. The effect of this is that when checking for DATABASE/SERVER privileges, two entries will be added to the privilege cache instead of one: one for the DATABASE/SERVER privileges, and one for the TABLE/COLUMN privileges. This is optional because in some cases, it isn't necessarily desirable to cache table-level privileges. E.g. when creating a table, we shouldn't cache table-level privileges because Sentry may create OWNER privileges for the new table that caching may miss out on when first authorizing the create request. This improves the worst case performance of ListTables where there are many databases on which the user has no privileges, and a single table per database. In this scenario, without this patch, there would be 2 * (# tables) calls to Sentry, and with this patch, there would be 1 * (# tables) calls. With this patch: ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=false sentry_authz_provider-test.cc:424] Time spent Listing tables: real 23.203s user 0.028s sys 0.004s Without this patch: ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=false sentry_authz_provider-test.cc:420] Time spent Listing tables: real 44.448s user 0.037s sys 0.018s Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 --- M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/master/sentry_authz_provider.h M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 5 files changed, 64 insertions(+), 45 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/13552/1 -- To view, visit http://gerrit.cloudera.org:8080/13552 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icec75ae9e5626c887af37568a6f64a8361d888b7 Gerrit-Change-Number: 13552 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao
[kudu-CR] sentry: avoid authorizing every table in ListTables
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13549 ) Change subject: sentry: avoid authorizing every table in ListTables .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13549/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13549/1//COMMIT_MSG@32 PS1, Line 32: patch. A follow-up patch will make this more performant by caching : privileges from requests at the database scope. : : Iterating through one database and no tables since the user has > I'm still digging into why this is so close to the third case, since I foun I was expecting this to be lower than the third case since it's only doing a lookup on the database. I hadn't taken into account that when running the test and granting database-level privileges, there'd be more stuff in Sentry, so authorization is slightly slower. I've updated this in the new revision. -- To view, visit http://gerrit.cloudera.org:8080/13549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 Gerrit-Change-Number: 13549 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 07 Jun 2019 00:36:30 + Gerrit-HasComments: Yes
[kudu-CR] sentry: avoid authorizing every table in ListTables
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13549 to look at the new patch set (#3). Change subject: sentry: avoid authorizing every table in ListTables .. sentry: avoid authorizing every table in ListTables Currently ListTables will call into Sentry for every table in Kudu's catalog, checking whether the user has metadata privileges on the table, and adding it to the ListTablesResponsePB if so. This is expensive, particularly when there are thousands of tables in Kudu. This patch updates the authorization behavior to check whether the user has METADATA privileges on the table's database for each table. If no such privilege exists for the database, each table within the database is checked. A benchmark is added to gauge the performance in various scenarios: Iterating through many databases: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=true sentry_authz_provider-test.cc:420] Time spent Listing tables: real 21.184s user 0.023s sys 0.003s Iterating through many databases and many tables. This is the worst case since listing a given table will require two lookups in Sentry -- one for the database and one for the table: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=false sentry_authz_provider-test.cc:420] Time spent Listing tables: real 44.448s user 0.037s sys 0.018s This above worst case is actually less performant than without this patch. A follow-up patch will make this more performant by caching privileges from requests at the database scope. Iterating through one database and no tables since the user has database-level privileges: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=1 --num_tables_per_db=600 --has-db-privileges=true sentry_authz_provider-test.cc:420] Time spent Listing tables: real 0.313s user 0.000s sys 0.000s Iterating through one database and many tables: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=1 --num_tables_per_db=600 --has-db-privileges=false sentry_authz_provider-test.cc:420] Time spent Listing tables: real 43.180s user 0.031s sys 0.011s Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 --- M src/kudu/master/CMakeLists.txt M src/kudu/master/authz_provider.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/default_authz_provider.cc M src/kudu/master/default_authz_provider.h M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/master/sentry_authz_provider.h 9 files changed, 237 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/13549/3 -- To view, visit http://gerrit.cloudera.org:8080/13549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 Gerrit-Change-Number: 13549 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [backup] Make KuduBackupCLI format optional
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13551 Change subject: [backup] Make KuduBackupCLI format optional .. [backup] Make KuduBackupCLI format optional This patch changes the KuduBackupCLI to not require a positional format and instead use `--format` with a default value of “pretty”. Change-Id: I15e0270a94844ffb7cb804a182dcf9699dd12ec7 --- M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCLI.scala M java/kudu-backup-tools/src/test/scala/org/apache/kudu/backup/TestKuduBackupCLI.scala 2 files changed, 16 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/13551/1 -- To view, visit http://gerrit.cloudera.org:8080/13551 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I15e0270a94844ffb7cb804a182dcf9699dd12ec7 Gerrit-Change-Number: 13551 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] sentry: avoid authorizing every table in ListTables
Hello Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13549 to look at the new patch set (#2). Change subject: sentry: avoid authorizing every table in ListTables .. sentry: avoid authorizing every table in ListTables Currently ListTables will call into Sentry for every table in Kudu's catalog, checking whether the user has metadata privileges on the table, and adding it to the ListTablesResponsePB if so. This is expensive, particularly when there are thousands of tables in Kudu. This patch updates the authorization behavior to check whether the user has METADATA privileges on the table's database for each table. If no such privilege exists for the database, each table within the database is checked. A benchmark is added to gauge the performance in various scenarios: Iterating through many databases: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=true sentry_authz_provider-test.cc:420] Time spent Listing tables: real 21.184s user 0.023s sys 0.003s Iterating through many databases and many tables. This is the worst case since listing a given table will require two lookups in Sentry -- one for the database and one for the table: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=300 --num_tables_per_db=1 --has-db-privileges=false sentry_authz_provider-test.cc:420] Time spent Listing tables: real 44.448s user 0.037s sys 0.018s Iterating through one database and no tables since the user has database-level privileges: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=1 --num_tables_per_db=600 --has-db-privileges=true sentry_authz_provider-test.cc:420] Time spent Listing tables: real 0.313s user 0.000s sys 0.000s Iterating through one database and many tables: $ ./bin/sentry_authz_provider-test --gtest_filter=*ListTablesBench* --num_databases=1 --num_tables_per_db=600 --has-db-privileges=false sentry_authz_provider-test.cc:420] Time spent Listing tables: real 43.180s user 0.031s sys 0.011s Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 --- M src/kudu/master/CMakeLists.txt M src/kudu/master/authz_provider.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/default_authz_provider.cc M src/kudu/master/default_authz_provider.h M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/master/sentry_authz_provider.h 9 files changed, 218 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/13549/2 -- To view, visit http://gerrit.cloudera.org:8080/13549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 Gerrit-Change-Number: 13549 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [backup] Fix backup CLI dependencies
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13547 to look at the new patch set (#2). Change subject: [backup] Fix backup CLI dependencies .. [backup] Fix backup CLI dependencies This patch fixes the backup dependencies in two ways. First, it fixes the Gradle handling of `isTool` to ensure sl4j is included. Second it breaks out the common “library” type functionality into a kudu-backup-common module so it can be shared between the kudu-back and kudu-backup-tools module. This allows kudu-backup-tools to shade slf4j and log4j without affecting the kudu-backup dependencies. I tested this by running a build and then running the kudu-backup-tools localy via: `java -cp build/libs/kudu-backup-tools-1.10.0-SNAPSHOT.jar org.apache.kudu.backup.KuduBackupCLI` The patch also sneaks in some interface annotations. Change-Id: I6bee1f467d5652ced8412eef0315057e5b0d684e --- M java/gradle/shadow.gradle A java/kudu-backup-common/build.gradle R java/kudu-backup-common/src/main/protobuf/backup.proto R java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/BackupGraph.scala R java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/BackupIO.scala R java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/TableMetadata.scala A java/kudu-backup-common/src/test/resources/log4j2.properties R java/kudu-backup-common/src/test/scala/org/apache/kudu/backup/TestBackupGraph.scala M java/kudu-backup-tools/build.gradle A java/kudu-backup-tools/src/main/resources/log4j2.properties M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCLI.scala M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCleaner.scala M java/kudu-backup/build.gradle M java/settings.gradle 14 files changed, 160 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13547/2 -- To view, visit http://gerrit.cloudera.org:8080/13547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6bee1f467d5652ced8412eef0315057e5b0d684e Gerrit-Change-Number: 13547 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2785: Add splitSizeBytes to the backup job
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13511 ) Change subject: KUDU-2785: Add splitSizeBytes to the backup job .. KUDU-2785: Add splitSizeBytes to the backup job This patch adds an experimental and hidden option to use the new scanner splitSizeBytes feature in the backup job. Change-Id: If6b8d02b71b1f463e4d0d9e04203d8edbd5e016b Reviewed-on: http://gerrit.cloudera.org:8080/13511 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy --- M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala 3 files changed, 50 insertions(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If6b8d02b71b1f463e4d0d9e04203d8edbd5e016b Gerrit-Change-Number: 13511 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] sentry: don't send requests for DATABASE/SERVER privileges
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13494 ) Change subject: sentry: don't send requests for DATABASE/SERVER privileges .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78 Gerrit-Change-Number: 13494 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 22:59:06 + Gerrit-HasComments: No
[kudu-CR] sentry: don't send requests for DATABASE/SERVER privileges
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13494 ) Change subject: sentry: don't send requests for DATABASE/SERVER privileges .. sentry: don't send requests for DATABASE/SERVER privileges The API used to request privileges from Sentry will return more privileges than required for a given request. For example, if requesting privileges for a specific database, the API will return privileges for all ancestors and all descendents of that database, i.e. privileges for the server, the database, all tables in the database, and all columns therein. To remediate this, this patch narrows the scope of requests sent to Sentry to the table-level. Table-level seems fitting since every request for privileges Kudu makes to Sentry thus far is naturally scoped to a table anyway. Note: this patch doesn't touch any of the caching logic; it only tweaks the request sent to Sentry. I wrote a small benchmark[1] to gauge the effects of this patch. The time spent checking database-level privileges with 20K privilege entries in the database was 1.989 without this patch, and 0.491s with this patch. W0602 07:44:03.110574 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.861suser 0.000s sys 0.000s I0602 07:44:03.110697 13909 sentry_authz_provider-test.cc:739] num tables granted: 19998 W0602 07:44:06.829799 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.719suser 0.000s sys 0.000s I0602 07:44:06.829936 13909 sentry_authz_provider-test.cc:739] num tables granted: 1 W0602 07:44:10.521934 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.692suser 0.000s sys 0.000s W0602 07:44:12.375082 14301 sasl_client_transport.cc:160] Received large Thrift SASL frame: 2.33M W0602 07:44:12.438380 14301 sentry_client.cc:170] Time spent list Sentry privileges by user: real 1.892suser 0.054s sys 0.009s W0602 07:44:12.509138 13909 sentry_authz_provider.cc:275] Action on table with authorizable scope is not permitted for user I0602 07:44:12.510848 13909 sentry_authz_provider-test.cc:744] Time spent Getting database privileges: real 1.989s user 0.070s sys 0.002s W0602 07:44:13.002019 13909 sentry_authz_provider.cc:275] Action on table with authorizable scope is not permitted for user I0602 07:44:13.002111 13909 sentry_authz_provider-test.cc:750] Time spent Getting database privileges: real 0.491s user 0.000s sys 0.000s While the test numbers themselves may not be representative of a real Sentry deployment (since the test uses a Sentry minicluster instead of a real cluster) the numbers still point to reasonable improvement. A version of this benchmark is included in this patch. I opted to not gate the behavior behind a gflag, so rather than running successive privileges with different behavior, the included test only checks privileges once. [1] https://gist.github.com/andrwng/88f3a760441b4b37b3ff011e656683ab Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78 Reviewed-on: http://gerrit.cloudera.org:8080/13494 Tested-by: Kudu Jenkins Reviewed-by: Hao Hao --- M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 3 files changed, 68 insertions(+), 17 deletions(-) Approvals: Kudu Jenkins: Verified Hao Hao: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78 Gerrit-Change-Number: 13494 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. hms: allow for tooling to run without Kudu plugin Currently, whenever an HMS client successfully connects to the HMS, it checks the various HMS service configurations (e.g. that it is using the Kudu-HMS plugin), returning an error if any are misconfigured. This is important to make it much more obvious when the Kudu Master's HMS synchronization is misconfigured. It is still useful for other HMS clients (e.g. that used by the HMS tooling) to operate on an HMS instance that is not configured with the Kudu-HMS plugin et al. This patch removes the requirement by plumbing the option to the HMS client as a member of ThriftOptions. This was the most straightforward way to plumb this option from the HmsCatalog to the HmsClient, given the templating layered in between them for HA. Besides, we can use this option in the future if we ever want to verify the configuration of Thrift-based clients for other services (e.g. Sentry). This patch additionally allows the -hive_metastore_sasl_enabled flag to be used without the -keytab_file flag if not running kudu-master. To get this behavior, I've moved the gflag validator into master_main.cc, which is not built by tooling. I manually tested that it works, i.e. that tooling will not validate and that a master will. To test, I added an HmsMode that starts the HMS without the Kudu-HMS plugin installed and used it in a couple of HMS tooling tests. I considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to since some tests are greatly simplified by ENABLE_HIVE_METASTORE having the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to enable the Kudu-HMS integration). Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Reviewed-on: http://gerrit.cloudera.org:8080/13510 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/common/common.proto M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/master/master_main.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/server/server_base.cc M src/kudu/thrift/client.h M src/kudu/tools/kudu-tool-test.cc 15 files changed, 121 insertions(+), 65 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 22:56:22 + Gerrit-HasComments: No
[kudu-CR] KUDU-2809 (1/6): temporarily disable TestKuduScanner.testDiffScan
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13533 ) Change subject: KUDU-2809 (1/6): temporarily disable TestKuduScanner.testDiffScan .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I981aaa47a532dd0c1271008e27de052e3c5ad1a6 Gerrit-Change-Number: 13533 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 22:52:37 + Gerrit-HasComments: No
[kudu-CR] fuzz-itest: assorted cleanup
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13532 ) Change subject: fuzz-itest: assorted cleanup .. Patch Set 2: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/13532/2/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: http://gerrit.cloudera.org:8080/#/c/13532/2/src/kudu/integration-tests/fuzz-itest.cc@118 PS2, Line 118: TEST_DIFF_SCAN, accidental inclusion? it's missing in the string names and the switch -- To view, visit http://gerrit.cloudera.org:8080/13532 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1792f4f2d83403c9ae57acb29190246d5f036e69 Gerrit-Change-Number: 13532 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 22:51:58 + Gerrit-HasComments: Yes
[kudu-CR] sentry: avoid authorizing every table in ListTables
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13549 ) Change subject: sentry: avoid authorizing every table in ListTables .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/13549/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13549/1//COMMIT_MSG@32 PS1, Line 32: : With 300 database and 1 table in each database, with privileges on the : databases (warrants lookups on every database): : sentry_authz_provider-test.cc:414] Time spent Listing tables: real 22.363s user 0.021s sys 0.004s I'm still digging into why this is so close to the third case, since I found this surprising. -- To view, visit http://gerrit.cloudera.org:8080/13549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 Gerrit-Change-Number: 13549 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 22:45:26 + Gerrit-HasComments: Yes
[kudu-CR] sentry: avoid authorizing every table in ListTables
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13549 Change subject: sentry: avoid authorizing every table in ListTables .. sentry: avoid authorizing every table in ListTables Currently ListTables will call into Sentry for every table in Kudu's catalog, checking whether the user has metadata privileges on the table, and adding it to the ListTablesResponsePB if so. This is expensive, particularly when there are thousands of tables in Kudu. This patch updates the authorization behavior to check whether the user has METADATA privileges on the table's database for each table. If no such privilege exists for the database, each table within the database is checked. A benchmark is added to gauge the performance in various scenarios: With 1 database and 300 tables in the database, with privileges on the database (warrants only a lookup on the database): sentry_authz_provider-test.cc:414] Time spent Listing tables: real 0.253s user 0.000s sys 0.000s With 1 database and 300 tables in the database, without privileges on the database (warrants lookups on the database and each table): sentry_authz_provider-test.cc:414] Time spent Listing tables: real 11.707s user 0.016s sys 0.005s With 300 database and 1 table in each database, without privileges on the databases (warrants lookups on every database and every table): sentry_authz_provider-test.cc:414] Time spent Listing tables: real 22.982s user 0.043s sys 0.022s With 300 database and 1 table in each database, with privileges on the databases (warrants lookups on every database): sentry_authz_provider-test.cc:414] Time spent Listing tables: real 22.363s user 0.021s sys 0.004s Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 --- M src/kudu/master/CMakeLists.txt M src/kudu/master/authz_provider.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/default_authz_provider.cc M src/kudu/master/default_authz_provider.h M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/master/sentry_authz_provider.h 9 files changed, 212 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/13549/1 -- To view, visit http://gerrit.cloudera.org:8080/13549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I7c495c635fbd2a661709b6d7f04973b7864b5527 Gerrit-Change-Number: 13549 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] memrowset: skip all uncommitted mutations when iterating
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13531 ) Change subject: memrowset: skip all uncommitted mutations when iterating .. memrowset: skip all uncommitted mutations when iterating We already take advantage of this optimization the DMS and DeltaFiles; not sure why we weren't doing it in the MRS. Change-Id: I6f4049a1524e04660d853474a0c140c602c88d33 Reviewed-on: http://gerrit.cloudera.org:8080/13531 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke Reviewed-by: Mike Percy --- M src/kudu/tablet/memrowset.cc 1 file changed, 5 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Grant Henke: Looks good to me, approved Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13531 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I6f4049a1524e04660d853474a0c140c602c88d33 Gerrit-Change-Number: 13531 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] memrowset: skip all uncommitted mutations when iterating
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13531 ) Change subject: memrowset: skip all uncommitted mutations when iterating .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13531 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6f4049a1524e04660d853474a0c140c602c88d33 Gerrit-Change-Number: 13531 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 22:39:52 + Gerrit-HasComments: No
[kudu-CR] [backup] Fix backup CLI dependencies
Mike Percy has posted comments on this change. (
http://gerrit.cloudera.org:8080/13547 )
Change subject: [backup] Fix backup CLI dependencies
..
Patch Set 1: Code-Review+1
(2 comments)
Thanks for doing this!
http://gerrit.cloudera.org:8080/#/c/13547/1/java/kudu-backup-tools/build.gradle
File java/kudu-backup-tools/build.gradle:
http://gerrit.cloudera.org:8080/#/c/13547/1/java/kudu-backup-tools/build.gradle@52
PS1, Line 52:
Can we make this an executable JAR? Something to the effect of:
jar {
manifest {
attributes 'Main-Class': 'org.apache.kudu.backup.KuduBackupCLI'
}
}
http://gerrit.cloudera.org:8080/#/c/13547/1/java/kudu-backup-tools/build.gradle@53
PS1, Line 53: // java -cp build/libs/kudu-backup-tools-1.10.0-SNAPSHOT.jar
org.apache.kudu.backup.KuduBackupCLI --help
?
--
To view, visit http://gerrit.cloudera.org:8080/13547
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6bee1f467d5652ced8412eef0315057e5b0d684e
Gerrit-Change-Number: 13547
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Thu, 06 Jun 2019 22:38:04 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2785: Add splitSizeBytes to the backup job
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13511 ) Change subject: KUDU-2785: Add splitSizeBytes to the backup job .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/13511/4/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/13511/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@346 PS4, Line 346: // Wait for mrs flushed. : Thread.sleep(5 * 1000) > This matches the pattern for all of the other split size unit tests. It mak We should rely on metrics for this. But since we are cutting the branch tomorrow I'm ok with letting it slide. -- To view, visit http://gerrit.cloudera.org:8080/13511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6b8d02b71b1f463e4d0d9e04203d8edbd5e016b Gerrit-Change-Number: 13511 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 22:31:47 + Gerrit-HasComments: Yes
[kudu-CR] [backup] Fix backup CLI dependencies
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13547 Change subject: [backup] Fix backup CLI dependencies .. [backup] Fix backup CLI dependencies This patch fixes the backup dependencies in two ways. First, it fixes the Gradle handling of `isTool` to ensure sl4j is included. Second it breaks out the common “library” type functionality into a kudu-backup-common module so it can be shared between the kudu-back and kudu-backup-tools module. This allows kudu-backup-tools to shade slf4j and log4j without affecting the kudu-backup dependencies. I tested this by running a build and then running the kudu-backup-tools localy via: `java -cp build/libs/kudu-backup-tools-1.10.0-SNAPSHOT.jar org.apache.kudu.backup.KuduBackupCLI` The patch also sneaks in some interface annotations. Change-Id: I6bee1f467d5652ced8412eef0315057e5b0d684e --- M java/gradle/shadow.gradle A java/kudu-backup-common/build.gradle R java/kudu-backup-common/src/main/protobuf/backup.proto R java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/BackupGraph.scala R java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/BackupIO.scala R java/kudu-backup-common/src/main/scala/org/apache/kudu/backup/TableMetadata.scala A java/kudu-backup-common/src/test/resources/log4j2.properties R java/kudu-backup-common/src/test/scala/org/apache/kudu/backup/TestBackupGraph.scala M java/kudu-backup-tools/build.gradle A java/kudu-backup-tools/src/main/resources/log4j2.properties M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCLI.scala M java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/KuduBackupCleaner.scala M java/kudu-backup/build.gradle M java/settings.gradle 14 files changed, 163 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/13547/1 -- To view, visit http://gerrit.cloudera.org:8080/13547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I6bee1f467d5652ced8412eef0315057e5b0d684e Gerrit-Change-Number: 13547 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] KUDU-2809 (2/6): skip unobservable rows when iterating an MRS
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13534 ) Change subject: KUDU-2809 (2/6): skip unobservable rows when iterating an MRS .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/13534 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I801991260749d1f810540cb32ec84c3ea6a02160 Gerrit-Change-Number: 13534 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 22:18:02 + Gerrit-HasComments: No
[kudu-CR] sentry: don't send requests for DATABASE/SERVER privileges
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13494 to look at the new patch set (#7). Change subject: sentry: don't send requests for DATABASE/SERVER privileges .. sentry: don't send requests for DATABASE/SERVER privileges The API used to request privileges from Sentry will return more privileges than required for a given request. For example, if requesting privileges for a specific database, the API will return privileges for all ancestors and all descendents of that database, i.e. privileges for the server, the database, all tables in the database, and all columns therein. To remediate this, this patch narrows the scope of requests sent to Sentry to the table-level. Table-level seems fitting since every request for privileges Kudu makes to Sentry thus far is naturally scoped to a table anyway. Note: this patch doesn't touch any of the caching logic; it only tweaks the request sent to Sentry. I wrote a small benchmark[1] to gauge the effects of this patch. The time spent checking database-level privileges with 20K privilege entries in the database was 1.989 without this patch, and 0.491s with this patch. W0602 07:44:03.110574 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.861suser 0.000s sys 0.000s I0602 07:44:03.110697 13909 sentry_authz_provider-test.cc:739] num tables granted: 19998 W0602 07:44:06.829799 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.719suser 0.000s sys 0.000s I0602 07:44:06.829936 13909 sentry_authz_provider-test.cc:739] num tables granted: 1 W0602 07:44:10.521934 13909 sentry_client.cc:188] Time spent alter Sentry role grant privileges: real 3.692suser 0.000s sys 0.000s W0602 07:44:12.375082 14301 sasl_client_transport.cc:160] Received large Thrift SASL frame: 2.33M W0602 07:44:12.438380 14301 sentry_client.cc:170] Time spent list Sentry privileges by user: real 1.892suser 0.054s sys 0.009s W0602 07:44:12.509138 13909 sentry_authz_provider.cc:275] Action on table with authorizable scope is not permitted for user I0602 07:44:12.510848 13909 sentry_authz_provider-test.cc:744] Time spent Getting database privileges: real 1.989s user 0.070s sys 0.002s W0602 07:44:13.002019 13909 sentry_authz_provider.cc:275] Action on table with authorizable scope is not permitted for user I0602 07:44:13.002111 13909 sentry_authz_provider-test.cc:750] Time spent Getting database privileges: real 0.491s user 0.000s sys 0.000s While the test numbers themselves may not be representative of a real Sentry deployment (since the test uses a Sentry minicluster instead of a real cluster) the numbers still point to reasonable improvement. A version of this benchmark is included in this patch. I opted to not gate the behavior behind a gflag, so rather than running successive privileges with different behavior, the included test only checks privileges once. [1] https://gist.github.com/andrwng/88f3a760441b4b37b3ff011e656683ab Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78 --- M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 3 files changed, 68 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13494/7 -- To view, visit http://gerrit.cloudera.org:8080/13494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78 Gerrit-Change-Number: 13494 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13510 to look at the new patch set (#6). Change subject: hms: allow for tooling to run without Kudu plugin .. hms: allow for tooling to run without Kudu plugin Currently, whenever an HMS client successfully connects to the HMS, it checks the various HMS service configurations (e.g. that it is using the Kudu-HMS plugin), returning an error if any are misconfigured. This is important to make it much more obvious when the Kudu Master's HMS synchronization is misconfigured. It is still useful for other HMS clients (e.g. that used by the HMS tooling) to operate on an HMS instance that is not configured with the Kudu-HMS plugin et al. This patch removes the requirement by plumbing the option to the HMS client as a member of ThriftOptions. This was the most straightforward way to plumb this option from the HmsCatalog to the HmsClient, given the templating layered in between them for HA. Besides, we can use this option in the future if we ever want to verify the configuration of Thrift-based clients for other services (e.g. Sentry). This patch additionally allows the -hive_metastore_sasl_enabled flag to be used without the -keytab_file flag if not running kudu-master. To get this behavior, I've moved the gflag validator into master_main.cc, which is not built by tooling. I manually tested that it works, i.e. that tooling will not validate and that a master will. To test, I added an HmsMode that starts the HMS without the Kudu-HMS plugin installed and used it in a couple of HMS tooling tests. I considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to since some tests are greatly simplified by ENABLE_HIVE_METASTORE having the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to enable the Kudu-HMS integration). Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 --- M src/kudu/common/common.proto M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/master/master_main.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/server/server_base.cc M src/kudu/thrift/client.h M src/kudu/tools/kudu-tool-test.cc 15 files changed, 121 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/6 -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] ksck remote-test: make checksum tests more robust
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13529 ) Change subject: ksck_remote-test: make checksum tests more robust .. ksck_remote-test: make checksum tests more robust They aren't joining the writing thread in the event of a test failure. I tried to clean up the synchronization with the writer thread but found it to be too difficult: we really do need to wait until some writes have happened (i.e. if safe time is not initialized, we fail abruptly), but we can also make the tests more robust by increasing scanner_max_wait_ms. Change-Id: I1822e5b31656acf4dfbe1991cd59fa58bb574c81 Reviewed-on: http://gerrit.cloudera.org:8080/13529 Reviewed-by: Will Berkeley Tested-by: Will Berkeley --- M src/kudu/tools/ksck_remote-test.cc 1 file changed, 66 insertions(+), 31 deletions(-) Approvals: Will Berkeley: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/13529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I1822e5b31656acf4dfbe1991cd59fa58bb574c81 Gerrit-Change-Number: 13529 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 21:56:07 + Gerrit-HasComments: No
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208 PS3, Line 208: > Seems I misunderstood this at first. Seems to work! Great! Thank you for addressing this. -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 21:55:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2809 (1/6): temporarily disable TestKuduScanner.testDiffScan
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13533 ) Change subject: KUDU-2809 (1/6): temporarily disable TestKuduScanner.testDiffScan .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I981aaa47a532dd0c1271008e27de052e3c5ad1a6 Gerrit-Change-Number: 13533 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 21:55:10 + Gerrit-HasComments: No
[kudu-CR] ksck remote-test: make checksum tests more robust
Will Berkeley has removed a vote on this change. Change subject: ksck_remote-test: make checksum tests more robust .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I1822e5b31656acf4dfbe1991cd59fa58bb574c81 Gerrit-Change-Number: 13529 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] ksck remote-test: make checksum tests more robust
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13529 ) Change subject: ksck_remote-test: make checksum tests more robust .. Patch Set 1: Verified+1 Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1822e5b31656acf4dfbe1991cd59fa58bb574c81 Gerrit-Change-Number: 13529 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 21:54:32 + Gerrit-HasComments: No
[kudu-CR] KUDU-2830: [java] Improve RPC traces on RPC timeout
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13509 ) Change subject: KUDU-2830: [java] Improve RPC traces on RPC timeout .. KUDU-2830: [java] Improve RPC traces on RPC timeout Currently when RPCs timeout a full trace of all of the sent RPCs is printed. Given many retries can occur within the timeout period, this trace is often very large and difficult to read and and understand. This patch introduces a new trace summary format for the output that prints the most relevant information in a compact readable way. Along with readability, this has the benefit of reducing log churn. The trace sumary format will be used when the log level is INFO or higher and the original full trace will be used when the log level is DEBUG or lower. The new format style is: Trace Summary(trace-duration ms): Sent(n), Received(n), Delayed(n), MasterRefresh(n), AuthWait(n), Truncated: ? Sent: (server-uuid, [ rpc-method, count ], ...), ... Received: (server-uuid, [ rpc-status, count ], ...), ... Delayed: (server-uuid, [ rpc-method, count ], ...), … Change-Id: I725e3e8f89f5a433a0ffcb4b0facf82d9ffc4840 Reviewed-on: http://gerrit.cloudera.org:8080/13509 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java M java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceFrame.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcTraces.java 3 files changed, 204 insertions(+), 6 deletions(-) Approvals: Kudu Jenkins: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13509 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I725e3e8f89f5a433a0ffcb4b0facf82d9ffc4840 Gerrit-Change-Number: 13509 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2830: [java] Improve RPC traces on RPC timeout
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13509 ) Change subject: KUDU-2830: [java] Improve RPC traces on RPC timeout .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13509 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I725e3e8f89f5a433a0ffcb4b0facf82d9ffc4840 Gerrit-Change-Number: 13509 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 21:49:33 + Gerrit-HasComments: No
[kudu-CR] KUDU-2809 (6/6): reenable and update TestKuduScanner.testDiffScan
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13538 ) Change subject: KUDU-2809 (6/6): reenable and update TestKuduScanner.testDiffScan .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13538 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia466853736bfd769da4c51d6387bcd903df590ed Gerrit-Change-Number: 13538 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 21:48:39 + Gerrit-HasComments: No
[kudu-CR] KUDU-2809 (3/6): C++ private API for diff scans
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/13535 )
Change subject: KUDU-2809 (3/6): C++ private API for diff scans
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1337
PS2, Line 1337: Status KuduScanner::SetDiffScan(uint64_t start_timestamp,
uint64_t end_timestamp) {
Should we verify start is before end?
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/scan_batch.h@189
PS2, Line 189: Status GetIsDeleted(const Slice& col_name, bool* val) const
WARN_UNUSED_RESULT KUDU_NO_EXPORT;
The user won't know the name/index here right? Can this be handled like
RowResult.java where it's found via the schema? and the method is just `Status
GetIsDeleted(bool* val)`?
--
To view, visit http://gerrit.cloudera.org:8080/13535
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I03bd5e51c553b239e36fbdfc4a94131c9599851e
Gerrit-Change-Number: 13535
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Thu, 06 Jun 2019 21:48:17 +
Gerrit-HasComments: Yes
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208 PS3, Line 208: > OK, it's up to you -- I don't feel strong about that. However, if it's not Seems I misunderstood this at first. Seems to work! Done -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 21:36:16 + Gerrit-HasComments: Yes
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 21:36:07 + Gerrit-HasComments: No
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13510 to look at the new patch set (#5). Change subject: hms: allow for tooling to run without Kudu plugin .. hms: allow for tooling to run without Kudu plugin Currently, whenever an HMS client successfully connects to the HMS, it checks the various HMS service configurations (e.g. that it is using the Kudu-HMS plugin), returning an error if any are misconfigured. This is important to make it much more obvious when the Kudu Master's HMS synchronization is misconfigured. It is still useful for other HMS clients (e.g. that used by the HMS tooling) to operate on an HMS instance that is not configured with the Kudu-HMS plugin et al. This patch removes the requirement by plumbing the option to the HMS client as a member of ThriftOptions. This was the most straightforward way to plumb this option from the HmsCatalog to the HmsClient, given the templating layered in between them for HA. Besides, we can use this option in the future if we ever want to verify the configuration of Thrift-based clients for other services (e.g. Sentry). This patch additionally allows the -hive_metastore_sasl_enabled flag to be used without the -keytab_file flag if not running kudu-master. To get this behavior, I've moved the gflag validator into master_main.cc, which is not built by tooling. I manually tested that it works, i.e. that tooling will not validate and that a master will. To test, I added an HmsMode that starts the HMS without the Kudu-HMS plugin installed and used it in a couple of HMS tooling tests. I considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to since some tests are greatly simplified by ENABLE_HIVE_METASTORE having the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to enable the Kudu-HMS integration). Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 --- M src/kudu/common/common.proto M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/master/master_main.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/thrift/client.h M src/kudu/tools/kudu-tool-test.cc 14 files changed, 123 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/5 -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123 PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with > I'm not sure this will work -- as I understand, master.cc is compiled as a Ah, yeah, you're right. You'd have to compile master.cc twice for this to work. OK, I get the motivation for a gflag now. -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 21:29:49 + Gerrit-HasComments: Yes
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208 PS3, Line 208: is_server > I'm leaving this as is unless you feel strongly about it. OK, it's up to you -- I don't feel strong about that. However, if it's not a big deal just to move that piece of code into master_main.cc, we could avoid adding this artificial hidden 'is_server' flag. -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 21:06:42 + Gerrit-HasComments: Yes
[kudu-CR] fuzz-itest: assorted cleanup
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13532 ) Change subject: fuzz-itest: assorted cleanup .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13532 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1792f4f2d83403c9ae57acb29190246d5f036e69 Gerrit-Change-Number: 13532 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 20:27:42 + Gerrit-HasComments: No
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG@27 PS2, Line 27: server. > nit: tserver? Done http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG@31 PS2, Line 31: > nit: a Reading this out loud, this reads more naturally. http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto@81 PS2, Line 81: > I think this comment is a bit misleading. If the only difference is to enab Done http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto@85 PS2, Line 85: > Same here. Done http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208 PS3, Line 208: is_server > This looks good and generic, but as a second thought, I'm curious whether a I'm leaving this as is unless you feel strongly about it. -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 20:24:33 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: deflake TestClientConnectionMetrics
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13543 ) Change subject: rpc-test: deflake TestClientConnectionMetrics .. Patch Set 1: Verified+1 Overriding Jenkins, unrelated test flake. -- To view, visit http://gerrit.cloudera.org:8080/13543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I705711a53a6b588c4c164990506c530c8bd6116f Gerrit-Change-Number: 13543 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 20:23:01 + Gerrit-HasComments: No
[kudu-CR] rpc-test: deflake TestClientConnectionMetrics
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13543 ) Change subject: rpc-test: deflake TestClientConnectionMetrics .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I705711a53a6b588c4c164990506c530c8bd6116f Gerrit-Change-Number: 13543 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13510 to look at the new patch set (#4). Change subject: hms: allow for tooling to run without Kudu plugin .. hms: allow for tooling to run without Kudu plugin Currently, whenever an HMS client successfully connects to the HMS, it checks the various HMS service configurations (e.g. that it is using the Kudu-HMS plugin), returning an error if any are misconfigured. This is important to make it much more obvious when the Kudu Master's HMS synchronization is misconfigured. It is still useful for other HMS clients (e.g. that used by the HMS tooling) to operate on an HMS instance that is not configured with the Kudu-HMS plugin et al. This patch removes the requirement by plumbing the option to the HMS client as a member of ThriftOptions. This was the most straightforward way to plumb this option from the HmsCatalog to the HmsClient, given the templating layered in between them for HA. Besides, we can use this option in the future if we ever want to verify the configuration of Thrift-based clients for other services (e.g. Sentry). This patch additionally allows the -hive_metastore_sasl_enabled flag to be used without the -keytab flag if not using a server. Tool users should just be able to kinit to authenticate. To get this behavior, I've gated gflag validator on a new hidden gflag and manually tested that it works (i.e. that tooling will not validate and that a server will). To test, I added an HmsMode that starts the HMS without the Kudu-HMS plugin installed and used it in a couple of HMS tooling tests. I considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to since some tests are greatly simplified by ENABLE_HIVE_METASTORE having the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to enable the Kudu-HMS integration). Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 --- M src/kudu/common/common.proto M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/server/server_base.cc M src/kudu/thrift/client.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_main.cc 15 files changed, 109 insertions(+), 45 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/4 -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] rpc-test: deflake TestClientConnectionMetrics
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13543 ) Change subject: rpc-test: deflake TestClientConnectionMetrics .. rpc-test: deflake TestClientConnectionMetrics Looks like send_bytes_per_sec can overflow: /home/jenkins-slave/workspace/kudu-master/2/src/kudu/rpc/rpc-test.cc:552 Expected: (conn.socket_stats().send_bytes_per_sec()) > (0), actual: -800244607 vs 0 The code in 'ss' (which inspired this calculation) uses double arithmetic, but we'll settle for int64, which should be good enough. Change-Id: I705711a53a6b588c4c164990506c530c8bd6116f Reviewed-on: http://gerrit.cloudera.org:8080/13543 Reviewed-by: Todd Lipcon Tested-by: Adar Dembo --- M src/kudu/rpc/rpc_introspection.proto 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Todd Lipcon: Looks good to me, approved Adar Dembo: Verified -- To view, visit http://gerrit.cloudera.org:8080/13543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I705711a53a6b588c4c164990506c530c8bd6116f Gerrit-Change-Number: 13543 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2785: Add splitSizeBytes to the backup job
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13511 ) Change subject: KUDU-2785: Add splitSizeBytes to the backup job .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/13511/4/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/13511/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@346 PS4, Line 346: // Wait for mrs flushed. : Thread.sleep(5 * 1000) > Why? This matches the pattern for all of the other split size unit tests. It makes sure there is no flakiness due to slow flushing. http://gerrit.cloudera.org:8080/#/c/13511/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@356 PS4, Line 356: task > nit: tasks Done -- To view, visit http://gerrit.cloudera.org:8080/13511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6b8d02b71b1f463e4d0d9e04203d8edbd5e016b Gerrit-Change-Number: 13511 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 20:20:37 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2785: Add splitSizeBytes to the backup job
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13511 to look at the new patch set (#5). Change subject: KUDU-2785: Add splitSizeBytes to the backup job .. KUDU-2785: Add splitSizeBytes to the backup job This patch adds an experimental and hidden option to use the new scanner splitSizeBytes feature in the backup job. Change-Id: If6b8d02b71b1f463e4d0d9e04203d8edbd5e016b --- M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala 3 files changed, 50 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/13511/5 -- To view, visit http://gerrit.cloudera.org:8080/13511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If6b8d02b71b1f463e4d0d9e04203d8edbd5e016b Gerrit-Change-Number: 13511 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2785: Add splitSizeBytes to the backup job
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13511 ) Change subject: KUDU-2785: Add splitSizeBytes to the backup job .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/13511/4/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/13511/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@346 PS4, Line 346: // Wait for mrs flushed. : Thread.sleep(5 * 1000) Why? http://gerrit.cloudera.org:8080/#/c/13511/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@356 PS4, Line 356: task nit: tasks -- To view, visit http://gerrit.cloudera.org:8080/13511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6b8d02b71b1f463e4d0d9e04203d8edbd5e016b Gerrit-Change-Number: 13511 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 20:16:11 + Gerrit-HasComments: Yes
[kudu-CR] [hms] Adjust storage handler package
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13540 ) Change subject: [hms] Adjust storage handler package .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/13540/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13540/1//COMMIT_MSG@7 PS1, Line 7: [hms] Adjust storage handler package > nit: maybe add the Kudu jira KUDU-442 to the commit msg? It doesn't really enable KUDU-442 even though it's forward looking for it. I would rather just leave it as is if that's okay. -- To view, visit http://gerrit.cloudera.org:8080/13540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I66b314c1c8b56785005d6e0d8b679e19219494fe Gerrit-Change-Number: 13540 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 19:55:06 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: deflake TestClientConnectionMetrics
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13543 ) Change subject: rpc-test: deflake TestClientConnectionMetrics .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I705711a53a6b588c4c164990506c530c8bd6116f Gerrit-Change-Number: 13543 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 19:52:28 + Gerrit-HasComments: No
[kudu-CR] [rebalance] Add '--known unhealthy tservers' flag to rebalancer
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13539 ) Change subject: [rebalance] Add '--known_unhealthy_tservers' flag to rebalancer .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/13539/1//COMMIT_MSG Commit Message: PS1: Curious why you went with this approach vs. a boolean flag that just allowed rebalancing to continue regardless of which tservers were unhealthy. What's the appeal in having to manually specify the UUIDs to get this behavior? http://gerrit.cloudera.org:8080/#/c/13539/1//COMMIT_MSG@10 PS1, Line 10: tervers tservers -- To view, visit http://gerrit.cloudera.org:8080/13539 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie83a7d2497b778833f3f96fa8aa20e7486c5ebbb Gerrit-Change-Number: 13539 Gerrit-PatchSet: 1 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 06 Jun 2019 19:12:41 + Gerrit-HasComments: Yes
[kudu-CR] [rebalance] Add '--known unhealthy tservers' flag to rebalancer
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13539 )
Change subject: [rebalance] Add '--known_unhealthy_tservers' flag to rebalancer
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13539/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13539/1//COMMIT_MSG@7
PS1, Line 7: known_unhealthy_tservers
> Maybe, name it '--tservers_to_ignore'. Essentially, it doesn't matter wha
Agreed with Alexey (my vote is for --ignored_tservers). Also, As please refer
to them as such throughout the patch, not just in the gflag name.
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h
File src/kudu/tools/rebalancer.h:
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h@68
PS1, Line 68: Config(std::vector known_unhealthy_tservers = {},
> warning: constructors that are callable with a single argument must be mark
If the goal is to allow implicit conversions here, you can silence the warning
with NOLINT or NOLINTNEXTLINE. See
https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics for
details.
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h@83
PS1, Line 83: // If not empty, allow to run the rebalancing when servers in
Trailing whitespace here.
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.cc
File src/kudu/tools/rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.cc@1231
PS1, Line 1231: auto it = std::find(known_unhealthy_tservers_.begin(),
: known_unhealthy_tservers_.end(),
: s.uuid);
: if (it != known_unhealthy_tservers_.end()) {
: continue;
: }
Agreed with Alexey on using std::unordered_set to speed up this lookup. Use
ContainsKey from gutil/map-util.h here and below.
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.cc@1445
PS1, Line 1445: rebalancer,std::move(known_unhealthy_tservers),
max_moves_per_server, std::move(deadline)),
Some of the lines here are too long; could you rewrap?
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer_tool-test.cc
File src/kudu/tools/rebalancer_tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer_tool-test.cc@221
PS1, Line 221: specifiing
specifying
--
To view, visit http://gerrit.cloudera.org:8080/13539
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie83a7d2497b778833f3f96fa8aa20e7486c5ebbb
Gerrit-Change-Number: 13539
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 06 Jun 2019 19:10:31 +
Gerrit-HasComments: Yes
[kudu-CR] [hms] Adjust storage handler package
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13540 ) Change subject: [hms] Adjust storage handler package .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/13540/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13540/1//COMMIT_MSG@7 PS1, Line 7: [hms] Adjust storage handler package nit: maybe add the Kudu jira KUDU-442 to the commit msg? -- To view, visit http://gerrit.cloudera.org:8080/13540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I66b314c1c8b56785005d6e0d8b679e19219494fe Gerrit-Change-Number: 13540 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 19:07:50 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: deflake TestClientConnectionMetrics
Hello Will Berkeley, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/13543 to review the following change. Change subject: rpc-test: deflake TestClientConnectionMetrics .. rpc-test: deflake TestClientConnectionMetrics Looks like send_bytes_per_sec can overflow: /home/jenkins-slave/workspace/kudu-master/2/src/kudu/rpc/rpc-test.cc:552 Expected: (conn.socket_stats().send_bytes_per_sec()) > (0), actual: -800244607 vs 0 The code in 'ss' (which inspired this calculation) uses double arithmetic, but we'll settle for int64, which should be good enough. Change-Id: I705711a53a6b588c4c164990506c530c8bd6116f --- M src/kudu/rpc/rpc_introspection.proto 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/13543/1 -- To view, visit http://gerrit.cloudera.org:8080/13543 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I705711a53a6b588c4c164990506c530c8bd6116f Gerrit-Change-Number: 13543 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalance] Add '--known unhealthy tservers' flag to rebalancer
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13539 ) Change subject: [rebalance] Add '--known_unhealthy_tservers' flag to rebalancer .. Patch Set 1: (2 comments) Thank you for your contribution! I took a quick look at the high level. http://gerrit.cloudera.org:8080/#/c/13539/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13539/1//COMMIT_MSG@7 PS1, Line 7: known_unhealthy_tservers Maybe, name it '--tservers_to_ignore'. Essentially, it doesn't matter what is the status of tablet servers in that list, right? The idea is just to remove replicas and the tablet servers themselves from the picture, as I understand. http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h File src/kudu/tools/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/13539/1/src/kudu/tools/rebalancer.h@239 PS1, Line 239: std::vector Given the pattern of usage of this container, it might be better if it were a fast-lookup dictionary like std::unordered_set. -- To view, visit http://gerrit.cloudera.org:8080/13539 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie83a7d2497b778833f3f96fa8aa20e7486c5ebbb Gerrit-Change-Number: 13539 Gerrit-PatchSet: 1 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 06 Jun 2019 18:55:53 + Gerrit-HasComments: Yes
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123 PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with > Hmm, why is a separate source file necessary? Wouldn't this work? I'm not sure this will work -- as I understand, master.cc is compiled as a part of the master library, so the compiler is run only once for this source file, right? At least, I can see from compile_commands.json that the compiler is invoked only once for this master.cc file. Maybe be I'm missing something. -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 18:47:15 + Gerrit-HasComments: Yes
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/13510 )
Change subject: hms: allow for tooling to run without Kudu plugin
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools
that run with
> Done
Hmm, why is a separate source file necessary? Wouldn't this work?
bool ValidateHiveMetastoreSaslEnabled() {
#ifndef KUDU_IN_CLI
if (FLAGS_hive_metastore_sasl_enabled &&
FLAGS_keytab_file.empty()) {
return false;
}
#endif
return true;
}
Then in tools/CMakeLists.txt:
add_compile_definitions(KUDU_IN_CLI);
--
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 18:43:12 +
Gerrit-HasComments: Yes
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208 PS3, Line 208: is_server This looks good and generic, but as a second thought, I'm curious whether a targeted approach will be a better case here. I.e., what if we set/register the group validator only for the master's code? Something like moving GROUP_FLAG_VALIDATOR(hive_metastore_sasl_enabled, ValidateHiveMetastoreSaslEnabled); into master_main.cc or moving the whole group validator code there? -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 18:28:25 + Gerrit-HasComments: Yes
[kudu-CR] [hms] Adjust storage handler package
Grant Henke has removed a vote on this change. Change subject: [hms] Adjust storage handler package .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I66b314c1c8b56785005d6e0d8b679e19219494fe Gerrit-Change-Number: 13540 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [hms] Adjust storage handler package
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13540 ) Change subject: [hms] Adjust storage handler package .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I66b314c1c8b56785005d6e0d8b679e19219494fe Gerrit-Change-Number: 13540 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 18:08:51 + Gerrit-HasComments: No
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123 PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with > After discussing it offline with Andrew, it seems the idea with a higgen gf Done -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 18:00:52 + Gerrit-HasComments: Yes
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13510 to look at the new patch set (#3). Change subject: hms: allow for tooling to run without Kudu plugin .. hms: allow for tooling to run without Kudu plugin Currently, whenever an HMS client successfully connects to the HMS, it checks the various HMS service configurations (e.g. that it is using the Kudu-HMS plugin), returning an error if any are misconfigured. This is important to make it much more obvious when the Kudu Master's HMS synchronization is misconfigured. It is still useful for other HMS clients (e.g. that used by the HMS tooling) to operate on an HMS instance that is not configured with the Kudu-HMS plugin et al. This patch removes the requirement by plumbing the option to the HMS client as a member of ThriftOptions. This was the most straightforward way to plumb this option from the HmsCatalog to the HmsClient, given the templating layered in between them for HA. Besides, we can use this option in the future if we ever want to verify the configuration of Thrift-based clients for other services (e.g. Sentry). This patch additionally allows the -hive_metastore_sasl_enabled flag to be used without the -keytab flag if not using a kserver. Tool users should just be able to kinit to authenticate. To get this behavior, I've gated gflag validator on a new hidden gflag. To test, I added an HmsMode that starts the HMS without the Kudu-HMS plugin installed and used it in a couple of HMS tooling tests. I considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to since some tests are greatly simplified by ENABLE_HIVE_METASTORE having the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to enable the Kudu-HMS integration). Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 --- M src/kudu/common/common.proto M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/server/server_base.cc M src/kudu/thrift/client.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_main.cc 15 files changed, 109 insertions(+), 45 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/3 -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] hms: allow for tooling to run without Kudu plugin
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 ) Change subject: hms: allow for tooling to run without Kudu plugin .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123 PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with > +1 for the CLI-specific macro and #define that uses it. After discussing it offline with Andrew, it seems the idea with a higgen gflag is a better fit. In case of 'macro approach' it would be necessary to create a separate source .cc file that we compile with different compiler flags for the 'kudu' and the 'master' binaries, linking the result object files into corresponding binaries. -- To view, visit http://gerrit.cloudera.org:8080/13510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0 Gerrit-Change-Number: 13510 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 06 Jun 2019 17:44:11 + Gerrit-HasComments: Yes
[kudu-CR] [java] Add newComparisonPredicate Object API
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13527 ) Change subject: [java] Add newComparisonPredicate Object API .. [java] Add newComparisonPredicate Object API Adds a `newComparisonPredicate` API that takes an Object in order to support creating comparison predicates for Java objects generically. Change-Id: If05b2d9f61990b86d92af4b89e21704d5188192e Reviewed-on: http://gerrit.cloudera.org:8080/13527 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala 3 files changed, 89 insertions(+), 27 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If05b2d9f61990b86d92af4b89e21704d5188192e Gerrit-Change-Number: 13527 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] [java] Add PartialRow addObject API
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13528 ) Change subject: [java] Add PartialRow addObject API .. [java] Add PartialRow addObject API Adds a new API to add an Object to a PartialRow. This method is useful when you don't care about autoboxing and your existing type handling logic is based on Java types. Change-Id: I046de9dba8f4bedae02b82632e26c10c313c775c Reviewed-on: http://gerrit.cloudera.org:8080/13528 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java 3 files changed, 163 insertions(+), 23 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13528 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I046de9dba8f4bedae02b82632e26c10c313c775c Gerrit-Change-Number: 13528 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2809 (5/6): add diff scan support to fuzz-itest
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13537 ) Change subject: KUDU-2809 (5/6): add diff scan support to fuzz-itest .. Patch Set 3: Verified+1 Overriding Jenkins, unrelated AlterTableRandomized flake. -- To view, visit http://gerrit.cloudera.org:8080/13537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f7dae20ef1b903dba80e90d5f491e4322815fbb Gerrit-Change-Number: 13537 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 06 Jun 2019 17:30:57 + Gerrit-HasComments: No
[kudu-CR] KUDU-2809 (5/6): add diff scan support to fuzz-itest
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13537 ) Change subject: KUDU-2809 (5/6): add diff scan support to fuzz-itest .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/13537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I3f7dae20ef1b903dba80e90d5f491e4322815fbb Gerrit-Change-Number: 13537 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
