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

Remove Dependency on commons-configuration #182

Closed
benjchristensen opened this issue Apr 30, 2014 · 24 comments
Closed

Remove Dependency on commons-configuration #182

benjchristensen opened this issue Apr 30, 2014 · 24 comments

Comments

@benjchristensen
Copy link

Is it possible to refactor and eliminate dependency on commons-configuration?

In Hystrix I'm exploring whether I should eliminate use of Archaius in core due to different environments having conflicts. The primary source of conflicts is the Apache Commons libraries (Netflix/Hystrix#252 as example).

If Archaius truly needs commons-configuration, does the usage of it need to be part of the public API, or can the usage be shaded internally so it's not a dependency seen by consumers?

@gorzell
Copy link
Collaborator

gorzell commented Apr 30, 2014

I think the dependency can be eliminated. This has been on my list of big things to look at for a long time, however given it's size I haven't had a big enough chunk of time to take a hard look. This came up in another issue I can try and look up later.

@benjchristensen
Copy link
Author

After writing this I realized there is now also Guava and Jackson ... those are both problematic for a library to depend on as well. What do you think of those?

@gorzell
Copy link
Collaborator

gorzell commented Apr 30, 2014

Those I haven't thought about at all. I was really trying to decouple the API from the implementation as a first pass to enable things like #148. The hardest part about making that work is dealing with all the bootstrap logic that is static.

@howardyuan
Copy link
Contributor

I'm more at ease with removing Guava and Jackson. It's all matter of internal implementation. I worry more about removing commons-configuration since that might break certain existing usage of Archaius. I know that we (Netflix) also depend on certain things provided by commons-configuration (don't remember what on top of my head). Nonetheless, it's doable. I'll look the issue as well.

@benjchristensen
Copy link
Author

Thanks @howardyuan Please let me know when it's done so I can update Hystrix to the new version.

@howardyuan
Copy link
Contributor

What I plan to do is breaking up the task into two parts

  1. Part with Guava and Jackson.
  2. Pull out any commons-configuration dependancies into a different module so people who depend on it can resort to it while keeping the archaius-core lean.

This might take some time given my current schedule. I'll try to come up with road map first and stick with it.

@allenxwang
Copy link

We used the following functionalities from commons-configuration:

  1. A key-value pair store that is listenable and fire events on updates. This is the foundation of dynamic properties.
  2. Support data type like Integer, Float, Boolean, etc., for getters.
  3. Support variable interpolation. For example, if foo=1 and bar=${foo}, then bar is also 1.
  4. Support creation of sub configuration based on a prefix of keys.

So it seems to me that we can abstract out these requirements into some interfaces and provide a default implementation on ConcurrentHashMap.

We may also consider if we should support custom data types and pluggable serialization mechanism.

@gorzell
Copy link
Collaborator

gorzell commented May 9, 2014

@allenxwang I think that might actually be a bit more than Ben really needs. Especially writing an alternate implementation based on ConcurrentHashMap. As a first step I think just limiting core to the bare bones functionality that people use, and especially removing the initialization logic is probably enough. SLF4J is probably a good example of a similar paradigm, where intermediate client libraries can use the core functionality, but allow downstream users to provide the implementation rather than forcing it on them. @benjchristensen correct me of I am getting this wrong.

From a users point of view there are two main portions, initialization and creating/using properties. I don't think that either of these need to have concrete implementations in core in order to be used by clients, so the main thing is to detach them from any specific implementation. Providing alternate implementations is a separate exercise.

@gorzell
Copy link
Collaborator

gorzell commented May 9, 2014

Also as @benjchristensen the current API does have places where it leaks the underlying implementation, which also will need to be closed up.

@benjchristensen
Copy link
Author

The 3rd party dependencies are the issues I'm trying to solve here as they are what cause the transitive problems.

Separating out implementations into separate modules would be great, and if that's how we solve the dependency issue that's fine, but it's not necessarily required for what I'm asking.

Ultimately a library using Archaius needs 2 things:

  1. Ability to implement dynamic properties via interfaces or concrete classes with no dependencies
  2. Allow the application using the library to configure how the backing implementation gets the properties ... but be able to function with defaults if no implementation is given (which means there is some concrete behavior, unlike SL4J).

Here is the usage of Archaius in Hystrix: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/strategy/properties/HystrixPropertiesChainedArchaiusProperty.java

And how the defaults are defined: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommandProperties.java#L108

As it is currently, it still requires far too much code to use it, as shown by the need to custom implement many types in that class.

Thank you @gorzell and @howardyuan for your involvement on this. It will be great to get Archaius streamlined.

@mattrjacobs
Copy link

@gorzell @howardyuan Any progress on removing Guava/Jackson from the exported set of Archaius dependencies? I'm combing through Hystrix backlog and ran across Netflix/Hystrix#252 and was curious where this was at. Thanks!

@gorzell
Copy link
Collaborator

gorzell commented Feb 26, 2015

I have been thinking about this a bit. I think it really depends on which dependency you care the most about. I don't actually know how guava is being used, I can guess about Jackson. These can probably be pulled out into sub modules, or surgically removed pretty easily.

The dependency on commons-configuration is a much bigger deal and would probably warrant a major version effort, and breaking backwards compatibility. Some of the key things to think about would be:

  • Separating the API from the implementation
  • How to best plug in/load the correct implementation at runtime
  • The bootstrap process for archaius in general. Right now it depends on a static block and the class loader. This would probably need to be changed to something totally different in order to support a more plugable implementation.

These are just my initial thoughts, I am sure there are more concerns that I am forgetting.

PS I need to go back and review @benjchristensen 's feedback in more detail. So apologies if this is redundant or ill informed.

@spencergibb
Copy link
Contributor

👍 on changing the way archaius bootstraps. @gorzell's list is great. spring-cloud-netflix has to work around the static initializers and general use of static singletons.

@benjchristensen
Copy link
Author

Perhaps we can instead just focus on Archaius 2 which we are wanting to do anyways as we move towards async/reactive support alongside Ribbon/RxNetty/Eureka2/etc?

@gorzell
Copy link
Collaborator

gorzell commented Feb 26, 2015

@benjchristensen I took a quick look around the project and didn't see anything relating to Archaius 2. Is there a design doc or something similar somewhere? Is it in another repo that I missed?

I think we can address these issues either way, or both. Although as mentioned some of them will involve breaking existing APIs/packaging.

@benjchristensen
Copy link
Author

No there aren't yet. Just emerging discussions that Archauis v1 doesn't fit the new apps we're building very well.

@gorzell
Copy link
Collaborator

gorzell commented Feb 27, 2015

It would be great to have a chat about the goals of that effort so that we can decide how and where we want to tackle some of these bigger issues with the library.

@howardyuan
Copy link
Contributor

@elandau has put in a lot of thoughts in Archaius V2.0. I'm looping him in so he can share his thoughts. I know that he's planning to write down some doc soon.

@elandau
Copy link
Contributor

elandau commented Feb 27, 2015

I'm going to open a bunch of issues regarding Archaius 2.0. But in the meantime here are some thoughts,

Archaius will be broken up into the following projects

  • archaius-core The only dependency will be commons-lang3 for the interpolation code
  • archaius-typesafe The only dependency will be typesafe config
  • archaius-governator Will pull in governator and guice and re-implement fabricator to merge configuration with configuration mapping and DI. This will likely be what's described here, Configuration 2.0 proposal governator#182
  • archaius-legacy Adapters for the legacy API

The new API will be designed with a more robust ConfigurationManager. There will be no static classes so archaius can be easily used in unit tests. The ConfigurationManager will manage configuration loading via a pluggable ConfigurationLoader SPI. Multiple ConfigurationLoaders may be added to support various configuration solutions such as Typesafe Config and Commons Configuration, all of which will reside in separate sub-projects so dependencies will be under tight control. Cascade loading will be specified via a CascadingPolicy interface that converts a single configuration name (basename, filename, URI, etc.) to a priority ordered list of possible permutations to load. ConfigurationManager will manage configuration in two tiers. The first is the top level property solution order, such as FastProperties, Application, Library, System, Env. The second will track cascaded configurations and library configurations in the order in which they are loaded. This will only be done in the Application and Library order. The top level configuration will be responsible for resolving the property name as well as interpolating the value.

I'm also planning to redo DynamicProperty to treat property value updates as streams of updates with binding to variables and methods as an extension. This will allow us to maintain a simple property update API.

For example,

public class MyService {
     private AtomicInteger timeout = new AtomicInteger();

    @Inject
    MyService(ObservablePropertyManager manager) {
        manager.subscribe("myservice.timeout", Integer.class, 1000, FieldUpdater.create(timeout));
    }

    private void doSomething() {
        while (true) {
               ... timeout.get();
        }
    }
}

Or react directly to updates,

public class MyService {
    @Inject
    MyService(ObservablePropertyManager manager) {
        manager.subscribe(new PropertyObserver<Integer>() {
                        public void onChange(Integer newValue) {
                        }
                   }, "myservice.timeout", Integer.class, 1000, );
    }
}

@elandau
Copy link
Contributor

elandau commented Feb 27, 2015

I plan to have a prototype ready by end of next week so we can reference the code in this discussion.

@gorzell
Copy link
Collaborator

gorzell commented Feb 27, 2015

That all sounds like good progress. I am wondering if you could even pull out the string stuff into another module. I am also curious if the cost of autoboxing really warrants having specially named classes for all the primitive types (but maybe I have just been in scala land too long).

Would the stream aspect introduce a dependency of core on Rx? I understand why Rx integration is important, but I am not sure how much of the community at large would want to have it as a direct dependency.

@elandau
Copy link
Contributor

elandau commented Feb 27, 2015

Sorry, I didn't mean to imply this will have a dependency on Rx. I was just describing an Rx like API. With that said, I do envision an archaius-rxjava subproject that can convert a property value stream to an Observable so that the property can be composed into your Rx code.

@gorzell
Copy link
Collaborator

gorzell commented Feb 28, 2015

Sounds good.
On Feb 27, 2015 1:04 PM, "elandau" notifications@github.com wrote:

Sorry, I didn't mean to imply this will have a dependency on Rx. I was
just describing an Rx like API. With that said, I do envision an
archaius-rxjava subproject that can convert a property value stream to an
Observable so that the property can be composed into your Rx code.


Reply to this email directly or view it on GitHub
#182 (comment).

@elandau
Copy link
Contributor

elandau commented Feb 28, 2015

I issued PR #216 for v2 initial implementation. If you want to review it's best to fork and look at the code locally because the PR changes are painful to look at as they include all the deleted files.

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

No branches or pull requests

7 participants