Skip to content

Conversation

@laminelam
Copy link
Contributor

@laminelam laminelam commented Mar 30, 2023

https://issues.apache.org/jira/browse/SOLR-16687

Description

A Solr class loader is needed in SolrZKClient to instantiate classes part of modules.

This is part of a bigger PR to add support of AWS Secret Manager to store Zookeeper credentials.

Tests

Local integration tests.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Copy link
Contributor

@gerlowskija gerlowskija left a comment

Choose a reason for hiding this comment

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

This looks 👍 so far. Given a bit more time for other folks to review and some automated tests, I'd be happy to merge!

@HoustonPutman
Copy link
Contributor

I think the classloader needs to be used in both createZkCredentialsToAddAutomatically and createZkACLProvider as well.

@laminelam
Copy link
Contributor Author

This looks 👍 so far. Given a bit more time for other folks to review and some automated tests, I'd be happy to merge!

Hi @gerlowskija
Added a test case to test SCL based instantiation, will add more tests (SC vs DCL scenarios) as part of the main PR to add support of AWS Secret Manager.

I have also changed the behavior in case of an Exception is raised when instantiating ZkACLProvider or ZkCredentialsInjector, it should fail-fast rather then ignoring it and go with the default ZkACLProvider/ZkCredentialsInjector which sets znodes permissions to OPEN_ACL_UNSAFE.

@laminelam
Copy link
Contributor Author

I think the classloader needs to be used in both createZkCredentialsToAddAutomatically and createZkACLProvider as well.

Hi @HoustonPutman

I think a solrClassLoader is not needed for ZkACLProvider and ZkCredentialsProvider except if we add in the future custom implementations part of another contrib/module, but it doesn't hurt to add it now. I have added it to both methods.

@laminelam laminelam requested a review from gerlowskija May 12, 2023 01:18
Copy link
Contributor

@gerlowskija gerlowskija left a comment

Choose a reason for hiding this comment

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

This LGTM from a testing perspective; it's not ideal but I've wracked my brain trying to come up with a more comprehensive way to do this, without much luck.

It'd be good to give Houston another few days to chime in though, since he had other concerns to speak to.

@HoustonPutman
Copy link
Contributor

My concerns have been addressed!

@laminelam laminelam requested a review from gerlowskija May 16, 2023 20:47
@gerlowskija gerlowskija merged commit 1016e8b into apache:main May 22, 2023
gerlowskija added a commit that referenced this pull request May 25, 2023
Allows ZkCredentialsProviders from other modules or plugins to be found by
SolrZkClient.

---------

Co-authored-by: Lamine Idjeraoui <lidjeraoui@apple.com>
Co-authored-by: Jason Gerlowski <gerlowskija@apache.org>
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.

3 participants