[kudu-CR] [Java] Add a Schema and Data Generator
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12061 )
Change subject: [Java] Add a Schema and Data Generator
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12061/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12061/1//COMMIT_MSG@9
PS1, Line 9: This patch adds a schema and data generator utility
: class
Nit: This patch adds schema and data generate utility classes
http://gerrit.cloudera.org:8080/#/c/12061/1//COMMIT_MSG@11
PS1, Line 11: usefull
useful
http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java
File java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java:
PS1:
License header.
http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java@58
PS1, Line 58: public void randomizeRow(PartialRow row) {
Seems redundant to have both randomizeRow and randomRow. The TestKuduBackup
case shows a need for randomizeRow; can we omit randomRow?
If not, could you at least rename one? The two names are quite similar.
http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java@97
PS1, Line 97: i++;
It's easy to miss this; perhaps convert into a for loop on i (capped at
columns.size())?
http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java@129
PS1, Line 129: public static class DataGeneratorBuilder {
Since the only way to create a DataGenerator is through here, maybe doc this a
bit?
http://gerrit.cloudera.org:8080/#/c/12061/1/java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java
File java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java:
PS1:
License header.
--
To view, visit http://gerrit.cloudera.org:8080/12061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56
Gerrit-Change-Number: 12061
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 11 Dec 2018 06:54:53 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] Integrate AuthzProvider into CatalogManager
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager
..
Patch Set 3:
(33 comments)
http://gerrit.cloudera.org:8080/#/c/11797/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11797/3//COMMIT_MSG@12
PS3, Line 12: as
such as
http://gerrit.cloudera.org:8080/#/c/11797/3//COMMIT_MSG@17
PS3, Line 17: More authorization endpoints, e.g ListTables
: RPC, will be addressed in a follow up patch.
Haven't reviewed this patch yet, but it's not obvious from the commit message
why this patch covers the set of endpoints that it does and not the others. My
guess is that you wanted to cover the "write" endpoints first and will do the
read-only ones later. If that's right, please update the commit message to
clarify.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/hms_itest-base.h
File src/kudu/integration-tests/hms_itest-base.h:
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/hms_itest-base.h@150
PS3, Line 150: boost::optional user) {
> warning: the parameter 'user' is copied for each invocation but only used a
In places where you pass none, use /*foo=*/ comments to indicate what it is.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc@147
PS3, Line 147: std::unique_ptr sentry_client_;
Nit: drop std:: prefix.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc@152
PS3, Line 152: #if defined(__linux__)
: { ExternalMiniCluster::BindMode::UNIQUE_LOOPBACK, },
: #elif defined(__APPLE__)
: // On macOS, it's not possible to have unique loopback
interfaces.
: // Thus, binds to loopback ip address "127.0.0.1".
: { ExternalMiniCluster::BindMode::LOOPBACK, },
: #endif
I don't understand why this is here; why can't you just kDefaultBindMode?
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc@169
PS3, Line 169: ASSERT_TRUE(s.IsNotAuthorized());
Nit: indentation
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc@183
PS3, Line 183: ASSERT_EVENTUALLY([&] {
: ASSERT_OK(client_->DeleteTable(Substitute("$0.$1",
kDatabaseName, "a")));
: });
Why does this need to be wrapped in ASSERT_EVENTUALLY?
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc@243
PS3, Line 243: // ASSERT_EVENTUALLY([&] {
: // ASSERT_OK(table_alterer->Alter());
: // });
Why will this need to be wrapped?
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/authz_provider.h@44
PS3, Line 44: virtual Status AuthorizeCreateTable(const std::string& user,
Curious what motivated the reordering changes here?
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/authz_provider.cc
File src/kudu/master/authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/authz_provider.cc@32
PS3, Line 32: DEFINE_string(trusted_user_acl, "impala",
Hmm, why don't we default to an empty set and let users who integrate with
Impala override this? Seems safer than this default.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/authz_provider.cc@51
PS3, Line 51: vector acls = strings::Split(FLAGS_trusted_user_acl,
",", strings::SkipEmpty());
Surprised you don't need a ScopedLeakCheckDisabler for g_trusted_users. Search
around in the codebase; maybe you'll understand better than I when it's needed
and when it isn't.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/authz_provider.cc@52
PS3, Line 52: g_trusted_users = new unordered_set(acls.begin(),
acls.end());
Nit: indentation.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.h@538
PS3, Line 538: getting
Nit: retrieving
For this and for the other functions for which 'rpc' is optional, please update
the various call-sites where you've passed nullptr with /*rpc=*/ comments.
Below too.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.h@722
PS3, Line 722: // Delete the specified table in the catalog.
Doc the effect that 'user' has. Below too.
http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.h@842
PS3, Line 842: // Looks up the table, locks it with the pro
[kudu-CR] KUDU-1375: [docs] Remove "binaries" in the Build from Source section
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12060 ) Change subject: KUDU-1375: [docs] Remove "binaries" in the Build from Source section .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12060/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12060/3//COMMIT_MSG@9 PS3, Line 9: make Nit: 'install' would be more appropriate here. -- To view, visit http://gerrit.cloudera.org:8080/12060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 Gerrit-Change-Number: 12060 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 Dec 2018 05:49:38 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: } > Unfortunately, this is not an option. As surprisedly, HMS always does wildc Search for THRIFT_BIND_HOST in HIVE-20794. Does that address HIVE-18998? It was merged with the fix version 4.0.0. Is that an HMS artifact we can use for our own testing? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/sentry/mini_sentry.h File src/kudu/sentry/mini_sentry.h: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/sentry/mini_sentry.h@93 PS2, Line 93: > Yeah, in class member initialization is allowed in C++11, https://isocpp.or Ah, I thought you had to use value initialization (i.e. surround the value with braces). Didn't realize copy initialization works too. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto@65 PS8, Line 65: // Whether or not the Sentry service should be set up. This isn't sufficient; you also have to update tool_action_test.cc (I think) to configure the ExternalMiniCluster the right way if this is set. And a new test in kudu-tool-test would be nice too. -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 11 Dec 2018 05:42:48 + Gerrit-HasComments: Yes
[kudu-CR] De-flake sentry tests
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11961 ) Change subject: De-flake sentry tests .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/mini-cluster/external_mini_cluster.cc@525 PS3, Line 525: index, opts_.bind_mode); Nit: indentation. http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/sentry/sentry-test-base.h File src/kudu/sentry/sentry-test-base.h: http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/sentry/sentry-test-base.h@44 PS3, Line 44: #if defined(__linux__) : host = GetBindIpForDaemon(1, BindMode::UNIQUE_LOOPBACK); : #elif defined(__APPLE__) : host = GetBindIpForDaemon(1, BindMode::LOOPBACK); : #endif Shouldn't this use kDefaultBindMode? http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/11961/3/src/kudu/util/test_util.h@165 PS3, Line 165: // BindMode lets you specify the socket binding mode for RPC and/or HTTP server. Given that ExternalMiniCluster isn't test code anymore, I don't think this stuff belongs in test_util (since EMC depends on it). Maybe util/net/net_util.h? -- To view, visit http://gerrit.cloudera.org:8080/11961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b Gerrit-Change-Number: 11961 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 Dec 2018 05:47:58 + Gerrit-HasComments: Yes
[kudu-CR] De-flake sentry tests
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11961 to look at the new patch set (#3). Change subject: De-flake sentry tests .. De-flake sentry tests Currently, sentry_client-test and sentry_authz_provider-test are still flaky due to possible port collision. To avoid such race condition, this patch moves the util function that finds the bind address for a daemon based on binding mode to test_util, and updates sentry-test-base to use it to pick up a unique bind address. Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b --- M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/sentry/sentry-test-base.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 13 files changed, 163 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/11961/3 -- To view, visit http://gerrit.cloudera.org:8080/11961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b Gerrit-Change-Number: 11961 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 8:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322
PS2, Line 322: which synchronizes the notification logs with the Sentry
: // service. So that Sentry service is
> Yeah.
Ah, I think my confusion is in separating out the rules for the HMS vs the
rules for Kudu. Maybe it's worth stating explicitly that it's verifying that
changes to Kudu metadata entries in the HMS are performed by `kudu` (or
whatever user it actually is).
"authorization metadata operations" is a bit ambiguous IMO since it can refer
to authorization for DDL/DML that starts in Kudu and ends up in the HMS, and
those DDL/DML operations do need to be authorized, but are referring to a
different user.
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@79
PS8, Line 79: sentry_service_principal
nit: perhaps DCHECK this isn't empty?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@229
PS8, Line 229: - hive.sentry.conf.url
: // Configuration URL of the Sentry authorization plugin in
the HMS.
: //
nit: should be a part of kHiveSentryFileTemplate?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@310
PS8, Line 310: - hive.metastore.filter.hook
: // Configures the HMS to use the Sentry plugin for
filtering
: // out information user has no privileges to access for
operations
: // as SHOWTABLES and SHOWDATABASES.
Does this mean that we'll only see HMS notifications that the `kudu` user has
Sentry privileges for? Or is this specific to SHOWTABLES and SHOWDATABASES
(SHOW TABLES and SHOW DATABASES?) requests to the HMS?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@321
PS8, Line 321: Configures the HMS to use the Sentry post-event listener
: // which synchronizes the notification logs with the
Sentry
: // service. So that Sentry service is aware of events
such
: // as table rename or the table location update, and
accordingly
: // updates internally.
nit reword a bit:
"Configures the HMS to use the Sentry post-event listener, which synchronizes
the Sentry service with the HMS via Sentry's notification log listener. The
Sentry service will be made aware of events like table renames and update
itself accordingly."
Also, what is a "table location update"? It's not the location of a Kudu table,
is it?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@95
PS8, Line 95: Substitute("$0.$1", kDatabaseName, kTableName)
nit: kTableIdentifier
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc@231
PS8, Line 231: 3. Restart the Sentry service on the same port, reconfigured to
talk
: //to the HMS's port.
It's been a while since we talked about it, but is it not possible that another
process will claim this port? If so, why not? Mind putting the answer in a
comment either here or around GetBindIpForExternalServer?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc
File src/kudu/mini-cluster/mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc@64
PS8, Line 64: case EXTERNAL_SERVER:
: idx = kServersMaxNum / 3 + index;
Somewhat of a nit: this is giving the larger partition to the masters, when
really, it's the tablet servers that we would expect to dominate the idx-space
in a larger EMC deployment (if we wanted to test such large clusters).
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@114
PS8, Line 114: 127.0.0.1
Why doesn't this need to be the HMS URIs?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@264
PS8, Line 264: kudu,hive
Should this depend on IsHmsEnabled()?
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kud
[kudu-CR] KUDU-1375: [docs] Remove "binaries" in the Build from Source section
Alex Rodoni has posted comments on this change. ( http://gerrit.cloudera.org:8080/12060 ) Change subject: KUDU-1375: [docs] Remove "binaries" in the Build from Source section .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc File docs/installation.adoc: http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc@420 PS2, Line 420: NOTE: 'make install' does not copy Kudu binaries. You need to manually copy them > This text block should be replicated into the other _from_source sections ( Done http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc@424 PS2, Line 424: the your > Nit: drop one of these. Done http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc@426 PS2, Line 426: if you use the default installation directory > I would tie this more directly to DESTDIR from L413-414. Done -- To view, visit http://gerrit.cloudera.org:8080/12060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 Gerrit-Change-Number: 12060 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 Dec 2018 00:46:29 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1375: [docs] Remove "binaries" in the Build from Source section
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12060 to look at the new patch set (#3). Change subject: KUDU-1375: [docs] Remove "binaries" in the Build from Source section .. KUDU-1375: [docs] Remove "binaries" in the Build from Source section - make install does not make the Kudu binaries Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 --- M docs/installation.adoc 1 file changed, 33 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/12060/3 -- To view, visit http://gerrit.cloudera.org:8080/12060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 Gerrit-Change-Number: 12060 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] De-flake sentry tests
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11961 ) Change subject: De-flake sentry tests .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11961/1/src/kudu/util/test_util.h File src/kudu/util/test_util.h: http://gerrit.cloudera.org:8080/#/c/11961/1/src/kudu/util/test_util.h@51 PS1, Line 51: > Nit: reformat to bring this stuff closer to GetBindIpForDaemon, which is wh Done -- To view, visit http://gerrit.cloudera.org:8080/11961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b Gerrit-Change-Number: 11961 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 Dec 2018 23:56:21 + Gerrit-HasComments: Yes
[kudu-CR] De-flake sentry tests
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11961 to look at the new patch set (#2). Change subject: De-flake sentry tests .. De-flake sentry tests Currently, sentry_client-test and sentry_authz_provider-test are still flaky due to possible port collision. To avoid such race condition, this patch moves the util function that finds the bind address for a daemon based on binding mode to test_util, and updates sentry-test-base to use it to pick up a unique bind address. Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b --- M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/sentry/sentry-test-base.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 13 files changed, 162 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/11961/2 -- To view, visit http://gerrit.cloudera.org:8080/11961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id112634ad5e6f0e2c48b7e0e89389faff4f5656b Gerrit-Change-Number: 11961 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-1375: [docs] Remove "binaries" in the Build from Source section
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12060 ) Change subject: KUDU-1375: [docs] Remove "binaries" in the Build from Source section .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc File docs/installation.adoc: http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc@420 PS2, Line 420: NOTE: 'make install' does not copy Kudu binaries. You need to manually copy them This text block should be replicated into the other _from_source sections (except for osx, where we don't doc `make install` at all). http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc@424 PS2, Line 424: the your Nit: drop one of these. http://gerrit.cloudera.org:8080/#/c/12060/2/docs/installation.adoc@426 PS2, Line 426: if you use the default installation directory I would tie this more directly to DESTDIR from L413-414. -- To view, visit http://gerrit.cloudera.org:8080/12060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 Gerrit-Change-Number: 12060 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 Dec 2018 23:07:12 + Gerrit-HasComments: Yes
[kudu-CR] [docs] add Hive Metastore integration
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11798 ) Change subject: [docs] add Hive Metastore integration .. Patch Set 5: (24 comments) http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc File docs/hive_metastore.adoc: http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@38 PS5, Line 38: : ## Databases and Table Names Are there column name restrictions that are worth mentioning? http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@42 PS5, Line 42: nit: not sure if jekyll will get rid of this, but there's an extra space here http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@45 PS5, Line 45: tables must belong to a database nit: since there's still no such thing as a database in Kudu, maybe "the table name must indicate the table's membership of a Hive database"? Not sure how far we want to go in emphasizing that databases are a Hive thing and not a Kudu thing. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@46 PS5, Line 46: identifiers are nit: maybe call out, "...identifiers (i.e. the table name and database name) are..."? I know we use it in our codebase, but I'm not sure "table name identifier" is common parlance, and might be construed to just mean the table name and not the database name. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@59 PS5, Line 59: application which nit: "applications that" http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@58 PS5, Line 58: By including databases as an implicit part : of the Kudu table name, existing application which use Kudu tables can operate : on non-HMS-integrated and HMS-integrated table names with minimal or no changes. Maybe as a note, do you think it's worth calling out the previous convention with Impala? I.e. that old tables were commonly prefixed with "impala::" and that they should be renamed? I guess that sort of thing should come about after the Impala integration lands. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@61 PS5, Line 61: : In order to create or drop Hive databases, administrators or users must use : existing Hive tools such as the Beeline Shell or Impala. I was a little confused about why this sentence was here, so maybe reword it slightly: "Kudu provides no additional tooling to create or drop Hive databases. Administrators or users should use existing Hive tools such as the Beeline Shell or Impala to do so." Also is it worth mentioning the effects of dropping a Hive database that contains Kudu tables? http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@68 PS5, Line 68: table name identifier constraints nit: maybe just "naming constraints"? Maybe the section should be called "Naming Constraints" too? Not sure if the term "table name identifier" has caught on elsewhere. WDYT? http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@70 PS5, Line 70: underscore nit: underscores http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@73 PS5, Line 73: table name identifiers Is this referring to both table names and database names? The configuration name makes me think just table names, but I'm not sure. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@78 PS5, Line 78: Kudu will prevent tables from being created whose names only differ : by case, and table lookup when opening, altering, or dropping tables is : case-insensitive, for both the Hive database and table name portion of the Kudu : table name. nit reword a bit: "Kudu will prevent tables from being created when a table already exists in the Hive Metastore that only differs by case. Additionally, operations that open, alter, or drop tables are case-insensitive for both the database and table name portion of the Kudu table name when the integration is enabled." http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@83 PS5, Line 83: Enabling the Hive Metastore Integration : There should be a step here that renames existing Kudu tables. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@86 PS5, Line 86: On the Command Line But this isn't necessarily on the command line? I think this is alluding to the expectation that Kudu vendors will provide an automated process for this (e.g. going through Cloudera Manager), and this is the process to enable the integration manually. I think we should just omit this title until that is the case, at which point we can say "If using Cloudera Manager, don't do any of this stuff". http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@101 PS5, Line 101: : hive.metastore.disallow.incompatible.col.type.changes
[kudu-CR] KUDU-1375: [docs] Remove "binaries" in the Build from Source section
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12060 to look at the new patch set (#2). Change subject: KUDU-1375: [docs] Remove "binaries" in the Build from Source section .. KUDU-1375: [docs] Remove "binaries" in the Build from Source section - make install does not make the Kudu binaries Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 --- M docs/installation.adoc 1 file changed, 12 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/12060/2 -- To view, visit http://gerrit.cloudera.org:8080/12060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 Gerrit-Change-Number: 12060 Gerrit-PatchSet: 2 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [Java] Add a Schema and Data Generator
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12061 Change subject: [Java] Add a Schema and Data Generator .. [Java] Add a Schema and Data Generator This patch adds a schema and data generator utility class that can be used to create random tables and random data. These utilities are usefull in fuzz tests and for various load and scale test applications. The initial implementation is inteneded to be fairly flexble without being overengineered. Follow on patches will improve the API and options. The classes are currently marked private, but could be changed in the future. Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56 --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java A java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java A java/kudu-client/src/main/java/org/apache/kudu/util/SchemaGenerator.java 4 files changed, 572 insertions(+), 152 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/12061/1 -- To view, visit http://gerrit.cloudera.org:8080/12061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I750d2d346c3eeb7075b21c3fec0fd25236da4f56 Gerrit-Change-Number: 12061 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] KUDU-1375: [docs] Remove "binaries" in the Build from Source section
Alex Rodoni has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12060 Change subject: KUDU-1375: [docs] Remove "binaries" in the Build from Source section .. KUDU-1375: [docs] Remove "binaries" in the Build from Source section - make install does not make the Kudu binaries Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 --- M docs/installation.adoc 1 file changed, 3 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/12060/1 -- To view, visit http://gerrit.cloudera.org:8080/12060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I5b1e7337cdd9f04eea1c2b4da3d27b9815553121 Gerrit-Change-Number: 12060 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni
[kudu-CR](branch-1.7.x) kudu support ttl
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12057 ) Change subject: kudu support ttl .. Patch Set 1: Thank you for your contribution. I just skimmed this change and had a few questions: 1. Did you intend to publish it against branch-1.7.x and not master? New features should always land in master first, and only be backported to older branches if they're small in scope and well tested. 2. There's very little documentation explaining what the change does. The commit description is non-existent and there are few (if any?) new comments. It's hard to figure out what's going on here. For patches that approach a thousand lines of code we typically like to see, at the very least: 2a. A terse summary of the change. 2b. A detailed description of what has changed. 2c. A discussion of trade-offs, if there were alternatives to consider. 2d. If there are persistent state or wire protocol changes, an evaluation of impact on backwards compatibility (if any). 3. I also see a fair amount of debugging code (new LOG(INFO) messages, #if statements, etc.) in the patch. Did you intend to leave them in? If not, could you clean up the patch in preparation for code review? -- To view, visit http://gerrit.cloudera.org:8080/12057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.7.x Gerrit-MessageType: comment Gerrit-Change-Id: I6503ca90ebca66d154e7084e0c93e94c9b0d9c0c Gerrit-Change-Number: 12057 Gerrit-PatchSet: 1 Gerrit-Owner: YangSong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 Dec 2018 20:26:59 + Gerrit-HasComments: No
[kudu-CR] [build] Fix bulding codegen on MacOS Mojave
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12054 ) Change subject: [build] Fix bulding codegen on MacOS Mojave .. Patch Set 5: Code-Review+2 Assuming it works... -- To view, visit http://gerrit.cloudera.org:8080/12054 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I33f7510c7bf0cc2a7708191653b23c04e6df8a29 Gerrit-Change-Number: 12054 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 10 Dec 2018 20:19:02 + Gerrit-HasComments: No
