[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 3: > Overall looks good to me structurally, some nits and maybe add a > unit test for IsSentryPrivilegeWellFormed() if it makes sense. I think the latter (the unit test for IsSentryPrivilegeWellFormed()) might be added in a separate patfch. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 06:29:54 + Gerrit-HasComments: No
[kudu-CR] WIP [master] introduced SentryPrivilegesFetcher
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 ) Change subject: WIP [master] introduced SentryPrivilegesFetcher .. Patch Set 8: > Uploaded patch set 8. I'm thinking to significantly chahge this patch upon rebasing atop of https://gerrit.cloudera.org/#/c/12919/ -- To view, visit http://gerrit.cloudera.org:8080/12833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Gerrit-Change-Number: 12833 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin 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: Thu, 04 Apr 2019 06:28:59 + Gerrit-HasComments: No
[kudu-CR] WIP [master] introduced SentryPrivilegesFetcher
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12833 to look at the new patch set (#8). Change subject: WIP [master] introduced SentryPrivilegesFetcher .. WIP [master] introduced SentryPrivilegesFetcher This patch incorporates a TTL-based cache into the data paths of SentryAuthzProvider. As of now, the cache stores raw responses received from Sentry. It's possible to enable or disable caching upon creation of SentryAuthzProvider instance: set the newly introduced `--sentry_authz_cache_capacity_mb` command-line flag to 0 to disable caching of authz privilege information returned from Sentry. In addition, it's possible to force the cache to fetch and cache information from broader levels of Sentry's authz hierarchy: use the `--sentry_authz_cache_finest_scope` flag for that. WIP * clarify on --sentry_authz_cache_finest_scope: do we need it or we are going to use some other approach * proper sanitization of Sentry responses (comes from Andrew's patch?) * cache processed and sanitized info, not raw Sentry responses * to add tests specific to SentryPrivilegesFetcher Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe --- M src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.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 A src/kudu/master/sentry_privileges_cache_metrics.cc A src/kudu/master/sentry_privileges_cache_metrics.h A src/kudu/master/sentry_privileges_fetcher.cc A src/kudu/master/sentry_privileges_fetcher.h M src/kudu/sentry/sentry_authorizable_scope.cc 12 files changed, 1,121 insertions(+), 304 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/12833/8 -- To view, visit http://gerrit.cloudera.org:8080/12833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Gerrit-Change-Number: 12833 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin 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)
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Hello Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12890 to look at the new patch set (#8). Change subject: java/c++: ColumnSchema supports storing column comment .. java/c++: ColumnSchema supports storing column comment This patch intends to add a new api for storing column comment in java client. And according to the comments of the reviews, the empty comment is defined to delete the comment for the uniform behavior. In addition, the server-side has already been implemented in: https://gerrit.cloudera.org/#/c/12849/ Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c --- M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/client/client-test.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/wire_protocol.cc M src/kudu/master/catalog_manager.cc M src/kudu/server/webui_util.cc 14 files changed, 200 insertions(+), 67 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12890/8 -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 8 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] java/c++: ColumnSchema supports storing column comment
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 ) Change subject: java/c++: ColumnSchema supports storing column comment .. Patch Set 7: Yeah, i will rebase on master after this modification. :) -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 04 Apr 2019 03:10:57 + Gerrit-HasComments: No
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 ) Change subject: java/c++: ColumnSchema supports storing column comment .. Patch Set 7: Oh, I think you still need to rebase. -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 04 Apr 2019 03:08:48 + Gerrit-HasComments: No
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 ) Change subject: java/c++: ColumnSchema supports storing column comment .. Patch Set 7: Code-Review+1 LGTM but I will let Adar review too. -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Thu, 04 Apr 2019 03:08:36 + Gerrit-HasComments: No
[kudu-CR] java/c++: ColumnSchema supports storing column comment
helifu has posted comments on this change. (
http://gerrit.cloudera.org:8080/12890 )
Change subject: java/c++: ColumnSchema supports storing column comment
..
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12890/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
http://gerrit.cloudera.org:8080/#/c/12890/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@408
PS6, Line 408: throw new IllegalArgumentException("The comment should
not be empty");
> I don't think we need this check. A user can set an empty comment and it sh
Done
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.h@445
PS6, Line 445: /// The comment for the column. An empty comment means
deleting an
> I think you can keep the docs as simple as "The comment for the column."
Done
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.cc@314
PS6, Line 314: return Status::InvalidArgument("cannot delete comment during
CreateTable",
> See my other comments on ColumnSchema.java and schema.h.
Done
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/common/schema.h@347
PS6, Line 347: // "empty comment" means "no comment".
> nit: I don't think you need this doc line here.
Done
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/server/webui_util.cc@53
PS6, Line 53: col_json["comment"] = !col.comment().empty() ? col.comment()
: "-";
> I don't think we need the empty check here. In the case the comment is empt
Done
--
To view, visit http://gerrit.cloudera.org:8080/12890
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c
Gerrit-Change-Number: 12890
Gerrit-PatchSet: 6
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Thu, 04 Apr 2019 02:48:49 +
Gerrit-HasComments: Yes
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Hello Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12890 to look at the new patch set (#7). Change subject: java/c++: ColumnSchema supports storing column comment .. java/c++: ColumnSchema supports storing column comment This patch intends to add a new api for storing column comment in java client. And according to the comments of the reviews, the empty comment is defined to delete the comment for the uniform behavior. In addition, the server-side has already been implemented in: https://gerrit.cloudera.org/#/c/12849/ Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c --- M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/client/client-test.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/wire_protocol.cc M src/kudu/master/catalog_manager.cc M src/kudu/server/webui_util.cc 14 files changed, 197 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12890/7 -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 7 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@132
PS3, Line 132: Why just the server? This guarantees that a request for any
: // authorizable scope will ignore such invalid privileges.
Yes, I understand the reasoning here: the responses are from real Sentry
instance, and that's about end-to-end verification.
What do you think about adding sort of unit test for
SentryPrivilegeIsWellFormed() (currently file-scope-private of
sentry_authz_provider.cc) where the fields of TSentryPrivilege are being messed
up mercilessly?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@171
PS3, Line 171: :
style nit: it seems the indent should be four spaces; a space after the column
is needed
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@264
PS3, Line 264: static const vector kInvalidPrivileges = {
: InvalidPrivilege::INCORRECT_ACTION,
: InvalidPrivilege::INCORRECT_SCOPE,
: InvalidPrivilege::INCORRECT_SERVER,
: InvalidPrivilege::FLIPPED_FIELD,
: };
syntax sugar addict's nit: could it be just
static constexpr InvalidPrivilege kInvalidPrivileges[] = {
...
};
?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@87
PS3, Line 87: the privileges apply.
> I'm not sure I understand what 'apply' means in this context. So, maybe ch
Whoops, it should have been '... the user is granted privileges on ...'
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@325
PS3, Line 325: SentryPrivilegeIsWellFormed
Why not
IsSentryPrivilegeWellFormed(...) ?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@463
PS3, Line 463: continue;
Do you think it's worth logging a message in that case? Maybe LOG(INFO) or at
least VLOG(1) ?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h
File src/kudu/sentry/sentry_action.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h@53
PS3, Line 53:ALL,
Not from this particular changelist, but while you are at it: maybe, move ALL
closer to OWNER (e.g. insert it between 'DROP' and 'OWNER')?
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 02:18:11 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 3: Overall looks good to me structurally, some nits and maybe add a unit test for IsSentryPrivilegeWellFormed() if it makes sense. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 02:19:58 + Gerrit-HasComments: No
[kudu-CR] util: pull Random methods out from tests
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12918 )
Change subject: util: pull Random methods out from tests
..
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h
File src/kudu/util/random_util.h:
http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h@75
PS5, Line 75: Rand* r, std::vector* result) {
> Mind if I don't? I'd be fighting the temptation to do the same for both Ran
Fair enough.
--
To view, visit http://gerrit.cloudera.org:8080/12918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 04 Apr 2019 01:05:50 +
Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12918 )
Change subject: util: pull Random methods out from tests
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h
File src/kudu/util/random_util.h:
http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h@75
PS5, Line 75: Rand* r, std::vector* result) {
> Nit: could we put 'r' first in the list? Because this is "almost" a Random
Mind if I don't? I'd be fighting the temptation to do the same for both
RandomString()s.
GSG is a bit fuzzy here since this isn't quite input-only insofar as it is
mutated.
--
To view, visit http://gerrit.cloudera.org:8080/12918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 04 Apr 2019 01:01:34 +
Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry
..
Patch Set 3:
(6 comments)
a few comments, more are coming
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@77
PS3, Line 77: authorized_actions
I understand that it's directly related to Sentry's terminology, but I'm not
sure we want to continue confusing ourselves and others who might be reading
this code...
I would expect those to be named somehow that reflects the fact that those are
granted privileges, not authorized actions. By my understanding, the
AuthzProvider is the authority to authorize actions based on the set of granted
privileges.
What do you think?
https://en.wikipedia.org/wiki/Privilege_(computing)
https://www.postgresql.org/docs/9.0/sql-grant.html
Maybe, name it 'granted_privileges' instead?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@87
PS3, Line 87: the privileges apply.
I'm not sure I understand what 'apply' means in this context. So, maybe change
into '... the user is granted privileges at ...'.
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@90
PS3, Line 90: apply to a single table for a single user.
Is it going to be bound to the table only?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@95
PS3, Line 95: const std::string& table_ident,
I'm not sure I understand why this parameter is necessary given the description
of this method. So, given some table 'A' and privileges that apply to it (say,
at table level only), it's possible to use this method to deduce whether this
set of privileges implies some action at a table 'B', maybe in other database?
From the other point, if we are about to include object identifier as
'table_identifier' here to verify it matches with what is passed as a
parameter, why not to include username as well?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h
File src/kudu/sentry/sentry_authorizable_scope.h:
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91
PS3, Line 91: AuthorizableScopesSet
Is it possible to create the result AuthorizableScopesSet once and the just
return references to those? E.g., maybe declare those as static inside the
function and populate those just once.
Also, is it really important to declare this function to be inline and in this
header? Maybe, just move the definition into sentry_authz_provider.cc file
(the only place it's used now, if I'm not mistaked) and drop the 'inline'
specified to allow the compiler do the right thing?
http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@114
PS3, Line 114: inline AuthorizableScopesSet
ExpectedNonEmptyFields(SentryAuthorizableScope::Scope scope) {
: AuthorizableScopesSet expected_nonempty_fields;
: switch (scope) {
: case SentryAuthorizableScope::COLUMN:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::COLUMN);
: FALLTHROUGH_INTENDED;
: case SentryAuthorizableScope::TABLE:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::TABLE);
: FALLTHROUGH_INTENDED;
: case SentryAuthorizableScope::DATABASE:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::DATABASE);
: FALLTHROUGH_INTENDED;
: case SentryAuthorizableScope::SERVER:
: InsertOrDie(&expected_nonempty_fields,
SentryAuthorizableScope::SERVER);
: break;
: default:
: LOG(DFATAL) << "not reachable";
: }
: return expected_nonempty_fields;
: }
ditto
--
To view, visit http://gerrit.cloudera.org:8080/12919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b
Gerrit-Change-Number: 12919
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Apr 2019 00:50:57 +
Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12918 )
Change subject: util: pull Random methods out from tests
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h
File src/kudu/util/random_util.h:
http://gerrit.cloudera.org:8080/#/c/12918/5/src/kudu/util/random_util.h@75
PS5, Line 75: Rand* r, std::vector* result) {
Nit: could we put 'r' first in the list? Because this is "almost" a Random
method, I think it feels more natural for the PRNG to be the first argument
keyed in.
--
To view, visit http://gerrit.cloudera.org:8080/12918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 04 Apr 2019 00:37:25 +
Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12918 ) Change subject: util: pull Random methods out from tests .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149 PS3, Line 149: std::lock_guard l(lock_); > But isn't it sufficient to hold the lock around the call to Uniform() withi Ah I think you're right. Done. -- To view, visit http://gerrit.cloudera.org:8080/12918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 Gerrit-Change-Number: 12918 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 03 Apr 2019 23:41:34 + Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12918 to look at the new patch set (#5). Change subject: util: pull Random methods out from tests .. util: pull Random methods out from tests I've found a couple of Random utility methods pretty useful in some tests I'm writing, so I'm pulling them out into random_util. Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 --- M src/kudu/master/placement_policy.cc M src/kudu/tserver/tablet_server_authorization-test.cc M src/kudu/util/random-test.cc M src/kudu/util/random.h M src/kudu/util/random_util.h 5 files changed, 80 insertions(+), 78 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/12918/5 -- To view, visit http://gerrit.cloudera.org:8080/12918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 Gerrit-Change-Number: 12918 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] util: pull Random methods out from tests
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12918 ) Change subject: util: pull Random methods out from tests .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149 PS3, Line 149: > The trouble with ThreadSafeRandom.ReservoirSample as a template is that it' But isn't it sufficient to hold the lock around the call to Uniform() within ReservoirSample rather than for the entirety of the method? -- To view, visit http://gerrit.cloudera.org:8080/12918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 Gerrit-Change-Number: 12918 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 03 Apr 2019 23:23:48 + Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12918 ) Change subject: util: pull Random methods out from tests .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149 PS3, Line 149: > I wonder if we should move these methods as well as the other "utility" met The trouble with ThreadSafeRandom.ReservoirSample as a template is that it's not really doing the exact same thing as Random.ReservoirSample on account of it taking a lock. That said, the utilities based on top of it are, so those are fair game. http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@152 PS3, Line 152: public: > Maybe rename these a bit to emphasize their similarity? Like, OneFromContai Went with SelectRandomSubset and SelectRandomElement. -- To view, visit http://gerrit.cloudera.org:8080/12918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 Gerrit-Change-Number: 12918 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 03 Apr 2019 23:19:39 + Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12918 to look at the new patch set (#4). Change subject: util: pull Random methods out from tests .. util: pull Random methods out from tests I've found a couple of Random utility methods pretty useful in some tests I'm writing, so I'm pulling them out into random_util. Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 --- M src/kudu/tserver/tablet_server_authorization-test.cc M src/kudu/util/random_util.h 2 files changed, 38 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/12918/4 -- To view, visit http://gerrit.cloudera.org:8080/12918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 Gerrit-Change-Number: 12918 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] dist test.py: support --collect-tmpdir in Java tests
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12924 ) Change subject: dist_test.py: support --collect-tmpdir in Java tests .. Patch Set 2: Verified+1 Overriding Jenkins, more flakes. -- To view, visit http://gerrit.cloudera.org:8080/12924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e Gerrit-Change-Number: 12924 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 03 Apr 2019 22:08:42 + Gerrit-HasComments: No
[kudu-CR] dist test.py: support --collect-tmpdir in Java tests
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12924 ) Change subject: dist_test.py: support --collect-tmpdir in Java tests .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e Gerrit-Change-Number: 12924 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Comment-Date: Wed, 03 Apr 2019 22:09:28 + Gerrit-HasComments: No
[kudu-CR] dist test.py: support --collect-tmpdir in Java tests
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12924 ) Change subject: dist_test.py: support --collect-tmpdir in Java tests .. dist_test.py: support --collect-tmpdir in Java tests KUDU-2761 means this isn't useful yet, but it will be when that's fixed. I also snuck in a fix for a typo in RetryRule. Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e Reviewed-on: http://gerrit.cloudera.org:8080/12924 Tested-by: Adar Dembo Reviewed-by: Grant Henke --- M build-support/dist_test.py M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java 3 files changed, 37 insertions(+), 14 deletions(-) Approvals: Adar Dembo: Verified Grant Henke: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e Gerrit-Change-Number: 12924 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke
[kudu-CR] dist test.py: support --collect-tmpdir in Java tests
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12924 ) Change subject: dist_test.py: support --collect-tmpdir in Java tests .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e Gerrit-Change-Number: 12924 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke
[kudu-CR] dist test.py: support --collect-tmpdir in Java tests
Hello Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12924 to look at the new patch set (#2). Change subject: dist_test.py: support --collect-tmpdir in Java tests .. dist_test.py: support --collect-tmpdir in Java tests KUDU-2761 means this isn't useful yet, but it will be when that's fixed. I also snuck in a fix for a typo in RetryRule. Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e --- M build-support/dist_test.py M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java 3 files changed, 37 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/12924/2 -- To view, visit http://gerrit.cloudera.org:8080/12924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e Gerrit-Change-Number: 12924 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.9.x) [docs] Update known issues docs for location awareness
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12925 ) Change subject: [docs] Update known issues docs for location awareness .. [docs] Update known issues docs for location awareness Removes the clauses indicating rack awareness and multi-datacenter are not supported and adds details on the remaining limitations. Change-Id: I9b5f32f1ab14305bdd83e0d6bf3f8ef2ca7cea8b Reviewed-on: http://gerrit.cloudera.org:8080/12925 Tested-by: Grant Henke Reviewed-by: Alexey Serbin --- M docs/known_issues.adoc 1 file changed, 6 insertions(+), 2 deletions(-) Approvals: Grant Henke: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: merged Gerrit-Change-Id: I9b5f32f1ab14305bdd83e0d6bf3f8ef2ca7cea8b Gerrit-Change-Number: 12925 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR](branch-1.9.x) [docs] Update known issues docs for location awareness
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12925 ) Change subject: [docs] Update known issues docs for location awareness .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: I9b5f32f1ab14305bdd83e0d6bf3f8ef2ca7cea8b Gerrit-Change-Number: 12925 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 21:23:20 + Gerrit-HasComments: No
[kudu-CR](branch-1.9.x) [docs] Update known issues docs for location awareness
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12925 ) Change subject: [docs] Update known issues docs for location awareness .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: I9b5f32f1ab14305bdd83e0d6bf3f8ef2ca7cea8b Gerrit-Change-Number: 12925 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 21:10:02 + Gerrit-HasComments: No
[kudu-CR] [docs] Update known issues docs for location awareness
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12920 ) Change subject: [docs] Update known issues docs for location awareness .. [docs] Update known issues docs for location awareness Removes the clauses indicating rack awareness and multi-datacenter are not supported and adds details on the remaining limitations. Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Reviewed-on: http://gerrit.cloudera.org:8080/12920 Tested-by: Grant Henke Reviewed-by: Alexey Serbin --- M docs/known_issues.adoc 1 file changed, 6 insertions(+), 4 deletions(-) Approvals: Grant Henke: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12917/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java: http://gerrit.cloudera.org:8080/#/c/12917/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@61 PS4, Line 61: il will -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 03 Apr 2019 21:16:27 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) [docs] Update known issues docs for location awareness
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12925 Change subject: [docs] Update known issues docs for location awareness .. [docs] Update known issues docs for location awareness Removes the clauses indicating rack awareness and multi-datacenter are not supported and adds details on the remaining limitations. Change-Id: I9b5f32f1ab14305bdd83e0d6bf3f8ef2ca7cea8b --- M docs/known_issues.adoc 1 file changed, 6 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12925/1 -- To view, visit http://gerrit.cloudera.org:8080/12925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: newchange Gerrit-Change-Id: I9b5f32f1ab14305bdd83e0d6bf3f8ef2ca7cea8b Gerrit-Change-Number: 12925 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] wip sentry: generate authz tokens
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12897 )
Change subject: wip sentry: generate authz tokens
..
Patch Set 1:
(5 comments)
I need to rebase this on top of https://gerrit.cloudera.org/c/12919/
http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/catalog_manager.cc@2752
PS1, Line 2752: token_signer && user
> Is it valid to have situations when { token_signer != nullptr, user == boo
token_signer != nullptr, user == boost::none: user==none AFAIK is a test-only
scenario.
token_signer == nullptr, user != boost::none: non-standard, but this would be
the case if a user were to set the --master_supports_authz_tokens=false.
http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@43
PS1, Line 43: struct SentryPrivilege {
> Add a few lines of comments explaining why this structure is useful.
Done
http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@47
PS1, Line 47: boost::optional column)
> nit: for convenience, maybe make last two parameters boost::none by default
Changed this to not use boost at all. With sufficient checking up front, I
don't think this will complicate the mental model for this struct, and it
avoids overhead of boost::optional.
http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@52
PS1, Line 52: #ifndef NDEBUG
> What if set 'scope' to DATABASE and specify db, table, and column as well?
I don't think so, it's not one we expect anyway.
E.g. say we have
INSERT ON DATABASE for db->table->col
What should we do with this? Treat it as INSERT ON DATABASE for db? Treat it as
INSERT ON COL for db->table->col? Both of these would seem incorrect, so it's
probably safest to just ignore it.
http://gerrit.cloudera.org:8080/#/c/12897/1/src/kudu/master/sentry_authz_provider.h@70
PS1, Line 70: sentry::SentryAuthorizableScope::Scope scope;
> Is it necessary to store the scope once the structure is constructed? If i
Not necessary, but convenient. Done
--
To view, visit http://gerrit.cloudera.org:8080/12897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I052432408045c48f6fe9bf921fd3cb6bcc36e9ad
Gerrit-Change-Number: 12897
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 03 Apr 2019 21:08:08 +
Gerrit-HasComments: Yes
[kudu-CR] [docs] Update known issues docs for location awareness
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12920 ) Change subject: [docs] Update known issues docs for location awareness .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 21:02:39 + Gerrit-HasComments: No
[kudu-CR] dist test.py: support --collect-tmpdir in Java tests
Hello Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/12924 to review the following change. Change subject: dist_test.py: support --collect-tmpdir in Java tests .. dist_test.py: support --collect-tmpdir in Java tests KUDU-2761 means this isn't useful yet, but it will be when that's fixed. Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e --- M build-support/dist_test.py M java/buildSrc/src/main/groovy/org/apache/kudu/gradle/DistTestTask.java 2 files changed, 36 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/12924/1 -- To view, visit http://gerrit.cloudera.org:8080/12924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic8bb699c78b3b1ea64c92d53996aa4e5c8cfbe0e Gerrit-Change-Number: 12924 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 ) Change subject: java/c++: ColumnSchema supports storing column comment .. Patch Set 6: Note: This has a merge conflict. You should rebase on master when you update the patch. -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 6 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 03 Apr 2019 20:47:17 + Gerrit-HasComments: No
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. build: adapt new Java flaky test infrastructure to existing controls Now that Java tests are reporting success/failure, we can use the existing flaky test controls to drive it. As a refresher, the C++ tests rely on these environment variables: - RUN_FLAKY_ONLY: whether to run just flaky tests or all tests - KUDU_FLAKY_TEST_ATTEMPTS: number of attempts for flaky tests - KUDU_FLAKY_TEST_LIST: path to list of flaky tests, one on each line - KUDU_RETRY_ALL_FAILED_TESTS: whether to retry all tests or just the ones in the flaky test list The algorithm is roughly: if RUN_FLAKY_ONLY or KUDU_FLAKY_TEST_ATTEMPTS > 1: populate KUDU_FLAKY_TEST_LIST from test result server if RUN_FLAKY_ONLY: testset = tests listed in KUDU_FLAKY_TEST_LIST else: testset = all tests for t in testset: if KUDU_RETRY_ALL_FAILED_TESTS or (KUDU_FLAKY_TEST_LIST and t in KUDU_FLAKY_TEST_LIST): num_attempts = KUDU_FLAKY_TEST_ATTEMPTS (or 1 if unset) else: num_attempts = 1 run t up to num_attempts times You can see it at work in build-and-test.sh/run-test.sh. You can also see it in dist-test.py though notably, it doesn't care about RUN_FLAKY_ONLY because we never used that particular combination (presumably the list of flaky tests is short enough that it wouldn't benefit from distributed testing). This patch attempts to mirror these exact semantics for Java tests. Here are the interesting changes: - In RetryRule, rerunFailingTestsCount is gone. The behavior is informed via the aforementioned environment variables instead. - In build-and-test.sh, if RUN_FLAKY_ONLY is set, parse the flaky test list into a series of --tests gradle command line arguments. - In dist-test.py, opt into the C++ flaky test handling (which reflects the above algorithm). There are also some small changes to flaky handling to accommodate Java's per-method flaky test tracking. Note: all of this assumes that there's no overlap between the names of any C++ or Java tests, which is currently true as all C++ tests have names like "tablet-test" or "master_cert_authority-itest" while all Java tests are prefixed with "org.apache.kudu...". If this were to change, we'd need to properly "namespace" the test results in the reporting infrastructure and fetch the flaky test lists separately for C++ and Java tests. For now there's just one flaky test list, and both ctest and gradle are OK with being asked to run irrelevant tests (they'll just be ignored). Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Reviewed-on: http://gerrit.cloudera.org:8080/12917 Reviewed-by: Grant Henke Tested-by: Adar Dembo --- M build-support/dist_test.py M build-support/jenkins/build-and-test.sh M java/gradle/tests.gradle M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java 4 files changed, 145 insertions(+), 31 deletions(-) Approvals: Grant Henke: Looks good to me, approved Adar Dembo: Verified -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy
[kudu-CR] build: enable Java flaky test reporting
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12043 ) Change subject: build: enable Java flaky test reporting .. build: enable Java flaky test reporting This patch moves flaky test environment setup out of report-test.sh and into build-and-test.sh so that those environment variables can be inherited by the Java build environment. That change enables flaky test reporting for Java tests. This was tested in a RHEL6 DEBUG build environment. Example: http://dist-test.cloudera.org:8080/test_drilldown?test_name=testHiveMetastoreIntegration%28org.apache.kudu.test.TestMiniKuduCluster%29 Change-Id: Ifef74fc9bf5453105c267418fa24daf4c33f73f3 Reviewed-on: http://gerrit.cloudera.org:8080/12043 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke --- M build-support/jenkins/build-and-test.sh M build-support/report-test.sh 2 files changed, 53 insertions(+), 32 deletions(-) Approvals: Kudu Jenkins: Verified Grant Henke: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ifef74fc9bf5453105c267418fa24daf4c33f73f3 Gerrit-Change-Number: 12043 Gerrit-PatchSet: 10 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] java: add support for flaky test reporting
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12042 ) Change subject: java: add support for flaky test reporting .. java: add support for flaky test reporting This patch hooks into the existing RetryRule to report test results to the flaky test server inline as the tests are executed. All of the actual reporting logic is factored out into a separate ResultReporter class. The interface for the test reporter to pass relevant information about the build environment to the flaky test server is based on environment variables. This includes configuration and build metadata such as flaky test server address, git revision, build id, build host, and build type. This patch also includes a simple integration test for the reporter using a mocked-up flaky test server HTTP endpoint. This patch does not integrate the above functionality into the build. That will happen in a follow-up patch. Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74 Reviewed-on: http://gerrit.cloudera.org:8080/12042 Tested-by: Adar Dembo Reviewed-by: Grant Henke --- M java/gradle/dependencies.gradle M java/kudu-test-utils/build.gradle M java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java A java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingToFileLogAppender.java A java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java A java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestRetryRule.java 8 files changed, 721 insertions(+), 18 deletions(-) Approvals: Adar Dembo: Verified Grant Henke: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74 Gerrit-Change-Number: 12042 Gerrit-PatchSet: 10 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] java: ensure KuduTestHarness or RetryRule in every test
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12872 ) Change subject: java: ensure KuduTestHarness or RetryRule in every test .. java: ensure KuduTestHarness or RetryRule in every test Now that test reporting is built into the RetryRule, we should ensure that every test uses either RetryRule or KuduTestHarness (which wraps RetryRule). This patch adds RetryRule to all tests that were missing one of the two. Change-Id: I951f9fbb516abdb24a74d5a2acd7e1f1cd8a6fa5 Reviewed-on: http://gerrit.cloudera.org:8080/12872 Tested-by: Adar Dembo Reviewed-by: Grant Henke --- M java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestBitSet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestBloomFilter.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestColumnRangePredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestErrorCollector.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiator.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRequestTracker.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcTraces.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestStatus.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeoutTracker.java M java/kudu-client/src/test/java/org/apache/kudu/util/TestAsyncUtil.java M java/kudu-client/src/test/java/org/apache/kudu/util/TestByteVec.java M java/kudu-client/src/test/java/org/apache/kudu/util/TestMurmurHash.java M java/kudu-client/src/test/java/org/apache/kudu/util/TestNetUtil.java M java/kudu-client/src/test/java/org/apache/kudu/util/TestStringUtil.java M java/kudu-client/src/test/java/org/apache/kudu/util/TestTimestampUtil.java M java/kudu-mapreduce/src/test/java/org/apache/kudu/mapreduce/TestJarFinder.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/cluster/TestKuduBinaryJarExtractor.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java 27 files changed, 193 insertions(+), 46 deletions(-) Approvals: Adar Dembo: Verified Grant Henke: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I951f9fbb516abdb24a74d5a2acd7e1f1cd8a6fa5 Gerrit-Change-Number: 12872 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Update known issues docs for location awareness
Grant Henke has removed a vote on this change. Change subject: [docs] Update known issues docs for location awareness .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Update known issues docs for location awareness
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12920 ) Change subject: [docs] Update known issues docs for location awareness .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 19:23:32 + Gerrit-HasComments: No
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Patch Set 3: Verified+1 More unknown Java flakes. -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 03 Apr 2019 18:59:00 + Gerrit-HasComments: No
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy
[kudu-CR] [docs] Update known issues docs for location awareness
Hello Will Berkeley, Alex Rodoni, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12920 to look at the new patch set (#2). Change subject: [docs] Update known issues docs for location awareness .. [docs] Update known issues docs for location awareness Removes the clauses indicating rack awareness and multi-datacenter are not supported and adds details on the remaining limitations. Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 --- M docs/known_issues.adoc 1 file changed, 6 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/12920/2 -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 03 Apr 2019 18:38:05 + Gerrit-HasComments: No
[kudu-CR] add document for KUDU-2080
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12774 ) Change subject: add document for KUDU-2080 .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12774 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a802a846ad5ec93ce4e0022ec279f1b4c6cc5db Gerrit-Change-Number: 12774 Gerrit-PatchSet: 1 Gerrit-Owner: Jia Hongchao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 03 Apr 2019 18:50:21 + Gerrit-HasComments: No
[kudu-CR] [docs] Update known issues docs for location awarness
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12920 ) Change subject: [docs] Update known issues docs for location awarness .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12920/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12920/1//COMMIT_MSG@9 PS1, Line 9: awarness > ditto Done http://gerrit.cloudera.org:8080/#/c/12920/1//COMMIT_MSG@9 PS1, Line 9: about > remove Done http://gerrit.cloudera.org:8080/#/c/12920/1/docs/known_issues.adoc File docs/known_issues.adoc: http://gerrit.cloudera.org:8080/#/c/12920/1/docs/known_issues.adoc@109 PS1, Line 109: * Multi-datacenter configurations require at least 3 datacenters to maintain availability. > +1. For thoroughness, add multi-availability-zone. Users can reason about m Done -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 18:49:43 + Gerrit-HasComments: Yes
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Hello Mike Percy, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12917 to look at the new patch set (#3). Change subject: build: adapt new Java flaky test infrastructure to existing controls .. build: adapt new Java flaky test infrastructure to existing controls Now that Java tests are reporting success/failure, we can use the existing flaky test controls to drive it. As a refresher, the C++ tests rely on these environment variables: - RUN_FLAKY_ONLY: whether to run just flaky tests or all tests - KUDU_FLAKY_TEST_ATTEMPTS: number of attempts for flaky tests - KUDU_FLAKY_TEST_LIST: path to list of flaky tests, one on each line - KUDU_RETRY_ALL_FAILED_TESTS: whether to retry all tests or just the ones in the flaky test list The algorithm is roughly: if RUN_FLAKY_ONLY or KUDU_FLAKY_TEST_ATTEMPTS > 1: populate KUDU_FLAKY_TEST_LIST from test result server if RUN_FLAKY_ONLY: testset = tests listed in KUDU_FLAKY_TEST_LIST else: testset = all tests for t in testset: if KUDU_RETRY_ALL_FAILED_TESTS or (KUDU_FLAKY_TEST_LIST and t in KUDU_FLAKY_TEST_LIST): num_attempts = KUDU_FLAKY_TEST_ATTEMPTS (or 1 if unset) else: num_attempts = 1 run t up to num_attempts times You can see it at work in build-and-test.sh/run-test.sh. You can also see it in dist-test.py though notably, it doesn't care about RUN_FLAKY_ONLY because we never used that particular combination (presumably the list of flaky tests is short enough that it wouldn't benefit from distributed testing). This patch attempts to mirror these exact semantics for Java tests. Here are the interesting changes: - In RetryRule, rerunFailingTestsCount is gone. The behavior is informed via the aforementioned environment variables instead. - In build-and-test.sh, if RUN_FLAKY_ONLY is set, parse the flaky test list into a series of --tests gradle command line arguments. - In dist-test.py, opt into the C++ flaky test handling (which reflects the above algorithm). There are also some small changes to flaky handling to accommodate Java's per-method flaky test tracking. Note: all of this assumes that there's no overlap between the names of any C++ or Java tests, which is currently true as all C++ tests have names like "tablet-test" or "master_cert_authority-itest" while all Java tests are prefixed with "org.apache.kudu...". If this were to change, we'd need to properly "namespace" the test results in the reporting infrastructure and fetch the flaky test lists separately for C++ and Java tests. For now there's just one flaky test list, and both ctest and gradle are OK with being asked to run irrelevant tests (they'll just be ignored). Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae --- M build-support/dist_test.py M build-support/jenkins/build-and-test.sh M java/gradle/tests.gradle M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java 4 files changed, 145 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/12917/3 -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh@246 PS1, Line 246: EXTRA_GRADLE_TEST_FLAGS="--tests $t $EXTRA_GRADLE_TEST_FLAGS" > Not accidentally, but yes. :) Mind a comment here that acknowledges we intend to pass them to gradle? A mostly copy past of this is okay. I feel like it's something we would second guess later otherwise. -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 03 Apr 2019 18:30:30 + Gerrit-HasComments: Yes
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh@246 PS1, Line 246: # 1. There are no test name collisions between C++ and Java tests. > Mind a comment here that acknowledges we intend to pass them to gradle? A m Done -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 03 Apr 2019 18:36:25 + Gerrit-HasComments: Yes
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 03 Apr 2019 18:30:39 + Gerrit-HasComments: No
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Patch Set 2: Verified+1 A Java test failed, but as before, I think it was an unknown flake that was previously retried because we retried all tests three times. Going forward we should start accumulating the failures in the flaky test dashboard. -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 03 Apr 2019 18:25:50 + Gerrit-HasComments: No
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12917 ) Change subject: build: adapt new Java flaky test infrastructure to existing controls .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Hello Mike Percy, Andrew Wong, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12917 to look at the new patch set (#2). Change subject: build: adapt new Java flaky test infrastructure to existing controls .. build: adapt new Java flaky test infrastructure to existing controls Now that Java tests are reporting success/failure, we can use the existing flaky test controls to drive it. As a refresher, the C++ tests rely on these environment variables: - RUN_FLAKY_ONLY: whether to run just flaky tests or all tests - KUDU_FLAKY_TEST_ATTEMPTS: number of attempts for flaky tests - KUDU_FLAKY_TEST_LIST: path to list of flaky tests, one on each line - KUDU_RETRY_ALL_FAILED_TESTS: whether to retry all tests or just the ones in the flaky test list The algorithm is roughly: if RUN_FLAKY_ONLY or KUDU_FLAKY_TEST_ATTEMPTS > 1: populate KUDU_FLAKY_TEST_LIST from test result server if RUN_FLAKY_ONLY: testset = tests listed in KUDU_FLAKY_TEST_LIST else: testset = all tests for t in testset: if KUDU_RETRY_ALL_FAILED_TESTS or (KUDU_FLAKY_TEST_LIST and t in KUDU_FLAKY_TEST_LIST): num_attempts = KUDU_FLAKY_TEST_ATTEMPTS (or 1 if unset) else: num_attempts = 1 run t up to num_attempts times You can see it at work in build-and-test.sh/run-test.sh. You can also see it in dist-test.py though notably, it doesn't care about RUN_FLAKY_ONLY because we never used that particular combination (presumably the list of flaky tests is short enough that it wouldn't benefit from distributed testing). This patch attempts to mirror these exact semantics for Java tests. Here are the interesting changes: - In RetryRule, rerunFailingTestsCount is gone. The behavior is informed via the aforementioned environment variables instead. - In build-and-test.sh, if RUN_FLAKY_ONLY is set, parse the flaky test list into a series of --tests gradle command line arguments. - In dist-test.py, opt into the C++ flaky test handling (which reflects the above algorithm). There are also some small changes to flaky handling to accommodate Java's per-method flaky test tracking. Note: all of this assumes that there's no overlap between the names of any C++ or Java tests, which is currently true as all C++ tests have names like "tablet-test" or "master_cert_authority-itest" while all Java tests are prefixed with "org.apache.kudu...". If this were to change, we'd need to properly "namespace" the test results in the reporting infrastructure and fetch the flaky test lists separately for C++ and Java tests. For now there's just one flaky test list, and both ctest and gradle are OK with being asked to run irrelevant tests (they'll just be ignored). Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae --- M build-support/dist_test.py M build-support/jenkins/build-and-test.sh M java/gradle/tests.gradle M java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java 4 files changed, 137 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/12917/2 -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12917 )
Change subject: build: adapt new Java flaky test infrastructure to existing
controls
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/dist_test.py@369
PS1, Line 369: test_name = ".".join(k.split(".")[:-1]) if
TEST_SHARD_RE.search(k) else k
> If I understand right, this doesn't mangle the java names because there is
Correct. And it looks like every class/package identifier needs to have at
least one letter[1], so no Java test should ever match this regex.
1. https://stackoverflow.com/a/65490
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh
File build-support/jenkins/build-and-test.sh:
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh@246
PS1, Line 246: EXTRA_GRADLE_TEST_FLAGS="--tests $t
$EXTRA_GRADLE_TEST_FLAGS"
> Will this accidentally pass the flaky c++ tests to the Gradle --tests flag?
Not accidentally, but yes. :)
As I wrote in the commit message, the shortcut that simplified this patch was
to _not_ separate the list of C++ and Java flakies. We can get away with it
because:
1. There are no collisions between C++/Java test names.
2. Both ctest (with "-R $test_regex") and gradle (with multiple "--tests $t"
entries) happily ignore tests they can't find.
A cleaner way would be to enforce the separation, but that means doing one of:
1. Splitting the unified flaky list here into two lists based on some
name-based criteria.
2. Adding another flaky list retrieval endpoint to test_result_server.py and
modifying the SQL queries to do the same name-based criteria.
3. Like #2 but modifying the reporting to include a bool for C++ or Java test,
and changing the results schema accordingly.
I opted for quick-and-dirty.
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
File
java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java:
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@51
PS1, Line 51: private static final int MAYBE_RETRY = 0;
> Might be more clear to call this NO_EXPLICIT_RETRIES or something. Alternat
Done
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@57
PS1, Line 57: this(MAYBE_RETRY, /*skipReporting=*/ false);
> Should this be DEFAULT_RETRY_COUNT?
Done
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@72
PS1, Line 72: String value = System.getenv("KUDU_FLAKY_TEST_LIST");
> I am not sure how costly these System.getenv calls are, but assuming this K
System.getenv() should be cheap; it's not even a system call [1]. I don't think
a map would save us any time, as we only call retryThisTest() once per
RetryRule.
Unless you're suggesting a static map created in a static block. I find static
blocks somewhat icky, but it would reduce the number of times we read the file,
so maybe it is worth doing.
1.
https://stackoverflow.com/questions/46623018/does-a-program-make-a-system-call-to-get-the-value-of-an-environment-variable-in
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@128
PS1, Line 128: reporter != null
> Do we need this condition? I think below it would always result in `actualR
Yes, counter-intuitively it's OK to construct a RetryStatement with
actualRetryCount == 0, because that's a test that won't be retried but will
report its results. Put another way, reporting and retrying should be
independently controllable (see the comment just above).
I don't think we take advantage of this particular combination, but the C++
tests support it, so I figured I'd at least reach parity with them.
--
To view, visit http://gerrit.cloudera.org:8080/12917
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae
Gerrit-Change-Number: 12917
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Wed, 03 Apr 2019 17:51:52 +
Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12918 ) Change subject: util: pull Random methods out from tests .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h File src/kudu/util/random.h: http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149 PS3, Line 149: > Could you add ThreadSafeRandom equivalents too? I wonder if we should move these methods as well as the other "utility" methods like ReservoirSample into random_util.h instead? We could templatize them on the RNG and avoid having to duplicate all the methods. -- To view, visit http://gerrit.cloudera.org:8080/12918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 Gerrit-Change-Number: 12918 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 03 Apr 2019 17:44:48 + Gerrit-HasComments: Yes
[kudu-CR] util: pull Random methods out from tests
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12918 )
Change subject: util: pull Random methods out from tests
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h
File src/kudu/util/random.h:
http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@149
PS3, Line 149:
Could you add ThreadSafeRandom equivalents too?
http://gerrit.cloudera.org:8080/#/c/12918/3/src/kudu/util/random.h@152
PS3, Line 152: T SelectAtRandom(const Container& c) {
Maybe rename these a bit to emphasize their similarity? Like, OneFromContainer
and SomeFromContainer? Okay, those are pretty bad, but maybe something along
those lines?
--
To view, visit http://gerrit.cloudera.org:8080/12918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93
Gerrit-Change-Number: 12918
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 03 Apr 2019 17:19:59 +
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Remove incorrect comments
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12921 ) Change subject: [metrics] Remove incorrect comments .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12921/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12921/1//COMMIT_MSG@10 PS1, Line 10: and it's not needed to sort metrics. I see how this was previously broken, but don't we want to sort the output? Why not keep the OrderedMetricsMap and add the right comparator? Or convert the map into a vector of pairs and sort that before printing? http://gerrit.cloudera.org:8080/#/c/12921/1/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/12921/1/src/kudu/util/metrics.cc@224 PS1, Line 224: InsertOrDie(&metrics, prototype, metric); With 'metrics' now being a MetricMap, it may be faster to reduce the amount of time spent holding lock_ by making a complete local copy of metric_map_, then manipulating it to remove unwanted metrics after releasing lock_. -- To view, visit http://gerrit.cloudera.org:8080/12921 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8788c38b73c623616b3fb2b73fcb6973df4ec87 Gerrit-Change-Number: 12921 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 03 Apr 2019 17:13:05 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Update known issues docs for location awarness
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12920 ) Change subject: [docs] Update known issues docs for location awarness .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/12920/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12920/1//COMMIT_MSG@7 PS1, Line 7: awarness awareness http://gerrit.cloudera.org:8080/#/c/12920/1//COMMIT_MSG@9 PS1, Line 9: awarness ditto http://gerrit.cloudera.org:8080/#/c/12920/1//COMMIT_MSG@9 PS1, Line 9: about remove http://gerrit.cloudera.org:8080/#/c/12920/1/docs/known_issues.adoc File docs/known_issues.adoc: http://gerrit.cloudera.org:8080/#/c/12920/1/docs/known_issues.adoc@109 PS1, Line 109: * Multi-datacenter configurations require at least 3 datacenters to maintain availability. > Maybe I should add say "Multi-datacenter or Multi-availability-zone" or som +1. For thoroughness, add multi-availability-zone. Users can reason about more exotic setups that might exist using induction. -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 17:11:10 + Gerrit-HasComments: Yes
[kudu-CR] [metrics] Remove incorrect comments
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12921 Change subject: [metrics] Remove incorrect comments .. [metrics] Remove incorrect comments Data store in std::map with key type 'const char*' is not in alphabetical order, and it's not needed to sort metrics. Change-Id: Id8788c38b73c623616b3fb2b73fcb6973df4ec87 --- M src/kudu/util/metrics.cc 1 file changed, 4 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/12921/1 -- To view, visit http://gerrit.cloudera.org:8080/12921 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id8788c38b73c623616b3fb2b73fcb6973df4ec87 Gerrit-Change-Number: 12921 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com>
[kudu-CR] util: pull Random methods out from tests
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12918 to look at the new patch set (#3). Change subject: util: pull Random methods out from tests .. util: pull Random methods out from tests I've found a couple of Random utility methods pretty useful in some tests I'm writing, so I'm pulling them out into util. Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 --- M src/kudu/tserver/tablet_server_authorization-test.cc M src/kudu/util/random.h 2 files changed, 31 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/12918/3 -- To view, visit http://gerrit.cloudera.org:8080/12918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie74eca50d770b9470f6cfb400cf632381a20bd93 Gerrit-Change-Number: 12918 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12919 to look at the new patch set (#3). Change subject: sentry: sanitize and parse privileges from Sentry .. sentry: sanitize and parse privileges from Sentry Currently, we pass around the Thrift privileges received from Sentry, which can be both expensive memory-wise and cumbersome to use. This patch: - sanitizes the responses from Sentry, only keeping those that are well-formed and potentially Kudu-related, - stores them in a more ergonomic form, e.g. keeping around enums rather than strings for SentryActions, etc. This form may be updated in the future to facilitate privilege evaluation -- for now, my goal is just to make it easier to work with Sentry privileges, - encapsulates the above in an abstracted version of a Sentry response that corresponds to the hierarchy tree for a given table, with the hope that it will make changing the in-memory format more painless, - switches the SentryAuthorizableScope and SentryAction enum classes to enums, to avoid having to use the extra enum class typename everywhere (e.g. now SentryAuthorizableScope::SERVER instead of SentryAuthorizableScope::Scope::SERVER will suffice). Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b --- M src/kudu/gutil/map-util.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 M src/kudu/sentry/sentry_action.h M src/kudu/sentry/sentry_authorizable_scope.h 6 files changed, 593 insertions(+), 84 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12919/3 -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [docs] Update known issues docs for location awarness
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12920 ) Change subject: [docs] Update known issues docs for location awarness .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12920/1/docs/known_issues.adoc File docs/known_issues.adoc: http://gerrit.cloudera.org:8080/#/c/12920/1/docs/known_issues.adoc@109 PS1, Line 109: * Multi-datacenter configurations require at least 3 datacenters to maintain availability. Maybe I should add say "Multi-datacenter or Multi-availability-zone" or something like that. Any suggestions? -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 15:49:19 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Update known issues docs for location awarness
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12920 Change subject: [docs] Update known issues docs for location awarness .. [docs] Update known issues docs for location awarness Removes the clauses about indicating rack awarness and multi-datacenter are not supported and adds details on the remaining limitations. Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 --- M docs/known_issues.adoc 1 file changed, 5 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/12920/1 -- To view, visit http://gerrit.cloudera.org:8080/12920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I5b083cdf60629aacef3a3ac186a2191f8d7a00d0 Gerrit-Change-Number: 12920 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] build: adapt new Java flaky test infrastructure to existing controls
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/12917 )
Change subject: build: adapt new Java flaky test infrastructure to existing
controls
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/dist_test.py@369
PS1, Line 369: test_name = ".".join(k.split(".")[:-1]) if
TEST_SHARD_RE.search(k) else k
If I understand right, this doesn't mangle the java names because there is no
java class or package that is purely numerical?
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh
File build-support/jenkins/build-and-test.sh:
http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh@246
PS1, Line 246: EXTRA_GRADLE_TEST_FLAGS="--tests $t
$EXTRA_GRADLE_TEST_FLAGS"
Will this accidentally pass the flaky c++ tests to the Gradle --tests flag?
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java
File
java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java:
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@51
PS1, Line 51: private static final int MAYBE_RETRY = 0;
Might be more clear to call this NO_EXPLICIT_RETRIES or something.
Alternatively, I am not sure you need a constant for the value 0. It would make
the `retryCount != MAYBE_RETRY;` line below easier to read to just hard code 0.
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@57
PS1, Line 57: this(MAYBE_RETRY, /*skipReporting=*/ false);
Should this be DEFAULT_RETRY_COUNT?
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@72
PS1, Line 72: String value = System.getenv("KUDU_FLAKY_TEST_LIST");
I am not sure how costly these System.getenv calls are, but assuming this
KUDU_FLAKY_TEST_LIST could be fairly large, can we populate a map when we
initialize the rule and consult it in this method?
We could read/parse KUDU_RETRY_ALL_FAILED_TESTS and KUDU_FLAKY_TEST_ATTEMPTS at
construction time too, though the impact is likely lower.
http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@128
PS1, Line 128: reporter != null
Do we need this condition? I think below it would always result in
`actualRetryCount = 0` in the case one of the others isn't also true.
--
To view, visit http://gerrit.cloudera.org:8080/12917
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae
Gerrit-Change-Number: 12917
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Wed, 03 Apr 2019 14:11:04 +
Gerrit-HasComments: Yes
[kudu-CR] java: add support for flaky test reporting
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 ) Change subject: java: add support for flaky test reporting .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74 Gerrit-Change-Number: 12042 Gerrit-PatchSet: 9 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 13:35:45 + Gerrit-HasComments: No
[kudu-CR] build: enable Java flaky test reporting
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12043 ) Change subject: build: enable Java flaky test reporting .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifef74fc9bf5453105c267418fa24daf4c33f73f3 Gerrit-Change-Number: 12043 Gerrit-PatchSet: 9 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 13:35:54 + Gerrit-HasComments: No
[kudu-CR] java: ensure KuduTestHarness or RetryRule in every test
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12872 ) Change subject: java: ensure KuduTestHarness or RetryRule in every test .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12872 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I951f9fbb516abdb24a74d5a2acd7e1f1cd8a6fa5 Gerrit-Change-Number: 12872 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 03 Apr 2019 13:34:46 + Gerrit-HasComments: No
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/12890 )
Change subject: java/c++: ColumnSchema supports storing column comment
..
Patch Set 6:
(5 comments)
Thank you for all the work on this helifu.
http://gerrit.cloudera.org:8080/#/c/12890/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:
http://gerrit.cloudera.org:8080/#/c/12890/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@408
PS6, Line 408: throw new IllegalArgumentException("The comment should
not be empty");
I don't think we need this check. A user can set an empty comment and it should
be okay. Especially because empty is the default.
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.h@445
PS6, Line 445: /// The comment for the column. An empty comment means
deleting an
I think you can keep the docs as simple as "The comment for the column."
I don't think we need to view "empty comment" as s special deleting operation.
Because we default to "", the only thing you need to worry about is "setting" a
comment. Whether it's setting it to "some new string" or setting it to "", then
end result is the user set a string for the comment.
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/client/schema.cc@314
PS6, Line 314: return Status::InvalidArgument("cannot delete comment during
CreateTable",
See my other comments on ColumnSchema.java and schema.h.
I don't think we need to prevent users from "setting" a column to an empty
string.
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/common/schema.h@347
PS6, Line 347: // "empty comment" means "no comment".
nit: I don't think you need this doc line here.
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:
http://gerrit.cloudera.org:8080/#/c/12890/6/src/kudu/server/webui_util.cc@53
PS6, Line 53: col_json["comment"] = !col.comment().empty() ? col.comment()
: "-";
I don't think we need the empty check here. In the case the comment is empty,
we can still display it that way.
It may also be confusing if a user sets a column comment of "-" themselves.
--
To view, visit http://gerrit.cloudera.org:8080/12890
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c
Gerrit-Change-Number: 12890
Gerrit-PatchSet: 6
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Wed, 03 Apr 2019 13:33:47 +
Gerrit-HasComments: Yes
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Hello Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12890 to look at the new patch set (#6). Change subject: java/c++: ColumnSchema supports storing column comment .. java/c++: ColumnSchema supports storing column comment This patch intends to add a new api for storing column comment in java client. And according to the comments of the reviews, the empty comment is defined to delete the comment for the uniform behavior. In addition, the server-side has already been implemented in: https://gerrit.cloudera.org/#/c/12849/ Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c --- M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/client/client-test.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/wire_protocol.cc M src/kudu/master/catalog_manager.cc M src/kudu/server/webui_util.cc 14 files changed, 244 insertions(+), 75 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12890/6 -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 6 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Hello Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12890 to look at the new patch set (#5). Change subject: java/c++: ColumnSchema supports storing column comment .. java/c++: ColumnSchema supports storing column comment This patch intends to add a new api for storing column comment in java client. And according to the comments of the reviews, the empty comment is defined to delete the comment for the uniform behavior. In addition, the server-side has already been implemented in: https://gerrit.cloudera.org/#/c/12849/ Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c --- M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/client/client-test.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/wire_protocol.cc M src/kudu/master/catalog_manager.cc M src/kudu/server/webui_util.cc 14 files changed, 244 insertions(+), 74 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12890/5 -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 5 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] java/c++: ColumnSchema supports storing column comment
helifu has posted comments on this change. (
http://gerrit.cloudera.org:8080/12890 )
Change subject: java/c++: ColumnSchema supports storing column comment
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12890/3/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:
http://gerrit.cloudera.org:8080/#/c/12890/3/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@144
PS3, Line 144: if (pb.hasComment()) {
> You don't need the duplication. Try this:
Done
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/common/wire_protocol.cc@307
PS3, Line 307: std::move(comment)
> I think you may be able to omit L300-302 and just pass pb.comment() here. I
Yeah, it's cool. Thanks!
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/master/catalog_manager.cc@1343
PS3, Line 1343: return Status::OK();
> This depends on whether we're creating or altering, right? Isn't the empty
No, there is no need to distinguish between creating and modifying a table.
Please review the code L1439/1441 and Line2474/2483.
--
To view, visit http://gerrit.cloudera.org:8080/12890
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c
Gerrit-Change-Number: 12890
Gerrit-PatchSet: 4
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Wed, 03 Apr 2019 10:04:13 +
Gerrit-HasComments: Yes
[kudu-CR] java/c++: ColumnSchema supports storing column comment
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12890 ) Change subject: java/c++: ColumnSchema supports storing column comment .. Patch Set 4: Sorry for quickly retriggering the builder :( -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 4 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 03 Apr 2019 09:18:59 + Gerrit-HasComments: No
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Hello Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12890 to look at the new patch set (#4). Change subject: java/c++: ColumnSchema supports storing column comment .. java/c++: ColumnSchema supports storing column comment This patch intends to add a new api for storing column comment in java client. And according to the comments of the reviews, the empty comment is defined to delete the comment for the uniform behavior. In addition, the server-side has already been implemented in: https://gerrit.cloudera.org/#/c/12849/ Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c --- M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/client/client-test.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/wire_protocol.cc M src/kudu/master/catalog_manager.cc M src/kudu/server/webui_util.cc 14 files changed, 241 insertions(+), 61 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12890/4 -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 4 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12890 )
Change subject: java/c++: ColumnSchema supports storing column comment
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12890/3/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:
http://gerrit.cloudera.org:8080/#/c/12890/3/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@144
PS3, Line 144: if (pb.hasComment()) {
You don't need the duplication. Try this:
ColumnSchema.ColumnSchemaBuilder builder = new
ColumnSchema.ColumnSchemaBuilder(...).key(...)...
if (pb.hasComment()) {
builder = builder.comment(pb.getComment());
}
return builder.build();
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/common/wire_protocol.cc@307
PS3, Line 307: std::move(comment)
I think you may be able to omit L300-302 and just pass pb.comment() here. IIUC
the default value for strings in protobuf is the empty string, so the pb has
something "valid" regardless of whether has_comment() is true or false.
See https://developers.google.com/protocol-buffers/docs/proto#optional for more
details.
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12890/3/src/kudu/master/catalog_manager.cc@1343
PS3, Line 1343: return Status::OK();
This depends on whether we're creating or altering, right? Isn't the empty
string only valid for the latter?
--
To view, visit http://gerrit.cloudera.org:8080/12890
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c
Gerrit-Change-Number: 12890
Gerrit-PatchSet: 3
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu
Gerrit-Comment-Date: Wed, 03 Apr 2019 09:15:10 +
Gerrit-HasComments: Yes
[kudu-CR] java/c++: ColumnSchema supports storing column comment
Hello Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12890 to look at the new patch set (#3). Change subject: java/c++: ColumnSchema supports storing column comment .. java/c++: ColumnSchema supports storing column comment This patch intends to add a new api for storing column comment in java client. And according to the comments of the reviews, the empty comment is defined to delete the comment for the uniform behavior. In addition, the server-side has already been implemented in: https://gerrit.cloudera.org/#/c/12849/ Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c --- M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/client/client-test.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/wire_protocol.cc M src/kudu/master/catalog_manager.cc M src/kudu/server/webui_util.cc 14 files changed, 241 insertions(+), 61 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12890/3 -- To view, visit http://gerrit.cloudera.org:8080/12890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4cdca4101898062cfe154c15ca40c5943d0e343c Gerrit-Change-Number: 12890 Gerrit-PatchSet: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu
