You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/04/18 05:36:58 UTC
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13063
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
WIP [master] RPC knobs to control authz privileges cache
WIP:
* I'd like to collect initial feedback on the approach used here:
** separate RPC service for authz cache control
(not in Master interface)
** assuming that 'real' authz provider will need to cache
information fetched from external privilege authority
to address latency issues talking to external systems
using RPC
** naming (the hardest part, yes)
* clarify on eviction Cache callback's lifecycle: it seems there is
an issue if destructing an instance of Cache when an entry is still
referenced by a handle. The 'eviction callback' is called upon
destruction of an entry, not upon eviction from the cache, actually.
But the eviction callback is a functor that is a member of TTLCache.
I'm thinking of rather invalidating cache entries instead.
* need to add tests
Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/master.cc
A src/kudu/master/privileges_cache.proto
A src/kudu/master/privileges_cache_service.cc
A src/kudu/master/privileges_cache_service.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
13 files changed, 321 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/13063/1
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13063
to look at the new patch set (#6).
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
[master] add RPC to reset AuthzProvider's cache
This changelist adds an RPC end-point to control SentryAuthzProvider's
privileges cache. As of now, it's only possible to reset the cache
of the aggregated SentryPrivilegesFetcher via the newly added
ResetAuthzCache() method.
The method has been added into the master's RPC interface. It's
necessary to have admin/superuser credentials to successfully invoke
the method via Kudu RPC.
Also, added few tests to cover the new functionality.
Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/mini-cluster/external_mini_cluster.h
14 files changed, 354 insertions(+), 63 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/13063/6
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@7
PS1, Line 7: WIP [master] RPC knobs to control authz privileges cache
The simpler "knob" would be to restart the master, right? Is this an important enough use case to mandate dedicated RPCs? Why or why not?
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc@453
PS1, Line 453: handle = cache_->Get(key);
If you give cache_ shared ownership, I think you may be able to avoid the lock altogether by using https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic to load (here) and to exchange (ResetCache()).
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Apr 2019 18:41:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Apr 2019 23:23:23 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@7
PS1, Line 7: WIP [master] RPC knobs to control authz privileges cache
> Restarting a master only means downtime in single-master configurations, wh
We can call it 'lever' instead of 'knob' :) Yep, I'll add a proper description.
As for the restart of masters, it will entail restarting all masters in multi-master configuration eventually if using that sort of 'knob' to clean the cache. As I understand, that's an operational burden. Also, when a master is restarted, all involved clients might need to retry some of their in-flight operations.
I'm not sure that's a better alternative than a single method (or even a few) that we can call from kudu CLI.
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@11
PS1, Line 11: separate RPC service for authz cache control
: (not in Master interface)
> Why was this necessary? Why not use one of the existing master services?
From purist's point of view, it's not a good idea to put unrelated stuff into one interface. From another point, it's not possible to have multiple ServiceIf served by one server in KRPC, sharing the same thread pool.
Andrew and I discussed this offline. The conclusion is that putting that method into master's interface is not such a big deal especially if we don't want to introduce additional thread pools that would serve RPC interface with just a single method.
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h@99
PS1, Line 99: // Authz provider with additional functionality to cache information received
: // from corresponding authoritative source. At this point, it's anticipated
: // that real providers would cache information received from the authz source
: // authority to reduce latency of operations requiring verification of granted
: // privileges. It's presumed that the source of information on granted
: // privileges is an external system.
: class CachingAuthzProvider : public AuthzProvider {
: public:
: // Reset the underlying cache, invalidating all cached entries. Returns
: // Status::NotSupported() if the provider doesn't support resetting its
: // cache.
: virtual Status ResetCache() WARN_UNUSED_RESULT = 0;
: };
:
> Why not just have ResetCache() be a member of AuthzProvider? Also not sure
My understanding is that AuthzProvider interface is a generic interface which contains authz-related only, so I though it's better to keep it pure and unobstructed from other stuff.
In this case, 'real' means providers we user and anticipate using in Kudu. I think it's better to drop 'real' there, just describing why providers may need to cache privilege data.
If it's OK with everybody to have this not exactly authz-like method in the AuthzInterface, I'll add it there.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Apr 2019 22:45:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@223
PS3, Line 223: std::shared_ptr<AuthzInfoCache> cache_;
Should doc why it needs shared ownership.
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@226
PS3, Line 226: // of operations with cache items and concurrent request to reset the cache.
requests (to agree with plural of 'operations')
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@227
PS3, Line 227: // Not using std::atomic_load and std::atomic_exchange since those are:
: // * have higher performance overhead for this particular case
: // * not available while compiling on pre-RH7 platforms, even if using
: // C++11-compatible compiler from thirdparty and devtoolset.
Nit: reword as:
An alternative would be to use std::atomic_load and std::atomic_exchange, but:
* They're higher overhead operations than just using a spinlock.
* They're not available in all C++11-compatible compilers.
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc@453
PS1, Line 453: const auto& db = privilege_resp.dbName;
> I looked at them and there are a couple of things:
Are they really heavier-weight than acquiring a spinlock?
Anyway, unavailability in devtoolset3 is certainly a deal-breaker.
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc@487
PS3, Line 487: ResetCache();
Curious why this call is here and not in the constructor?
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 20:25:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 4: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc@487
PS3, Line 487: // The semantics of SentryAuthzProvider's Start()/Stop() doesn't guarantee
> I tried to follow the pattern for sentry_client. I think we need it here a
Ah, wasn't aware that Start/Stop allows for Sentry RPC address changes. In that case resetting the cache here makes sense, for the reason you provided.
http://gerrit.cloudera.org:8080/#/c/13063/4/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/4/src/kudu/master/sentry_privileges_fetcher.cc@487
PS4, Line 487: doesn't
don't (agreeing with plural of 'semantics')
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 21:31:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13063/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/5//COMMIT_MSG@9
PS5, Line 9: RPC
> Mention it can only used by super user?
Done
http://gerrit.cloudera.org:8080/#/c/13063/5//COMMIT_MSG@9
PS5, Line 9: control SentryAuthzProvider's
: privileges cache
> nit: maybe mention this is specific to clear the privilege cache.
Done
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@200
PS5, Line 200: client_->DeleteTable(table)
> nit: use DropTable below instead?
Done
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@203
PS5, Line 203: DropTable
> nit: is this used somewhere? can DeleteTable be used instead? It is a bit w
It was used in the new code exploiting {Create,Drop}Table(). Since it was failing, I removed that scenario instead of leaving it DISABLED in this changelist.
I'm going to publish the set of disabled tests separately -- that will address one of the comments you left in this review round.
As for the DropTable, I was a bit frustrated that there is GrantDropTablePrivilege() method, but the utility method to drop a table called DeleteTable(), so I added a new method :)
All right, I'll consolidate those and name those 'DropTable'.
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@759
PS5, Line 759: AlterTable
> (I know you mentioned but) maybe add a TODO for all other DDLs like CreateT
CreateTable doesn't work in such a scenario: it fails. I have that in my repo.
I can put those scenarios with the 'DISABLED_' prefix in a separate changelist (will add TODO to enable those). Does it make sense?
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/master/sentry_privileges_fetcher.cc@450
PS5, Line 450: VLOG(1)
> Why remove VLOG_IS_ON?
Because it's not needed: VLOG(1) logs onlt if VLOG_IS_ON(1). In the former version, VLOG_IS_ON() was necessary since there were some conditional operations not gated by the evaluation of VLOG(1) itself.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 26 Apr 2019 04:10:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] add RPC to reset AuthzProvider's cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 2:
(7 comments)
All nits pretty much, but unsure what the status is re: the second WIP bullet.
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@730
PS2, Line 730: Status ResetCache() {
: SCOPED_CLEANUP({
: WARN_NOT_OK(cluster_->kdc()->Kinit(kTestUser),
: "could not restore Kerberos credentials");
: });
nit: add docs about how we're kinitting as the admin first so we can run the reset.
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@830
PS2, Line 830:
: Status ResetCache(const string& user,
nit: docs
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@868
PS2, Line 868: static constexpr const char* const
Wow.
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@869
PS2, Line 869: ASSERT_OK(cluster_->kdc()->Kinit(kUserTestAdmin));
This happens in ResetCache() already, no? If so, let's not define kUserTestAdmin and just call ResetCache on "test-admin" directly.
Same above.
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/master_service.cc@626
PS2, Line 626: const auto s = server_->catalog_manager()->authz_provider()->ResetCache();
: CheckRespErrorOrSetUnknown(s, resp);
nit: could merge these lines?
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h@157
PS2, Line 157: Also, it's used by tests: if the authz information has been updated by the
: // test, the cache needs to be invalidated.
This is true of non-tests too, so maybe omit the detail about tests specifically?
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h@223
PS2, Line 223: rw_spinlock cache_reset_lock_;
nit: cache_reset_lock_ makes it seem like we're only guarding resets of the cache, when really, it seems what's implemented is guarding the cache itself. Maybe rename it cache_lock_?
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 22 Apr 2019 22:36:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 2:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@7
PS1, Line 7: WIP [master] add RPC to reset AuthzProvider's cache
> Yeah, in terms of ergonomics as an operator, having a tool seems desirable.
Ack
http://gerrit.cloudera.org:8080/#/c/13063/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/2//COMMIT_MSG@17
PS2, Line 17: * clarify on eviction Cache callback's lifecycle: it seems there is
: an issue if destructing an instance of Cache when an entry is still
: referenced by a handle. The 'eviction callback' is called upon
: destruction of an entry, not upon eviction from the cache, actually.
: But the eviction callback is a functor that is a member of TTLCache.
: I'm thinking of rather invalidating cache entries instead.
> Right, the cache's destructor will deref all entries and call the eviction
Ack
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@730
PS2, Line 730: Status ResetCache() {
: SCOPED_CLEANUP({
: WARN_NOT_OK(cluster_->kdc()->Kinit(kTestUser),
: "could not restore Kerberos credentials");
: });
> nit: add docs about how we're kinitting as the admin first so we can run th
Done
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@830
PS2, Line 830:
: Status ResetCache(const string& user,
> nit: docs
Done
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@868
PS2, Line 868: static constexpr const char* const
> Wow.
Ack
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/integration-tests/master_sentry-itest.cc@869
PS2, Line 869: ASSERT_OK(cluster_->kdc()->Kinit(kUserTestAdmin));
> This happens in ResetCache() already, no? If so, let's not define kUserTest
Done
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h@99
PS1, Line 99:
: private:
: std::unordered_set<std::string> trusted_users_;
: };
:
: } // namespace master
: } // namespace kudu
:
:
:
:
:
:
:
> If by AuthzInterface you mean in the interface of AuthzProvider, I'm on boa
I moved it into AuthzProvider, as suggested.
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/catalog_manager.cc@762
PS1, Line 762: RETURN_NOT_OK_PREPE
> Good catch: that's not needed at all.
Done
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/master_service.cc@626
PS2, Line 626: const auto s = server_->catalog_manager()->authz_provider()->ResetCache();
: CheckRespErrorOrSetUnknown(s, resp);
> nit: could merge these lines?
Done
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h@157
PS2, Line 157: Also, it's used by tests: if the authz information has been updated by the
: // test, the cache needs to be invalidated.
> This is true of non-tests too, so maybe omit the detail about tests specifi
Done
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h@222
PS2, Line 222: operaitons
> operations
Done
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h@223
PS2, Line 223: rw_spinlock cache_reset_lock_;
> nit: cache_reset_lock_ makes it seem like we're only guarding resets of the
Done
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc@453
PS1, Line 453: handle = cache_->Get(key);
> If you give cache_ shared ownership, I think you may be able to avoid the l
I looked at them and there are a couple of things:
* performance-wise, those are too heavy for this purpose
* those are not available in libstdc++ on pre-RH7 platforms, even if using clang++ 6.0 from the thirdparty and devtoolset-3; below is the snippet of the compiler's output
[ 88%] Building CXX object src/kudu/master/CMakeFiles/master.dir/sentry_privileges_fetcher.cc.o
/data/8/aserbin/Projects/kudu/src/kudu/master/sentry_privileges_fetcher.cc:542:3: error: no matching
function for call to 'atomic_exchange'
atomic_exchange(&cache_, new_cache);
^~~~~~~~~~~~~~~
/opt/rh/devtoolset-3/root/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/atomic:914:5: note:
candidate template ignored: could not match 'atomic' against 'shared_ptr'
atomic_exchange(atomic<_ITp>* __a, _ITp __i) noexcept
^
/opt/rh/devtoolset-3/root/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/atomic:919:5: note:
candidate template ignored: could not match 'atomic' against 'shared_ptr'
atomic_exchange(volatile atomic<_ITp>* __a, _ITp __i) noexcept
^
/data/8/aserbin/Projects/kudu/src/kudu/master/sentry_privileges_fetcher.cc:576:14: error: no
matching function for call to 'atomic_load'
auto cache(atomic_load(&cache_));
^~~~~~~~~~~
/opt/rh/devtoolset-3/root/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/atomic:904:5: note:
candidate template ignored: could not match 'atomic' against 'shared_ptr'
atomic_load(const atomic<_ITp>* __a) noexcept
^
/opt/rh/devtoolset-3/root/usr/lib/gcc/x86_64-redhat-linux/4.9.2/../../../../include/c++/4.9.2/atomic:909:5: note:
candidate template ignored: could not match 'atomic' against 'shared_ptr'
atomic_load(const volatile atomic<_ITp>* __a) noexcept
^
2 errors generated.
make[2]: *** [src/kudu/master/CMakeFiles/master.dir/sentry_privileges_fetcher.cc.o] Error 1
make[1]: *** [src/kudu/master/CMakeFiles/master.dir/all] Error 2
make: *** [all] Error 2
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 18:50:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 26 Apr 2019 21:23:37 +0000
Gerrit-HasComments: No
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 5: Code-Review+1
(6 comments)
Just few nits.
http://gerrit.cloudera.org:8080/#/c/13063/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/5//COMMIT_MSG@9
PS5, Line 9: RPC
Mention it can only used by super user?
http://gerrit.cloudera.org:8080/#/c/13063/5//COMMIT_MSG@9
PS5, Line 9: control SentryAuthzProvider's
: privileges cache
nit: maybe mention this is specific to clear the privilege cache.
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@200
PS5, Line 200: client_->DeleteTable(table)
nit: use DropTable below instead?
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@203
PS5, Line 203: DropTable
nit: is this used somewhere? can DeleteTable be used instead? It is a bit weird to have one method named DropTable and the other as DeleteTable which calls the same thing under the hood.
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@759
PS5, Line 759: AlterTable
(I know you mentioned but) maybe add a TODO for all other DDLs like CreateTable/RenameTable/DropTable for the same kind of test. Also for Concurrent test below.
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/master/sentry_privileges_fetcher.cc@450
PS5, Line 450: VLOG(1)
Why remove VLOG_IS_ON?
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Apr 2019 00:56:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13063
to look at the new patch set (#4).
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
[master] add RPC to reset AuthzProvider's cache
This changelist adds an RPC end-point to control SentryAuthzProvider's
privileges cache. A new ResetAuthzCache() method has been added
into the master's RPC interface. Also, added few tests to cover the new
functionality.
Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/mini-cluster/external_mini_cluster.h
14 files changed, 352 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/13063/4
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13063
to look at the new patch set (#2).
Change subject: WIP [master] add RPC to reset AuthzProvider's cache
......................................................................
WIP [master] add RPC to reset AuthzProvider's cache
This changelist adds an RPC end-point to control SentryAuthzProvider's
privileges cache. A new ResetAuthzCache() method has been added
into the master's RPC interface. Also, added tests to cover the new
functionality.
WIP:
* clarify on strange Status::AlreadyPresent() error while trying
to create a table when Sentry is not available.
* clarify on eviction Cache callback's lifecycle: it seems there is
an issue if destructing an instance of Cache when an entry is still
referenced by a handle. The 'eviction callback' is called upon
destruction of an entry, not upon eviction from the cache, actually.
But the eviction callback is a functor that is a member of TTLCache.
I'm thinking of rather invalidating cache entries instead.
Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/mini-cluster/external_mini_cluster.h
14 files changed, 281 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/13063/2
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc@487
PS3, Line 487: // The semantics of SentryAuthzProvider's Start()/Stop() doesn't guarantee
> Ah, wasn't aware that Start/Stop allows for Sentry RPC address changes. In
Ack
http://gerrit.cloudera.org:8080/#/c/13063/4/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/4/src/kudu/master/sentry_privileges_fetcher.cc@487
PS4, Line 487: doesn't
> don't (agreeing with plural of 'semantics')
whoops.
For some reason, my English teachers taught me that 'economics' should be used in singular, so I'm conflating that with everything else '-ics' and still re-learning.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 21:40:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 2: Verified+1
non-related flake: KUDU-2501
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Apr 2019 16:39:17 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/catalog_manager.cc@762
PS1, Line 762: RETURN_NOT_OK_PREPE
> Looking around, this seems important for initializing on-disk state only wh
Good catch: that's not needed at all.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Apr 2019 06:39:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] add RPC to reset AuthzProvider's cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13063/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/2//COMMIT_MSG@17
PS2, Line 17: * clarify on eviction Cache callback's lifecycle: it seems there is
: an issue if destructing an instance of Cache when an entry is still
: referenced by a handle. The 'eviction callback' is called upon
: destruction of an entry, not upon eviction from the cache, actually.
: But the eviction callback is a functor that is a member of TTLCache.
: I'm thinking of rather invalidating cache entries instead.
Right, the cache's destructor will deref all entries and call the eviction callback on any entries whose refcount is 0, but if there's an entry with an extra refcount (i.e. an outstanding handle), the callback will only be invoked later, when that last handle is destroyed and the refcount drops to 0.
I think this would be addressed by giving the TTLCache shared ownership as I suggested in another comment in this patch. You might also have to store a ref to the TTLCache in its outstanding handles, if the handles are expected to outlive the TTLCache refs held by clients.
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/13063/2/src/kudu/master/sentry_privileges_fetcher.h@222
PS2, Line 222: operaitons
operations
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 21 Apr 2019 20:39:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13063
to look at the new patch set (#3).
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
[master] add RPC to reset AuthzProvider's cache
This changelist adds an RPC end-point to control SentryAuthzProvider's
privileges cache. A new ResetAuthzCache() method has been added
into the master's RPC interface. Also, added few tests to cover the new
functionality.
Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/mini-cluster/external_mini_cluster.h
14 files changed, 345 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/13063/3
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13063
to look at the new patch set (#5).
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
[master] add RPC to reset AuthzProvider's cache
This changelist adds an RPC end-point to control SentryAuthzProvider's
privileges cache. A new ResetAuthzCache() method has been added
into the master's RPC interface. Also, added few tests to cover the new
functionality.
Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/mini-cluster/external_mini_cluster.h
14 files changed, 352 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/13063/5
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@7
PS1, Line 7: WIP [master] RPC knobs to control authz privileges cache
> We can call it 'lever' instead of 'knob' :) Yep, I'll add a proper descrip
Yeah, in terms of ergonomics as an operator, having a tool seems desirable.
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h@99
PS1, Line 99: // Authz provider with additional functionality to cache information received
: // from corresponding authoritative source. At this point, it's anticipated
: // that real providers would cache information received from the authz source
: // authority to reduce latency of operations requiring verification of granted
: // privileges. It's presumed that the source of information on granted
: // privileges is an external system.
: class CachingAuthzProvider : public AuthzProvider {
: public:
: // Reset the underlying cache, invalidating all cached entries. Returns
: // Status::NotSupported() if the provider doesn't support resetting its
: // cache.
: virtual Status ResetCache() WARN_UNUSED_RESULT = 0;
: };
:
> My understanding is that AuthzProvider interface is a generic interface whi
If by AuthzInterface you mean in the interface of AuthzProvider, I'm on board.
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/catalog_manager.cc@762
PS1, Line 762: if (is_first_run) {
Looking around, this seems important for initializing on-disk state only when creating a new server (a better name would probably be is_new_server). Was this change necessary?
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 19 Apr 2019 01:38:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 5: Verified+1
Unrelated tests flakies.
DEBUG:
org.apache.kudu.client.TestKuduScanner
org.apache.kudu.spark.kudu.DefaultSourceTest
RELEASE:
master_failover-itest
The failure of the scenarios in master_failover-itest were due to NTP issues:
E0425 21:47:25.020566 8573 system_ntp.cc:105] /usr/bin/ntpq -n -c timeout 1000 -c readvar -c sysinfo -c lpeers -c opeers -c version
------------------------------------------
stdout:
ntpq 4.2.6p5@1.2349-o Wed Jul 12 12:23:07 UTC 2017 (1)
stderr:
ntpq: read: Connection refused
***Command `sysinfo' unknown
ntpq: read: Connection refused
ntpq: read: Connection refused
E0425 21:47:25.025511 8573 system_ntp.cc:105] /usr/bin/ntpdc -n -c timeout 1000 -c peers -c sysinfo -c sysstats -c version
------------------------------------------
stdout:
ntpdc 4.2.6p5@1.2349-o Wed Jul 12 12:23:02 UTC 2017 (1)
stderr:
ntpdc: read: Connection refused
ntpdc: read: Connection refused
ntpdc: read: Connection refused
W0425 21:47:25.027098 8573 system_ntp.cc:96] could not find executable: chronyc
W0425 21:47:25.028698 8573 system_ntp.cc:96] could not find executable: chronyc
F0425 21:47:25.028751 8573 master_main.cc:80] Check failed: _s.ok() Bad status: Runtime error: Cannot initialize clock: failed to wait for clock sync using command '/usr/sbin/ntp-wait -s 1 -n 60': /usr/sbin/ntp-wait: process exited with non-zero status 1
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 22:05:43 +0000
Gerrit-HasComments: No
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 6: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@203
PS5, Line 203: AlterTabl
> It was used in the new code exploiting {Create,Drop}Table(). Since it was
Thanks for the explanation, LGTM.
http://gerrit.cloudera.org:8080/#/c/13063/5/src/kudu/integration-tests/master_sentry-itest.cc@759
PS5, Line 759: _alterer(
> CreateTable doesn't work in such a scenario: it fails. I have that in my r
LGTM.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 26 Apr 2019 21:04:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
[master] add RPC to reset AuthzProvider's cache
This changelist adds an RPC end-point to control SentryAuthzProvider's
privileges cache. As of now, it's only possible to reset the cache
of the aggregated SentryPrivilegesFetcher via the newly added
ResetAuthzCache() method.
The method has been added into the master's RPC interface. It's
necessary to have admin/superuser credentials to successfully invoke
the method via Kudu RPC.
Also, added few tests to cover the new functionality.
Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Reviewed-on: http://gerrit.cloudera.org:8080/13063
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao <ha...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/master.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/mini-cluster/external_mini_cluster.h
14 files changed, 354 insertions(+), 63 deletions(-)
Approvals:
Kudu Jenkins: Verified
Hao Hao: Looks good to me, approved
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: WIP [master] add RPC to reset AuthzProvider's cache
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master] add RPC to reset AuthzProvider's cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: [master] add RPC to reset AuthzProvider's cache
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h
File src/kudu/master/sentry_privileges_fetcher.h:
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@223
PS3, Line 223: std::shared_ptr<AuthzInfoCache> cache_;
> Should doc why it needs shared ownership.
Done
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@226
PS3, Line 226: // of operations with cache items and concurrent request to reset the cache.
> requests (to agree with plural of 'operations')
Done
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.h@227
PS3, Line 227: // Not using std::atomic_load and std::atomic_exchange since those are:
: // * have higher performance overhead for this particular case
: // * not available while compiling on pre-RH7 platforms, even if using
: // C++11-compatible compiler from thirdparty and devtoolset.
> Nit: reword as:
Done. The reworded version is more concise. Thanks!
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/sentry_privileges_fetcher.cc@453
PS1, Line 453: const auto& db = privilege_resp.dbName;
> Are they really heavier-weight than acquiring a spinlock?
* according to https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic#Notes , those are usually implemented using mutexes
* yep, atomic-related for shared pointers works great with libc++ (at least on macOS), but on CentOS 6.6 there was an error I mentioned earlier
Also, I took a look into thirdparty/src/llvm-6.0.0.src/projects/libcxx/include/memory:
template <class _Tp>
_LIBCPP_AVAILABILITY_ATOMIC_SHARED_PTR
shared_ptr<_Tp>
atomic_load(const shared_ptr<_Tp>* __p)
{
__sp_mut& __m = __get_sp_mut(__p);
__m.lock();
shared_ptr<_Tp> __q = *__p;
__m.unlock();
return __q;
}
Some details on __get_sp_mut from thirdparty/src/llvm-6.0.0.src/projects/libcxx/src/memory.cpp:
__sp_mut&
__get_sp_mut(const void* p)
{
static __sp_mut muts[__sp_mut_count]
{
&mut_back[ 0], &mut_back[ 1], &mut_back[ 2], &mut_back[ 3],
&mut_back[ 4], &mut_back[ 5], &mut_back[ 6], &mut_back[ 7],
&mut_back[ 8], &mut_back[ 9], &mut_back[10], &mut_back[11],
&mut_back[12], &mut_back[13], &mut_back[14], &mut_back[15]
};
return muts[hash<const void*>()(p) & (__sp_mut_count-1)];
}
So, indeed: a global table hash-table consisting of 16 buckets, with mutex in each:
_LIBCPP_SAFE_STATIC static const std::size_t __sp_mut_count = 16;
_LIBCPP_SAFE_STATIC static __libcpp_mutex_t mut_back[__sp_mut_count] =
{
_LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER,
_LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER,
_LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER,
_LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER, _LIBCPP_MUTEX_INITIALIZER
};
From thirdparty/src/llvm-6.0.0.src/projects/libcxx/include/__threading_support:
typedef pthread_mutex_t __libcpp_mutex_t;
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc
File src/kudu/master/sentry_privileges_fetcher.cc:
http://gerrit.cloudera.org:8080/#/c/13063/3/src/kudu/master/sentry_privileges_fetcher.cc@487
PS3, Line 487: ResetCache();
> Curious why this call is here and not in the constructor?
I tried to follow the pattern for sentry_client. I think we need it here anyways (even if adding ResetCache() into the constructor as well) since the semantics of Start/Stop propagated from SentryAuthzProvider assumes Sentry RPC address might change after stop, so it's important to clean the cache of possible irrelevant information.
From the other side, there is not much sense in an empty cache created in the constructor before calling Start().
I'll add a comment.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 21:24:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@7
PS1, Line 7: WIP [master] RPC knobs to control authz privileges cache
> My rationale would be that a master restart means cluster downtime, which m
Restarting a master only means downtime in single-master configurations, which aren't recommended for production use cases. With multiple masters, there's no downtime at all.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Apr 2019 19:12:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
Patch Set 1:
(3 comments)
High level pass so far, so just high level comments.
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@7
PS1, Line 7: WIP [master] RPC knobs to control authz privileges cache
> The simpler "knob" would be to restart the master, right? Is this an import
My rationale would be that a master restart means cluster downtime, which might not be desirable.
Though not having gone through the patch, I thought the only "knob" would be some "reset cache" RPC, which isn't all that knob-like. Would be good to add a summary of what we are tuning in this patch.
http://gerrit.cloudera.org:8080/#/c/13063/1//COMMIT_MSG@11
PS1, Line 11: separate RPC service for authz cache control
: (not in Master interface)
Why was this necessary? Why not use one of the existing master services?
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/13063/1/src/kudu/master/authz_provider.h@99
PS1, Line 99: // Authz provider with additional functionality to cache information received
: // from corresponding authoritative source. At this point, it's anticipated
: // that real providers would cache information received from the authz source
: // authority to reduce latency of operations requiring verification of granted
: // privileges. It's presumed that the source of information on granted
: // privileges is an external system.
: class CachingAuthzProvider : public AuthzProvider {
: public:
: // Reset the underlying cache, invalidating all cached entries. Returns
: // Status::NotSupported() if the provider doesn't support resetting its
: // cache.
: virtual Status ResetCache() WARN_UNUSED_RESULT = 0;
: };
:
Why not just have ResetCache() be a member of AuthzProvider? Also not sure what "real provider" means in this context.
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Apr 2019 19:04:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13063 )
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
Patch Set 1: Verified+1
Unrelated flakes in Java tests & rpc-test
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 18 Apr 2019 15:42:48 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [master] RPC knobs to control authz privileges cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: WIP [master] RPC knobs to control authz privileges cache
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ib9a5c1f84172acf5751f68edfb8a9bb25df6b3f6
Gerrit-Change-Number: 13063
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)