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.

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

updates column visibility to use accumulo access library #3746

Merged
merged 20 commits into from
Jul 8, 2024

Conversation

keith-turner
Copy link
Contributor

Updates ColumnVisibility and VisibilityEvaluator to use the Accumulo Access control library :

https://github.com/apache/accumulo-access

This library is not yet released, so building requires installing it locally.

Updates ColumnVisibility and VisibilityEvaluator to use the Accumulo
Access control library :

https://github.com/apache/accumulo-access

This library is not yet released, so building requires installing it
locally.
@keith-turner keith-turner changed the title updates column visibility to accumulo access library updates column visibility to use accumulo access library Sep 7, 2023
@keith-turner keith-turner marked this pull request as ready for review June 14, 2024 20:17
*/
public org.apache.accumulo.access.Authorizations toAccessAuthorizations() {
if (auths.isEmpty()) {
return org.apache.accumulo.access.Authorizations.of(Set.of());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If org.apache.accumulo.access.Authorizations.EMPTY is not made public in apache/accumulo-access#70, then we should get a reference to an empty authorizations object and return it here instead of calling Set.of here. Considering this is called from the VisibilityFilter, we should make this as optimized as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the impl of Set.of() in my ide it does not allocate any objects, it always returns a statically initialized immutable empty set. Then with the eventual changes in accumulo-access this line will do no obj allocations, however it does that in a very round about way.

Should probably add a Authorizations.of() method to follow javas conventions. Also would make it consistent w/ AccessExpression.of(). If Authorizations.of() existed then this could be more straightforward.

Copy link
Contributor Author

@keith-turner keith-turner Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a constant iin 5b825ab, seemed the best options w/ the current constraints of the beta API. Can easily change it later.

@dlmarion
Copy link
Contributor

dlmarion commented Jul 8, 2024

I added accumulo-access to the ASF Jenkins (https://ci-builds.apache.org/job/Accumulo/job/Accumulo-Access-main) which should push the built jars into the snapshot repo. I'm wondering if that's enough to change the dependency from 1.0.0-beta to 1.0.0-SNAPSHOT as there have been changes since the beta release. It doesn't look like it's building reliably, but I just kicked a build off.

Update: Build finished, new snapshot artifacts at https://repository.apache.org/content/repositories/snapshots/org/apache/accumulo/accumulo-access/1.0.0-SNAPSHOT/.

Update #2: Since Accumulo's parent pom is the Apache Parent POM, we don't need to do anything to reference snapshot releases in the ASF repo. It's already included in the Parent POM.

@keith-turner keith-turner merged commit 1e347f8 into apache:main Jul 8, 2024
8 checks passed
@keith-turner keith-turner deleted the use-access-library branch July 8, 2024 21:38
@ctubbsii ctubbsii added this to the 3.1.0 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants