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

fixes #3399: Access to configuration in context class loader factory #3400

Merged
merged 7 commits into from May 23, 2023
Merged

fixes #3399: Access to configuration in context class loader factory #3400

merged 7 commits into from May 23, 2023

Conversation

ivakegg
Copy link
Contributor

@ivakegg ivakegg commented May 12, 2023

  • Added a context class loader environment that supplies configuration
  • Updated class loader util to set the configuration on the factory

@ivakegg
fixes apache#3399: Access to configuration in context class loader fa…
34ff056 
…ctory

* Added a context class loader environment that supplies configuration
* Updated class loader util to set the configuration on the factory
@DomGarguilo DomGarguilo linked an issue May 12, 2023 that may be closed by this pull request
Access to AccumuloConfiguration when overridding classloader #3399
Closed
@ivakegg
re apache#3399: Access to configuration in context class loader factory
de074bd 
* Updated default context class loader factory to use environment
@dlmarion dlmarion requested a review from ctubbsii May 12, 2023 20:13
@ctubbsii ctubbsii added this to In progress in 2.1.1 via automation May 13, 2023
@ctubbsii
Copy link
Member

Overall, this looks like a good idea, but I'm still working through reviewing the details of the change.

@ctubbsii
Code review changes - Minimize changes for apache#3400
6175dd5 
* Improve javadoc comments on environment interface
* Remove unneeded AccumuloConfigurationAdaptor - instead, keep the
  DefaultContextClassLoaderFactory constructed directly using the
  internal configuration, rather than load it through reflection using
  the no-arg constructor. We don't want users configuring this class
  manually, so it should not have a no-arg constructor. As the javadoc
  states, it is for internal use only.
* Use a lambda for passing the service configuration rather than an
  anonymous inner class (less code, same effect)
ctubbsii
ctubbsii requested changes May 13, 2023
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I think this PR can be much more minimal. I have provided my code review suggestions to minimize the changes for this in ivakegg#5

core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfigurationAdapter.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderEnvironment.java Show resolved Hide resolved
@keith-turner
Copy link
Contributor

but the InitParameters doesn't seem to be as accurate as the ...Environment name.

I think of it as a generic pattern, not something specific to this use. If an API/SPI method takes 1 parameter and you later want to add another, then it would have to be overloaded. Overloading is not a great way to evolve methods, especially for plugins. If each plugin interface method uses a bit of indirection and only takes one parameter which is an interface that gives access to all of the actual parameters, it makes it much easier to evolve the inputs for the method. So I see this as a generic pattern that all SPI methods can follow. If they are all following same pattern, its nice to use the same names.

@ivakegg ivakegg requested a review from ctubbsii May 23, 2023 15:42
ctubbsii and others added 2 commits May 23, 2023 17:12
@ctubbsii ctubbsii merged commit 969fc52 into apache:2.1 May 23, 2023
8 checks passed
2.1.1 automation moved this from In progress to Done May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith-turner keith-turner keith-turner approved these changes

@ctubbsii ctubbsii ctubbsii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
2.1.1
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Access to AccumuloConfiguration when overridding classloader
3 participants
@ivakegg @ctubbsii @keith-turner