Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add CachingGroupProviderModule to trino-plugin-toolkit #18174

Closed
wants to merge 1 commit into from
Closed

Add CachingGroupProviderModule to trino-plugin-toolkit #18174

wants to merge 1 commit into from

Conversation

ksobolew
Copy link
Contributor

@ksobolew ksobolew commented Jul 7, 2023 •
edited
Loading

Description

If added to the list of Modules used in initialization of a Guice context in a GroupProviderFactory, it will (almost) automatically add caching capability to the group provider. Requirements:

  • The GroupProvider available in the Guice context must be bound annotated with @ForCachingGroupProvider binding annotation

The module will make the following configuration options available (to be set in etc/group-provider.properties:

  • <prefix>.cache.enabled - the toggle to enable or disable caching
  • <prefix>.cache.ttl - determines how long group information will be cached for each user
  • <prefix>.cache.maximum-size - maximum number of users for which groups are stored in the cache

Where <prefix> is optional and configurable. Also configurable is a binding annotation with which the "output" GroupProvider interface will be bound (useful in some cases when the module is not strictly isolated).

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Security
* Introduce option to enable caching of group information retrieved from group providers.

@cla-bot cla-bot bot added the cla-signed label Jul 7, 2023
@ksobolew ksobolew mentioned this pull request Jul 7, 2023
Closed
@ksobolew ksobolew force-pushed the kudi/group-provider-caching branch from cd9a62a to 974b8b7 Compare July 10, 2023 15:21
@ksobolew ksobolew changed the title Group provider caching Add a caching layer on top of GroupProviderManager Jul 10, 2023
@ksobolew ksobolew marked this pull request as ready for review July 10, 2023 15:23
@github-actions github-actions bot added tests:hive hive Hive connector labels Jul 10, 2023
@ksobolew
Copy link
Contributor Author

The test failure is caused by the test removed in #18202.

@kokosing
Copy link
Member

Let's provide a caching in the plugin toolkit and then we could apply caching to specific group providers where caching is needed only.

@ksobolew ksobolew force-pushed the kudi/group-provider-caching branch from 974b8b7 to 8fa87c9 Compare July 11, 2023 14:54
@ksobolew ksobolew changed the title Add a caching layer on top of GroupProviderManager Add CachingGroupProviderModule to trino-plugin-toolkit Jul 11, 2023
@ksobolew
Copy link
Contributor Author

Pushed a second version. PTAL.

@ksobolew ksobolew force-pushed the kudi/group-provider-caching branch 2 times, most recently from c519115 to 569ce68 Compare July 12, 2023 07:45
kokosing
kokosing reviewed Jul 12, 2023
View reviewed changes
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/group/CachingGroupProvider.java Outdated Show resolved Hide resolved
...rino-plugin-toolkit/src/main/java/io/trino/plugin/base/group/CachingGroupProviderModule.java Show resolved Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/group/GroupProviderConfig.java Outdated Show resolved Hide resolved
...rino-plugin-toolkit/src/main/java/io/trino/plugin/base/group/CachingGroupProviderModule.java Outdated Show resolved Hide resolved
...rino-plugin-toolkit/src/main/java/io/trino/plugin/base/group/CachingGroupProviderConfig.java Show resolved Hide resolved
lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/group/TestCachingGroupProvider.java Show resolved Hide resolved
@ksobolew ksobolew force-pushed the kudi/group-provider-caching branch 2 times, most recently from 184e6fe to 1e39460 Compare July 13, 2023 10:55
@ksobolew
Copy link
Contributor Author

Added the ability to invalidate individual keys or all of the cache.

@ksobolew
Copy link
Contributor Author

Oh, and the first commit is refactored somewhat :)

@ksobolew ksobolew force-pushed the kudi/group-provider-caching branch from 1e39460 to 28493ab Compare July 13, 2023 14:18
@ksobolew ksobolew force-pushed the kudi/group-provider-caching branch 2 times, most recently from ff64aae to 35c091b Compare July 27, 2023 15:14
@ksobolew
Copy link
Contributor Author

I believe this is now fully ready for review.

@ksobolew ksobolew requested a review from kokosing July 28, 2023 12:41
@ksobolew
Copy link
Contributor Author

Flaky hit: #18392 (already fixed, rebase needed)

@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jan 12, 2024
@mosabua
Copy link
Member

mosabua commented Jan 12, 2024

@kokosing and @ksobolew I assume you are continuing the work on this PR.

@ksobolew
Copy link
Contributor Author

@mosabua I believe this has stalled because this has the most value with Group Providers that are not (yet) in Trino itself. I am aware of 2 (two) proposed PRs to add an LDAP Group Provider and it may be valuable for them, but one of them has also stalled and the other I haven't looked at yet to see if it does any "native" caching.

@github-actions github-actions bot removed the stale label Jan 15, 2024
@mosabua
Copy link
Member

mosabua commented Jan 15, 2024

I will leave it in your hands to decide how to proceed @ksobolew ;-)

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Feb 6, 2024
@ksobolew
Add CachingGroupProviderModule to trino-plugin-toolkit
88dd9e6 
This Guice Module can be used to enable caching in the group provider,
by adding it to the list of modules in a Guice context in a group
provider factory, or to any other Guice context as needed.

Features:

* Configurable configuration prefix
* Ability to bind the final `GroupProvider` with a custom binding
  annotation
  * useful especially when the Guice context is not entirely
    isolated and there are other `GroupProvider` bindings in it
* An `@Inject`-able hook for cache invalidation
@ksobolew ksobolew mentioned this pull request Feb 7, 2024
Closed
@github-actions github-actions bot removed the stale label Feb 7, 2024
@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Feb 29, 2024
@mosabua
Copy link
Member

mosabua commented Feb 29, 2024

@kokosing can you get this PR to completion with @ksobolew ?

@ksobolew
Copy link
Contributor Author

ksobolew commented Mar 1, 2024

This work has been technically merged into #20157, but it seems that one has also stalled.

@github-actions github-actions bot removed the stale label Mar 4, 2024
@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Mar 26, 2024
@github-actions GitHub Actions
Copy link

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions bot closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kokosing kokosing Awaiting requested review from kokosing

Assignees
No one assigned
Labels
cla-signed hive Hive connector stale
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ksobolew @kokosing @mosabua