[kudu-CR] tool: fixes for kudu local replica dump rowset
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12976 ) Change subject: tool: fixes for kudu local_replica dump rowset .. tool: fixes for kudu local_replica dump rowset This patch makes several adjustments to 'kudu local_replica dump rowset': - The existing 'metadata_only' and 'nrows' controls were being ignored. - The existing 'rowset_index' control wasn't working properly. - I changed the "what to dump" contols to 'dump_all_columns' and 'dump_metadata'. When 'dump_all_columns' is false, the row keys are dumped in a format that's comparable and ASCII-compatible (currently hex). This functionality helped me dump a tablet's keys (grouped by rowset), which I then used for a series of MergeIterator experiments. Change-Id: Ib50ab4e7b2aa0fec60ce0718d16823945a05cb7f Reviewed-on: http://gerrit.cloudera.org:8080/12976 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_local_replica.cc 2 files changed, 163 insertions(+), 57 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12976 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib50ab4e7b2aa0fec60ce0718d16823945a05cb7f Gerrit-Change-Number: 12976 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] experiments: merge iterator optimization tests
Adar Dembo has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/12587 )
Change subject: experiments: merge iterator optimization tests
..
experiments: merge iterator optimization tests
Here's a brief exploration into various MergeIterator algorithms, prototyped
in Python. Only after I was done did I see that there was an existing
experiment on this same subject in C++ (see merge-test.cc). It's not all
wasted work though; that experiment didn't include the new "hot/cold" heap
algorithms, nor did it account for all MergeIterator quirks such as paged
blocks and lower/upper bounds.
Below are some timing results on a big el7 machine. The "real" input was a
representative (i.e. mostly compacted) 40GB tablet:
- NaiveMergeIterator, half-overlapping: 44.7854778767s Counter({'cmp':
25291510, 'peak_blocks_in_mem': 100})
- SingleHeapMergeIterator, half-overlapping: 11.020619154s Counter({'cmp':
10266988, 'peak_blocks_in_mem': 3})
- DoubleHeapMergeIterator, half-overlapping: 3.72211503983s Counter({'cmp':
1178497, 'peak_blocks_in_mem': 3})
- TripleHeapMergeIterator, half-overlapping: 3.52963089943s Counter({'cmp':
1071682, 'peak_blocks_in_mem': 3})
- NaiveMergeIterator, non-overlapping: 44.3896560669s Counter({'cmp': 25958482,
'peak_blocks_in_mem': 100})
- SingleHeapMergeIterator, non-overlapping: 10.9636461735s Counter({'cmp':
10598336, 'peak_blocks_in_mem': 1})
- DoubleHeapMergeIterator, non-overlapping: 2.80402898788s Counter({'cmp':
4021, 'peak_blocks_in_mem': 1})
- TripleHeapMergeIterator, non-overlapping: 2.83524298668s Counter({'cmp':
4021, 'peak_blocks_in_mem': 1})
- NaiveMergeIterator, overlapping: 80.1467709541s Counter({'cmp': 47662665,
'peak_blocks_in_mem': 100})
- SingleHeapMergeIterator, overlapping: 9.61102318764s Counter({'cmp': 8554237,
'peak_blocks_in_mem': 100})
- DoubleHeapMergeIterator, overlapping: 9.68881893158s Counter({'cmp': 8553345,
'peak_blocks_in_mem': 100})
- TripleHeapMergeIterator, overlapping: 9.55243206024s Counter({'cmp': 8563292,
'peak_blocks_in_mem': 100})
- NaiveMergeIterator, real: 1099763.37405s Counter({'cmp': 578660759971,
'peak_blocks_in_mem': 1294})
- SingleHeapMergeIterator, real: 30513.3831122s Counter({'cmp': 30785961774,
'peak_blocks_in_mem': 5})
- DoubleHeapMergeIterator, real: 7987.11197996s Counter({'cmp': 4173739455,
'peak_blocks_in_mem': 15})
- TripleHeapMergeIterator, real: 7155.59520698s Counter({'cmp': 2784969619,
'peak_blocks_in_mem': 5})
Change-Id: I6ae1d2f9e4f41337f475146c648cbab122395f83
Reviewed-on: http://gerrit.cloudera.org:8080/12587
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke
---
A src/kudu/experiments/merge-test.py
1 file changed, 519 insertions(+), 0 deletions(-)
Approvals:
Kudu Jenkins: Verified
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6ae1d2f9e4f41337f475146c648cbab122395f83
Gerrit-Change-Number: 12587
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [mini-sentry] increase timeout for Sentry startup
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12990 ) Change subject: [mini-sentry] increase timeout for Sentry startup .. [mini-sentry] increase timeout for Sentry startup This patch increases timeout for Sentry startup time from 60 to 120 seconds. The motivation for this change is spotting flakiness of some tests from sentry_authz_provider-test where Sentry's JVM was killed with SIGQUIT (generating a core), and also spotting messages like below for the tests that hasn't failed: W0410 23:19:17.437450 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 47.459s user 0.002s sys 0.033s W0410 23:20:30.217487 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 60.443s user 0.000s sys 0.042s W0410 23:21:42.836758 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 60.369s user 0.002s sys 0.042s W0410 23:22:37.729540 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 52.703s user 0.000s sys 0.035s W0410 23:19:26.633388 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 62.565s user 0.002s sys 0.048s W0410 23:20:13.020031 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 44.958s user 0.008s sys 0.030s W0410 23:21:08.531953 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 47.127s user 0.001s sys 0.038s W0410 23:22:18.332482 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 58.223s user 0.000s sys 0.043s Change-Id: I555fb017087c6804c47a0c4bcc40d3ae0ee6006f Reviewed-on: http://gerrit.cloudera.org:8080/12990 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M src/kudu/sentry/mini_sentry.cc 1 file changed, 5 insertions(+), 5 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I555fb017087c6804c47a0c4bcc40d3ae0ee6006f Gerrit-Change-Number: 12990 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [mini-sentry] increase timeout for Sentry startup
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12990 ) Change subject: [mini-sentry] increase timeout for Sentry startup .. Patch Set 1: Code-Review+2 Interesting, I wonder if Hao has any insight into why it might be taking so long. -- To view, visit http://gerrit.cloudera.org:8080/12990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I555fb017087c6804c47a0c4bcc40d3ae0ee6006f Gerrit-Change-Number: 12990 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 11 Apr 2019 02:26:24 + Gerrit-HasComments: No
[kudu-CR] [mini-sentry] increase timeout for Sentry startup
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12990 Change subject: [mini-sentry] increase timeout for Sentry startup .. [mini-sentry] increase timeout for Sentry startup This patch increases timeout for Sentry startup time from 60 to 120 seconds. The motivation for this change is spotting flakiness of some tests from sentry_authz_provider-test where Sentry's JVM was killed with SIGQUIT (generating a core), and also spotting messages like below for the tests that hasn't failed: W0410 23:19:17.437450 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 47.459s user 0.002s sys 0.033s W0410 23:20:30.217487 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 60.443s user 0.000s sys 0.042s W0410 23:21:42.836758 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 60.369s user 0.002s sys 0.042s W0410 23:22:37.729540 7615 mini_sentry.cc:85] Time spent Starting Sentry: real 52.703s user 0.000s sys 0.035s W0410 23:19:26.633388 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 62.565s user 0.002s sys 0.048s W0410 23:20:13.020031 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 44.958s user 0.008s sys 0.030s W0410 23:21:08.531953 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 47.127s user 0.001s sys 0.038s W0410 23:22:18.332482 32082 mini_sentry.cc:85] Time spent Starting Sentry: real 58.223s user 0.000s sys 0.043s Change-Id: I555fb017087c6804c47a0c4bcc40d3ae0ee6006f --- M src/kudu/sentry/mini_sentry.cc 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/12990/1 -- To view, visit http://gerrit.cloudera.org:8080/12990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I555fb017087c6804c47a0c4bcc40d3ae0ee6006f Gerrit-Change-Number: 12990 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] tool: fixes for kudu local replica dump rowset
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12976 ) Change subject: tool: fixes for kudu local_replica dump rowset .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12976 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib50ab4e7b2aa0fec60ce0718d16823945a05cb7f Gerrit-Change-Number: 12976 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 11 Apr 2019 01:23:45 + Gerrit-HasComments: No
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. [SentryPrivilegesFetcher] deduplicate RPCs to Sentry With this patch, concurrent requests to Sentry with the same set of parameters are queued, so the actual number of RPC requests sent to Sentry is reduced. Added a couple of tests to cover the newly added functionality. Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Reviewed-on: http://gerrit.cloudera.org:8080/12967 Tested-by: Alexey Serbin Reviewed-by: Hao Hao Reviewed-by: Andrew Wong --- M src/kudu/master/CMakeLists.txt M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 4 files changed, 197 insertions(+), 20 deletions(-) Approvals: Alexey Serbin: Verified Hao Hao: Looks good to me, approved Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 5 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] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc@355
PS4, Line 355: if (VLOG_IS_ON(1)) {
: std::ostringstream os;
: privilege_resp.printTo(os);
: if (action != SentryAction::ALL && action !=
SentryAction::OWNER &&
: privilege_resp.grantOption ==
TSentryGrantOption::ENABLED) {
: VLOG(1) << "ignoring ENABLED grant option for unexpected
action: "
: << static_cast(action);
: }
> Whoops, just noticed this upon looking at the 3->4 interdiff, this is alway
Good catch! I'll take care of that in a follow-up patch to address Hao's
request to add an extra scenario for the concurrent requests.
I thought the compiler would issue a warning, but may be we have those
particular warnings disabled.
--
To view, visit http://gerrit.cloudera.org:8080/12967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef
Gerrit-Change-Number: 12967
Gerrit-PatchSet: 4
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, 11 Apr 2019 01:15:53 +
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
..
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12967/4/src/kudu/master/sentry_privileges_fetcher.cc@355
PS4, Line 355: if (VLOG_IS_ON(1)) {
: std::ostringstream os;
: privilege_resp.printTo(os);
: if (action != SentryAction::ALL && action !=
SentryAction::OWNER &&
: privilege_resp.grantOption ==
TSentryGrantOption::ENABLED) {
: VLOG(1) << "ignoring ENABLED grant option for unexpected
action: "
: << static_cast(action);
: }
Whoops, just noticed this upon looking at the 3->4 interdiff, this is always
filling an ostringstream and not doing anything with it. Should probably be:
if (VLOG_IS_ON(1) &&
action != SentryAction::ALL && action != SentryAction::OWNER &&
privilege_resp.grantOption == TSentryGrantOption::ENABLED) {
VLOG(1) << "ignoring enabled grant option for action: " <<
ActionToString(action);
}
or just get rid of it; having a grant option for a non-ALL/OWNER action isn't
interesting to know about.
Feel free to do this separately though, since it's not part of this patch.
--
To view, visit http://gerrit.cloudera.org:8080/12967
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef
Gerrit-Change-Number: 12967
Gerrit-PatchSet: 4
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, 11 Apr 2019 01:11:56 +
Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@1010 PS3, Line 1010: est scenario wit > Sure, but I'd like to add it in a separate changelist, otherwise chances of Sure, LGTM. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 4 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, 11 Apr 2019 00:17:31 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Add maximum tablet size recommendations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12979 ) Change subject: [docs] Add maximum tablet size recommendations .. Patch Set 1: > although it would be useful to link users to relevant Jiras The main thing I do not want to indicate is that the jiras I list are the "only" issues, given my examples are not exhaustive because we don't know all the issues or what the next limit would be. Additionally, as we learn more issues I don't want to feel required to update this page since it's meant more for users than contributors and it is only published periodically. Perhaps we can meet in the middle and link to a label in jira that can be updated dynamically as more or less details are found. That work could be done as a follow up change given many of the items on the list could be tied to a list of jiras for "known" issues. -- To view, visit http://gerrit.cloudera.org:8080/12979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9fe48e483334812795d124ee63a3af92f39135de Gerrit-Change-Number: 12979 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 23:52:41 + Gerrit-HasComments: No
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 4: Verified+1 unrelated flake in RemoteKsckTest.TestClusterWithLocation -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 4 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: Wed, 10 Apr 2019 23:45:19 + Gerrit-HasComments: No
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has removed a vote on this change. Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 4 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] [master] introduce SentryPrivilegesFetcher
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12833 ) Change subject: [master] introduce SentryPrivilegesFetcher .. [master] introduce SentryPrivilegesFetcher This patch incorporates a TTL-based cache into the data paths of SentryAuthzProvider by introducing a new entity responsible for fetching and caching the information received from Sentry authz authority: SentryPrivilegesFetcher. A cache's entry contains sanitized and transformed information received as TListSentryPrivilegesResponse from Sentry. Set the newly introduced `--sentry_authz_cache_capacity_mb` command-line flag to 0 to disable caching of authz privilege information returned from Sentry. Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Reviewed-on: http://gerrit.cloudera.org:8080/12833 Reviewed-by: Hao Hao Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/master/CMakeLists.txt 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 M src/kudu/sentry/sentry_authorizable_scope.h 12 files changed, 1,232 insertions(+), 642 deletions(-) Approvals: Hao Hao: Looks good to me, approved Andrew Wong: Looks good to me, approved Kudu Jenkins: Verified -- 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: merged Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Gerrit-Change-Number: 12833 Gerrit-PatchSet: 14 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] 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 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87 PS11, Line 87: sentry::SentryActionsSet granted_privileges; > Your point is valid: granted_actions suggests that actions may be "granted" Thank you for the follow-up comment! Yep, I think so as well. After clarifying on the terminology and coming to better understanding of each other and the matter behind this bike-shedding exercise, we eventually converged on 'allowed_actions' as the name for this field: 'granted_privileges' --> 'allowed_actions'. -- 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: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 23:26:08 + Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
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/12967 to look at the new patch set (#4). Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. [SentryPrivilegesFetcher] deduplicate RPCs to Sentry With this patch, concurrent requests to Sentry with the same set of parameters are queued, so the actual number of RPC requests sent to Sentry is reduced. Added a couple of tests to cover the newly added functionality. Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef --- M src/kudu/master/CMakeLists.txt M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 4 files changed, 197 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12967/4 -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 4 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] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc@906 PS1, Line 906: SERT_EQ(2, GetReconnectionsSucceeded()); : : // NotAuthorized() from Sentry itself considered as a fatal error. : // TODO(KUDU-2769): clarify whether it is a bug in Ha > Ah I see. Did you loop this with stress to arrive at kNumThreads/2 ? Nope, I just put that as some 'reasonable enough' criteria. But I did run this scenario in loop mode with --stress-cpu-threads=16 and verified that it passes with no issues for TSAN builds (~256 runs). http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@998 PS3, Line 998: questThreads / 2) of actual RPC request > Can you explain a bit more on what kind of scheduling anomalies can happen? Done http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@1010 PS3, Line 1010: est scenario wit > Can you add a test case when the Sentry server is not reachable? Sure, but I'd like to add it in a separate changelist, otherwise chances of conflicts are increasing, and I spent considerable amount of time resolving those already. Will it be OK with you if I add that in a follow-up changelist? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h@237 PS3, Line 237: > Remove? Done http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc@154 PS3, Line 154: using std::string; > nit: this should be in L153? Indeed; re-ordered. -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 4 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: Wed, 10 Apr 2019 22:57:51 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 22:57:20 + Gerrit-HasComments: No
[kudu-CR] [docs] Add maximum tablet size recommendations
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12979 ) Change subject: [docs] Add maximum tablet size recommendations .. Patch Set 1: Code-Review+1 The guardrail numbers look good to me, although it would be useful to link users to relevant Jiras tracking the underlying limitations for the following reasons: 1. contributors who want to help address the issues will know where to look; 2. we will remember the reasons that we decided on these numbers in the future. -- To view, visit http://gerrit.cloudera.org:8080/12979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9fe48e483334812795d124ee63a3af92f39135de Gerrit-Change-Number: 12979 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 22:56:29 + Gerrit-HasComments: No
[kudu-CR] [master] introduce SentryPrivilegesFetcher
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 ) Change subject: [master] introduce SentryPrivilegesFetcher .. Patch Set 13: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@78 PS9, Line 78: s = privileges_branch.privileges(); > > But I think this is confusing because this means that "privilege" would r :) Indeed, SGTM -- 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: 13 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: Wed, 10 Apr 2019 22:45:26 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: > Grant and I talked about this offline. SGTM, thanks for the clarification. -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 22:34:12 + Gerrit-HasComments: Yes
[kudu-CR] [master] introduce SentryPrivilegesFetcher
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 ) Change subject: [master] introduce SentryPrivilegesFetcher .. Patch Set 13: Code-Review+2 -- 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: 13 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: Wed, 10 Apr 2019 22:32:54 + Gerrit-HasComments: No
[kudu-CR] [master] introduce 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 (#13). Change subject: [master] introduce SentryPrivilegesFetcher .. [master] introduce SentryPrivilegesFetcher This patch incorporates a TTL-based cache into the data paths of SentryAuthzProvider by introducing a new entity responsible for fetching and caching the information received from Sentry authz authority: SentryPrivilegesFetcher. A cache's entry contains sanitized and transformed information received as TListSentryPrivilegesResponse from Sentry. Set the newly introduced `--sentry_authz_cache_capacity_mb` command-line flag to 0 to disable caching of authz privilege information returned from Sentry. Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe --- M src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/master/CMakeLists.txt 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 M src/kudu/sentry/sentry_authorizable_scope.h 12 files changed, 1,232 insertions(+), 642 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/12833/13 -- 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: 13 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] [master] introduce SentryPrivilegesFetcher
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12833 )
Change subject: [master] introduce SentryPrivilegesFetcher
..
Patch Set 13:
(10 comments)
> Patch Set 12: Code-Review+1
>
> (6 comments)
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@182
PS9, Line 182: rivileges_cach
> Ah, missed the fact that it'd be recursive naming.
Yep, the function look-up rules in C++ turned to be non-intuitive in this
particular case. I initially wrote that without namespace specifiers and got
that compiler error.
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426
PS9, Line 426: for (const auto& action :
SelectRandomSubset(
: kAllActions, 0, )) {
:
> Sorry for the conflicts. Thanks for being accommodating.
np, thanks for putting up a path to address that feedback. It seems I kept
this patch around for too long, so I should have expected something like that
anyways :)
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider-test.cc@697
PS12, Line 697: caching
> nit: Cache?
Done
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@78
PS9, Line 78: s = privileges_branch.privileges();
> But I think this is confusing because this means that "privilege" would refer
> to both the struct itself and a member internal to the struct. And so a clear
> way to distinguish these two "privileges" is to call the member "privilege"
> "action" instead.
Continuing this bike-shedding venue, I could propose to rename
AuthorizablePrivileges into AuthorizableAccessInfo or AuthorizableAccessRules.
However, I think we need this patch to land eventually, so I hope renaming this
into 'allowed_action' will be a reasonable solution :)
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@182
PS9, Line 182:
> SGTM
Great, thanks for being open for this alternative naming approach.
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc@50
PS12, Line 50: // action ('required_action') in the specified scope
('required_scope')
> nit: missing '
Done
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.h@194
PS12, Line 194: Send
> nit: Sends.
Done
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.h@201
PS12, Line 201: Reset
> nit: Resets
Done
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@100
PS9, Line 100: std::string table_name;
> Thanks for doing that Alexey.
np :) probably we will come up with something better in next revisions while
iterating on the signatures of methods used as interface between
SentryAuthzProvider and SentryAuthzFetcher.
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.cc@219
PS12, Line 219: i
> Nit: got two spaces here.
Done
--
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: 13
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: Wed, 10 Apr 2019 22:20:31 +
Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Grant Henke has removed a vote on this change. Change subject: [docs] Adjust latency and bandwidth limitations .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 21:44:21 + Gerrit-HasComments: No
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Hello Brian McDevitt, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12949 to look at the new patch set (#4). Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. Changed the add and set method return types from void to PartialRow so that the method calls can be chained. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 132 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12949/4 -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Hello Brian McDevitt, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12949 to look at the new patch set (#3). Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. -Changed PartialRow return types from void to PartialRow so that the method calls can be chained. -Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. -Removed redundant indentation. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 132 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12949/3 -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12949 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12949/3//COMMIT_MSG Commit Message: PS3: Some of the lines in this commit message are too long. Please wrap lines at 72, 76, or 80 characters (your choice). http://gerrit.cloudera.org:8080/#/c/12949/3//COMMIT_MSG@10 PS3, Line 10: -Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. : -Removed redundant indentation. These two reflect changes made to your original patch in response to code review, right? You don't need to list them here; ultimately this commit message is going to be merged into the source repository as-is and anyone looking at this commit will only see the change it made, not all of its intermediate states in code review. -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Apr 2019 20:38:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
Hello Brian McDevitt, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12949 to look at the new patch set (#2). Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. KUDU-2689: Made PartialRow setters use a fluent-style. -Changed PartialRow return types from void to PartialRow so that the method calls can be chained. -Changed '@return a PartialRow' to '@return this PartialRow' to clearly indicate that it's the current row that is being mutated rather than a new PartialRow. -Removed redundant indentation. Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 132 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12949/2 -- To view, visit http://gerrit.cloudera.org:8080/12949 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3476fb3c0a28812e2732e6c6268f31a393928206 Gerrit-Change-Number: 12949 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Brian McDevitt Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR](branch-1.9.x) tools: escape brackets when generating XML
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12988 ) Change subject: tools: escape brackets when generating XML .. tools: escape brackets when generating XML When building the site, we generate the XML for tools. Usually we escape ambiguous characters for tooling arguments; it seems that wasn't the case for arguments that are GFlags. This meant that before, we would run into errors like the following when generating the site (new in 1.9.0 for the --predicates argument of the new `table scan` tool): /kudu/build/release/gen-docs/kudu.xml:49: parser error : StartTag: invalid element name * The 'Comparison' type supports <=, <, =, >, and >=, ^ Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Reviewed-on: http://gerrit.cloudera.org:8080/12981 Reviewed-by: Adar Dembo Reviewed-by: Grant Henke Tested-by: Andrew Wong (cherry picked from commit 4eb74ae233fadf5184632f4b291b0befeb371dd5) Reviewed-on: http://gerrit.cloudera.org:8080/12988 Tested-by: Kudu Jenkins --- M src/kudu/tools/tool_action.cc 1 file changed, 3 insertions(+), 2 deletions(-) Approvals: Grant Henke: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: merged Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12988 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@998 PS3, Line 998: kNumRequestThreads / 2, sentry_rpcs_num Can you explain a bit more on what kind of scheduling anomalies can happen? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_authz_provider-test.cc@1010 PS3, Line 1010: FailureResponses Can you add a test case when the Sentry server is not reachable? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.h@237 PS3, Line 237: //std::unordered_map pending_requests_; Remove? http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12967/3/src/kudu/master/sentry_privileges_fetcher.cc@154 PS3, Line 154: using std::shared_ptr; nit: this should be in L153? -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 3 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: Wed, 10 Apr 2019 19:29:36 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 19:05:14 + Gerrit-HasComments: No
[kudu-CR] KUDU-2689: Made PartialRow setters use a fluent-style.
raym...@phdata.io has abandoned this change. ( http://gerrit.cloudera.org:8080/12952 ) Change subject: KUDU-2689: Made PartialRow setters use a fluent-style. .. Abandoned Abandoning redundant change. -- To view, visit http://gerrit.cloudera.org:8080/12952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I87b26e775075a0ce16da925df74240fef305f47a Gerrit-Change-Number: 12952 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] experiments: merge iterator optimization tests
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12587 ) Change subject: experiments: merge iterator optimization tests .. Patch Set 6: Code-Review+2 Reviewing with a +2 without a thorough review given the primary value of this is for "archival" reference and a starting point for any future work that is similar. I don't think full fledged review is required. -- To view, visit http://gerrit.cloudera.org:8080/12587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ae1d2f9e4f41337f475146c648cbab122395f83 Gerrit-Change-Number: 12587 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 10 Apr 2019 18:55:32 + Gerrit-HasComments: No
[kudu-CR](branch-1.9.x) tools: escape brackets when generating XML
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12988 ) Change subject: tools: escape brackets when generating XML .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.9.x Gerrit-MessageType: comment Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12988 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:55:53 + Gerrit-HasComments: No
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: > Yep, I agree with Grant that mentioning multi-AZ/DC implicitly endorses the Grant and I talked about this offline. I'm trying to walk a fine line between being sensitive to the ongoing multi-AZ/DC debate while still offering some useful context. I think briefly mentioning "different locations" (where 'location' is exactly the noun exposed by new the location awareness feature) does just that. It doesn't imply multi-AZ or multi-DC. In fact, you could argue that measuring latency and bandwidth between different _racks_ is a useful thing to do; this calls that out without muddying the waters with multi-AZ or multi-DC. -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:55:26 + Gerrit-HasComments: Yes
[kudu-CR] tools: escape brackets when generating XML
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12981 ) Change subject: tools: escape brackets when generating XML .. tools: escape brackets when generating XML When building the site, we generate the XML for tools. Usually we escape ambiguous characters for tooling arguments; it seems that wasn't the case for arguments that are GFlags. This meant that before, we would run into errors like the following when generating the site (new in 1.9.0 for the --predicates argument of the new `table scan` tool): /kudu/build/release/gen-docs/kudu.xml:49: parser error : StartTag: invalid element name * The 'Comparison' type supports <=, <, =, >, and >=, ^ Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Reviewed-on: http://gerrit.cloudera.org:8080/12981 Reviewed-by: Adar Dembo Reviewed-by: Grant Henke Tested-by: Andrew Wong --- M src/kudu/tools/tool_action.cc 1 file changed, 3 insertions(+), 2 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Grant Henke: Looks good to me, approved Andrew Wong: Verified -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] tools: escape brackets when generating XML
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12981 ) Change subject: tools: escape brackets when generating XML .. Patch Set 3: Verified+1 Failure seems unrelated. -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:54:16 + Gerrit-HasComments: No
[kudu-CR] tools: escape brackets when generating XML
Andrew Wong has removed a vote on this change. Change subject: tools: escape brackets when generating XML .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] tools: escape brackets when generating XML
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12981 ) Change subject: tools: escape brackets when generating XML .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:53:51 + Gerrit-HasComments: No
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: > Yep, I agree with Grant that mentioning multi-AZ/DC implicitly endorses the Update to use the word "location" and encourage proactive testing based on Adar's feedback. That is generic enough it should be okay. -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:52:31 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Hello Will Berkeley, Mike Percy, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12970 to look at the new patch set (#3). Change subject: [docs] Adjust latency and bandwidth limitations .. [docs] Adjust latency and bandwidth limitations Adjusts the documentation for latency and bandwidth limitations to be a bit more conservative. Additionally, removes any reference to DC/AZ deployments given we are primarily concerned about latency and bandwidth as the concrete limitations. Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 --- M docs/known_issues.adoc 1 file changed, 6 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/12970/3 -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 2: Code-Review+2 (1 comment) This looks good to me, but maybe Adar or Will might have some additional thoughts/ideas. Probably, it's worth to wait for their approval as well before submitting this patch. http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: > I removed any reference to multi AZ/DC because I didn't want to indicate or Yep, I agree with Grant that mentioning multi-AZ/DC implicitly endorses the feature that we should play more with before endorsing. Also, if mentioning multi-AZ/DC, probably that would entail more details on how to make the deployment HA-hardened: how many locations to define, how to distribute masters and tablet servers among locations, etc. I think at this point the purpose of providing this information is more about warning users on very basic restrictions we are aware of once the note of non-supported multi-AZ/multi-DC deployments has been removed from this document. -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:49:18 + Gerrit-HasComments: Yes
[kudu-CR] tools: escape brackets when generating XML
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12981 ) Change subject: tools: escape brackets when generating XML .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12981/2/src/kudu/tools/tool_action.cc File src/kudu/tools/tool_action.cc: http://gerrit.cloudera.org:8080/#/c/12981/2/src/kudu/tools/tool_action.cc@388 PS2, Line 388: EscapeForHtmlToString(gflag_info.default_value)); > Should probably escape this too, since the default value for a string can h Done -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:29:40 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: > Basically, anyone using the location awareness feature to deploy a cross-lo I removed any reference to multi AZ/DC because I didn't want to indicate or endorse support for that feature. My original patches and suggested changes included that context but it opened long and detailed discussion about remaining work to encourage deployments like that. For that reason, this patch contains only concrete quantifiable limitations based on existing knowledge/deployments to warn/guide anyone who wants a more "creative" deployment in the future. -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:34:49 + Gerrit-HasComments: Yes
[kudu-CR] tools: escape brackets when generating XML
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12981 ) Change subject: tools: escape brackets when generating XML .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:31:30 + Gerrit-HasComments: No
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: > What additional context are you thinking specifically? I left out multi-AZ/ Basically, anyone using the location awareness feature to deploy a cross-location cluster should be aware of this, and maybe make an effort to measure latency/bandwidth between locations before deploying. -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:30:55 + Gerrit-HasComments: Yes
[kudu-CR] tools: escape brackets when generating XML
Hello Will Berkeley, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12981 to look at the new patch set (#3). Change subject: tools: escape brackets when generating XML .. tools: escape brackets when generating XML When building the site, we generate the XML for tools. Usually we escape ambiguous characters for tooling arguments; it seems that wasn't the case for arguments that are GFlags. This meant that before, we would run into errors like the following when generating the site (new in 1.9.0 for the --predicates argument of the new `table scan` tool): /kudu/build/release/gen-docs/kudu.xml:49: parser error : StartTag: invalid element name * The 'Comparison' type supports <=, <, =, >, and >=, ^ Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 --- M src/kudu/tools/tool_action.cc 1 file changed, 3 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/12981/3 -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 3: Code-Review+1 (1 comment) LGTM, just one question. http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc@906 PS1, Line 906: SERT_EQ(2, GetReconnectionsSucceeded()); : : // NotAuthorized() from Sentry itself considered as a fatal error. : // TODO(KUDU-2769): clarify whether it is a bug in Ha > Actually, in both cases it should be ASSERT_GE(...). The statement about g Ah I see. Did you loop this with stress to arrive at kNumThreads/2 ? -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 3 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: Wed, 10 Apr 2019 18:27:56 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:16:37 + Gerrit-HasComments: No
[kudu-CR] [docs] Remove flume kerberos limitation
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12978 ) Change subject: [docs] Remove flume kerberos limitation .. [docs] Remove flume kerberos limitation Removes the flume kerberos limitation as it was resolved via KUDU-2012. Change-Id: Ied4704fc16b18fe0ee93570c3cbbbf6f61da8a4a Reviewed-on: http://gerrit.cloudera.org:8080/12978 Reviewed-by: Mike Percy Tested-by: Grant Henke --- M docs/known_issues.adoc 1 file changed, 0 insertions(+), 3 deletions(-) Approvals: Mike Percy: Looks good to me, approved Grant Henke: Verified -- To view, visit http://gerrit.cloudera.org:8080/12978 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ied4704fc16b18fe0ee93570c3cbbbf6f61da8a4a Gerrit-Change-Number: 12978 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] [docs] Remove flume kerberos limitation
Grant Henke has removed a vote on this change. Change subject: [docs] Remove flume kerberos limitation .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12978 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Ied4704fc16b18fe0ee93570c3cbbbf6f61da8a4a Gerrit-Change-Number: 12978 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] [docs] Remove flume kerberos limitation
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12978 ) Change subject: [docs] Remove flume kerberos limitation .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12978 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied4704fc16b18fe0ee93570c3cbbbf6f61da8a4a Gerrit-Change-Number: 12978 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 10 Apr 2019 18:16:11 + Gerrit-HasComments: No
[kudu-CR] [docs] Add maximum tablet size recommendations
Grant Henke has removed a vote on this change. Change subject: [docs] Add maximum tablet size recommendations .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I9fe48e483334812795d124ee63a3af92f39135de Gerrit-Change-Number: 12979 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12970/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12970/1//COMMIT_MSG@10 PS1, Line 10: Additionaly > Additionally Done http://gerrit.cloudera.org:8080/#/c/12970/1//COMMIT_MSG@12 PS1, Line 12: concered > concerned Done -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:15:13 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Hello Will Berkeley, Mike Percy, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12970 to look at the new patch set (#2). Change subject: [docs] Adjust latency and bandwidth limitations .. [docs] Adjust latency and bandwidth limitations Adjusts the documentation for latency and bandwidth limitations to be a bit more conservative. Additionally, removes any reference to DC/AZ deployments given we are primarily concerned about latency and bandwidth as the concrete limitations. Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 --- M docs/known_issues.adoc 1 file changed, 2 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/12970/2 -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: > Should we provide at least some context for this? I mean, it's useful advic What additional context are you thinking specifically? I left out multi-AZ/DC because it's not clear we want to encourage or discourage certain deployment types. -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 18:13:57 + Gerrit-HasComments: Yes
[kudu-CR] [master] introduce SentryPrivilegesFetcher
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12833 )
Change subject: [master] introduce SentryPrivilegesFetcher
..
Patch Set 12: Code-Review+1
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@182
PS9, Line 182: rivileges_cach
> Nope, that's left for a purpose: if dropping that here and below, compilati
Ah, missed the fact that it'd be recursive naming.
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426
PS9, Line 426: for (const auto& action :
SelectRandomSubset(
: kAllActions, 0, )) {
:
> All right, it seem it's already taken care of (that bring many conflicts in
Sorry for the conflicts. Thanks for being accommodating.
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@78
PS9, Line 78: s = privileges_branch.privileges();
> Yeah, just another bikeshade-able point of this changelist :) It seems I s
I think I understand where you're coming from and I agree that there is no
contention that privileges are units that can be granted from a Sentry POV.
That said, I think if we don't make the conscious decision to clarify different
terms, it might be confusing.
In Sentry, you can have "a privilege on a scope" (per those docs), but what
this really means is "a privilege to perform an action at a given scope". I
don't think those docs do a great job making this distinction because an
end-user of Sentry doesn't really need to make that distinction -- the
"privilege" _is_ the action, and you can grant a privilege on a scope.
When it comes down to these code structs, we want our struct to encapsulate (in
terms from the docs) a privilege that has been granted on a scope. A simple way
to do this would to have a GrantedPrivilege be a struct. But
I think this is confusing because this means that "privilege" would refer to
both the struct itself and a member internal to the struct. And so a clear way
to distinguish these two "privileges" is to call the member "privilege"
"action" instead, so a Privilege is a struct. This corresponds
roughly to what's in TSentryPrivilege in sentry/sentry_policy_service.thrift.
In that way, we care about keeping track of the Privileges (ie. the
structs), and a user can be granted many of these Privileges.
And so when I think of "granting a privilege", I think of "granting a
struct", and by extension, a "granted action" in the context of
these Privileges, is the actions on which the privilege has been granted. But
if you're caught up on the "granted" aspect of it, I'd also be fine calling
this "actions".
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@182
PS9, Line 182:
> I moved this away from SentryPrivilegesBranch since I didn't want to bring
SGTM
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc@50
PS12, Line 50: // action ('required_action) in the specified scope
('required_scope')
nit: missing '
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@100
PS9, Line 100: std::string table_name;
> Yep, the newly added SentryPrivilegesBranch::Init() method addresses that.
Thanks for doing that Alexey.
--
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: 12
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: Wed, 10 Apr 2019 18:04:51 +
Gerrit-HasComments: Yes
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12970/1/docs/known_issues.adoc File docs/known_issues.adoc: PS1: Should we provide at least some context for this? I mean, it's useful advice in the abstract, but perhaps we could hint that this should be particularly relevant to users trying to deploy clusters that span multiple locations? -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 17:04:32 + Gerrit-HasComments: Yes
[kudu-CR] tools: escape brackets when generating XML
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12981 ) Change subject: tools: escape brackets when generating XML .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12981/2/src/kudu/tools/tool_action.cc File src/kudu/tools/tool_action.cc: http://gerrit.cloudera.org:8080/#/c/12981/2/src/kudu/tools/tool_action.cc@388 PS2, Line 388: gflag_info.default_value); Should probably escape this too, since the default value for a string can have arbitrary characters in it, including HTML. -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 17:01:50 + Gerrit-HasComments: Yes
[kudu-CR] sentry: sanitize and parse privileges from Sentry
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 ) Change subject: sentry: sanitize and parse privileges from Sentry .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87 PS11, Line 87: sentry::SentryActionsSet granted_privileges; > This seems to be the most bike-shadeable point in this changelist :) Your point is valid: granted_actions suggests that actions may be "granted", which, as you pointed out, isn't something that makes sense within the Sentry authz model. Agreed that 'actions' would be clearer (or at least less confusing). But, I don't feel strongly about it; I know Andrew preferred to keep the current name, so please do that if it's your preference. I just wanted to make sure we had a mutual understanding of the appropriate terminology, and I think we do. -- 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: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 16:53:10 + Gerrit-HasComments: Yes
[kudu-CR] [master] introduce SentryPrivilegesFetcher
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 ) Change subject: [master] introduce SentryPrivilegesFetcher .. Patch Set 12: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_privileges_fetcher.cc@219 PS12, Line 219: Nit: got two spaces here. -- 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: 12 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: Wed, 10 Apr 2019 16:49:01 + Gerrit-HasComments: Yes
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12967 ) Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 3 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: Wed, 10 Apr 2019 16:43:30 + Gerrit-HasComments: No
[kudu-CR] [docs] Adjust latency and bandwidth limitations
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12970 ) Change subject: [docs] Adjust latency and bandwidth limitations .. Patch Set 1: (2 comments) Thanks for putting up this update! http://gerrit.cloudera.org:8080/#/c/12970/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12970/1//COMMIT_MSG@10 PS1, Line 10: Additionaly Additionally http://gerrit.cloudera.org:8080/#/c/12970/1//COMMIT_MSG@12 PS1, Line 12: concered concerned -- To view, visit http://gerrit.cloudera.org:8080/12970 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42fe8a65779c3f5ad366403366e534fa28713a76 Gerrit-Change-Number: 12970 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 16:23:04 + Gerrit-HasComments: Yes
[kudu-CR] [master] introduce SentryPrivilegesFetcher
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12833 )
Change subject: [master] introduce SentryPrivilegesFetcher
..
Patch Set 11:
(31 comments)
http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@7
PS11, Line 7: WIP
> This is no longer a WIP?
Done
http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@10
PS11, Line 10: raw responses
: received from Sentry
> We actually store sanitized response from Sentry (SentryPrivilegesBranch),
Done
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@157
PS9, Line 157: public:
> Omit this?
Done.
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@182
PS9, Line 182: y() {
> Drop, here and below
Nope, that's left for a purpose: if dropping that here and below, compilation
fails (at least on macOS):
/Users/aserbin/Projects/kudu/src/kudu/master/sentry_authz_provider-test.cc:215:19:
error: too many arguments to function call, expected 0, have 2; did you mean
'::kudu::master::DropRole'?
RETURN_NOT_OK(DropRole(sentry_client_.get(), kRoleName));
^~~~
::kudu::master::DropRole
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426
PS9, Line 426: SentryPrivilegesBranch privileges_info;
:
ASSERT_OK(sentry_authz_provider_->fetcher_.GetSentryPrivileges(
:
> I agree with limiting the test matrix, OTOH if we're limiting it, we might
All right, it seem it's already taken care of (that bring many conflicts into
this patch, BTW):
https://github.com/apache/kudu/commit/16dc218b5a26fa18dee9e36e09e86c9c38f0d55c
Also, since this test doesn't depend much on the caching/non-caching behavior
of SentryPrivilegesFetcher, I'm leaving it in 'caching enabled' mode.
http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h@98
PS11, Line 98: GetSentryPrivileges
> nit: it looks like the comment is removed?
This method is moved SentryAuthzProvider's interface, yes.
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.h@57
PS9, Line 57: privielge
> privilege
Done
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.h@98
PS9, Line 98: Status GetSentryPrivileges(sentry::SentryAuthorizableScope::Scope
scope,
> doc
Done
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@78
PS9, Line 78: bleScope(privilege.scope).Implies(scope)) {
> I strongly feel that we should keep granted_action. Even if an action, with
Yeah, just another bikeshade-able point of this changelist :) It seems I
started it a few revisions ago with a request to change naming of one field in
SentryPrivilegesBranch and now it continues on.
After some consideration I realized that I'm not sure what 'granted action'
means. How do you grant an action? I think that semantically 'action' in
English is not something that can be granted. Also, I don't know of any notion
of 'granting an action', and I could not find any notion of 'granted action' in
user-facing Sentry's documentation at
https://www.cloudera.com/documentation/enterprise/latest/topics/cm_sg_sentry_service.html
What would 'granted action' mean in your terms? Maybe, there is some
documentation on Sentry that clarifies on 'granting an action', but I could not
find it.
http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@182
PS9, Line 182: RETURN_NOT_OK(fetcher_.GetS
> This reads weirdly ("if" and "do" never follow in an English sentence) and
I moved this away from SentryPrivilegesBranch since I didn't want to bring too
much of the implication logic into the Fetcher.
It seems code conventions often contradict the rules of composition of
sentences in English. In current Kudu codebase you can see names of functions
like IsXxx, and that's the prevaling rules of naming such methods/functions.
That's why I decided to name this function using the same pattern. I think
it's better to have more or less uniform naming rules in the codebase. What do
you think?
http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc
File
[kudu-CR] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12967 )
Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
..
Patch Set 2:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/12967/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12967/1//COMMIT_MSG@7
PS1, Line 7: [SentryAuthzProvider]
> nit: seems like most of the heavy lifting is done in sentry_privileges_fetc
Done
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_authz_provider-test.cc@906
PS1, Line 906: // If cache is not enabled and some threads are delayed with
their call
: // to SentryAuthzProvider up to the point when the first
batch of requests
: // has already been processed, some extra RPC(s) might be
observed.
: EXPECT_GE(kNumRequestThreads / 2, sentry_rpcs_num);
> Hrm, why isn't this EXPECT_EQ(kNumRequestThreads, sentry_rpcs_num)? With ca
Actually, in both cases it should be ASSERT_GE(...). The statement about
guaranteed single RPC request to Sentry in case if the caching is enabled is
wrong -- see Adar's comment and my response to that in PS2.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc@884
PS2, Line 884: vector threads;
> Might find it easier to use a ThreadPool for this, especially since the def
I guess ThreadPool is harder to me than simple std::thread in this context :)
Testing it for various number of threads sounds like a good idea, especially to
cover the case when requests to SentryAuthzProvider come from Kudu's
ThreadPool. I added parameterisation for that.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc@903
PS2, Line 903: // If cache is enabled, it's guaranteed to have a single RPC
sent to Sentry.
> I don't see how this is the case. Couldn't a particularly pathological seri
Ah, right. It seems that I confused current implementation with some other
wild idea I had: using the cache to store information about pending requests
along with the response (if any). But the cache's interface didn't allow for
achieving that way of synchronization, in fact.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_authz_provider-test.cc@947
PS2, Line 947: is not storing
> Nit: does not store
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.h@234
PS2, Line 234: parametes
> parameters
Done
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@437
PS1, Line 437:
> Perhaps call this "fetched_privileges"? That way it's clear everywhere belo
Done
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@440
PS1, Line 440: sentry_client_.Stop();
: }
> How about calling this PendingSentryRequests and 'pending_request'? 'info'
Done. I thin naming the variable as 'pending_request' suffices.
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@446
PS1, Line 446: scope_depth_limit_.reset();
: } else {
: SentryAuthorizableScope deepest_scope;
: RETURN_NOT_OK(SentryAuthorizableScope::FromString(
: scope_level_limit_str, _scope));
: s
> nit: slightly simpler as:
Done
http://gerrit.cloudera.org:8080/#/c/12967/1/src/kudu/master/sentry_privileges_fetcher.cc@481
PS1, Line 481: *privileges = handle.value();
: return Status::OK();
: }
:
> nit: for parity with L453, may be written as:
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@484
PS2, Line 484:
> Should explain why we need to wrap result in shared_ptr.
Done
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@485
PS2, Line 485: Synchronizer sync;
> How many usages of this pattern do we have now? I can think of another one
All right, I'll open a JIRA for that.
http://gerrit.cloudera.org:8080/#/c/12967/2/src/kudu/master/sentry_privileges_fetcher.cc@487
PS2, Line 487: std::shared_ptr result;
[kudu-CR] [master] introduce 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 (#12). Change subject: [master] introduce SentryPrivilegesFetcher .. [master] introduce SentryPrivilegesFetcher This patch incorporates a TTL-based cache into the data paths of SentryAuthzProvider by introducing new entity responsible for fetching and caching the information received from Sentry authz authority. The cache's entry contains sanitized and transformed information received as TListSentryPrivilegesResponse from Sentry. Set the newly introduced `--sentry_authz_cache_capacity_mb` command-line flag to 0 to disable caching of authz privilege information returned from Sentry. Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe --- M src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/master/CMakeLists.txt 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 M src/kudu/sentry/sentry_authorizable_scope.h 12 files changed, 1,224 insertions(+), 632 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/12833/12 -- 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: 12 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] [SentryPrivilegesFetcher] deduplicate RPCs to Sentry
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/12967 to look at the new patch set (#3). Change subject: [SentryPrivilegesFetcher] deduplicate RPCs to Sentry .. [SentryPrivilegesFetcher] deduplicate RPCs to Sentry With this patch, concurrent requests to Sentry with the same set of parameters are queued, so the actual number of RPC requests sent to Sentry is reduced. Added a couple of tests to cover the newly added functionality. Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef --- M src/kudu/master/CMakeLists.txt M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_privileges_fetcher.cc M src/kudu/master/sentry_privileges_fetcher.h 4 files changed, 186 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/12967/3 -- To view, visit http://gerrit.cloudera.org:8080/12967 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2714ab032df6029240ad9e7f4fb03ff73c6b79ef Gerrit-Change-Number: 12967 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] tools: escape brackets when generating XML
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12981 ) Change subject: tools: escape brackets when generating XML .. Patch Set 2: This should be backported to 1.9.x as well. -- To view, visit http://gerrit.cloudera.org:8080/12981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I16c13d86b0b452e0559e245ee33373078e5e3713 Gerrit-Change-Number: 12981 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 10 Apr 2019 06:37:59 + Gerrit-HasComments: No
[kudu-CR] WIP [master] introduced SentryPrivilegesFetcher
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 ) Change subject: WIP [master] introduced SentryPrivilegesFetcher .. Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@7 PS11, Line 7: WIP This is no longer a WIP? http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@10 PS11, Line 10: raw responses : received from Sentry We actually store sanitized response from Sentry (SentryPrivilegesBranch), right? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h@98 PS11, Line 98: GetSentryPrivileges nit: it looks like the comment is removed? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc@44 PS11, Line 44: DoPrivilegesImplyAction Maybe rename it to 'PrivilegeBranchImplies' to state that this is 'Whether the given privilege imply the one from the same branch but with the specified action at the specified scope with 'GRANT ALL' option (if any).' http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc@182 PS11, Line 182: fetcher_.GetSentryPrivileges nit: use SentryAuthzProvider::GetSentryPrivileges? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@92 PS11, Line 92: granted_privileges Not your fault, but I think here name it as granted_actions is more appropriate. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@183 PS11, Line 183: Transform nit: Transforms. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@189 PS11, Line 189: Set nit: Sets, and the below. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@200 PS11, Line 200: test-only method Is this still a test-only method, I saw it has been used in Start()? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@207 PS11, Line 207: Broaden the authz scope Can you comment on how scope_depth_limit_ is used in this method and why it is used like that? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc@224 PS11, Line 224: const boost::optional& scope_limit = boost::none It doesn't seem we are using 'scope_limit' in this patch, and I am not sure if we need it in the further cache optimization patch. If not, should it be removed? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc@233 PS11, Line 233: futher nit: further -- 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: 11 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: Wed, 10 Apr 2019 06:05:23 + Gerrit-HasComments: Yes
