Make Archaius a soft dependency through reflection and improve plugin…

[agentgt]

See #970 #129 #252

This change does a couple of things to hystrix-core that should be fairly safe and backward compatibility

• It checks to see if Archaius is available through reflection and will use it if is available. Notice I did not change the dependencies so backward compatibility should be fine.
• Besides using properties for plugin discovery the JDK service loader is used as a fallback.
• I added a new plugin called HystrixDynamicProperties due to the whole nasty coupling with HystrixPropertiesChainedArchaiusProperty and PropertyWrapper. Thus you don't have to replace every XXXStrategy if you only want to change configuration source.
• The HystrixDynamicProperties can only be loaded through a ServiceLoader (cause you got not properties to resolve it :) ). Thus HystrixPlugin must load with some plugin implementation and thus is final. If no implementation is found an implementation that uses the System.properties is used ( we could instead fail). This would only happen if you didn't have Archaius in your classpath and no other implementation available (ie serviceloader).
• There are a whole bunch of contrib stuff that uses Archaius directly that I did not fix but in theory could be.

I need these changes because right now my company cannot use Archaius and thus we have our own nasty circruitbreaker stuff that I would love to get rid of.

I know @mattrjacobs was working on this as well. This is mainly to invoke discussion. I'm also not so sure what the coding style is or should be (I saw tabs but used spaces...). Advance apologies.

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #340 SUCCESS
This pull request looks good

[mattrjacobs]

Thanks for the PR, @agentgt . Very good timing, as I was just working on this. Will review and leave comments.

[agentgt]

Thanks @mattrjacobs . I can cleanup the code and add Javadoc tomorrow. The only downside to this implementation is that if you really really do not want Archaius you will have to manually exclude the dependency (since if we make the dependency optional it will break existing users). But even if it is in the classpath one could provide another HystrixDynamicProperties implementation through the ServiceLoader mechanism and thus prevent loading (since Java lazy loads classes).

This is also fairly hard to unit test with out creating dummy projects but I have done something similar in the past to test something like this (apologies for no unit test). I'll see if I can add a unit test later.

[mattrjacobs]

I took a first pass through this and it looks like it does what I think it should. I will set up a couple projects in hystrix-examples to get some more experience with it before I merge.

Another problem that I'd like to solve is the actual compile dependency on Archaius. I'd like to drop that and make a new contrib module hystrix-archauis that provides that binding. I know that's a bit of work - I was already planning on doing it, so I'm happy to either do that myself or you can.

Once I get some examples, with and without Archaius, working, I'll layer those on top of this PR.

Thanks for this!

[agentgt]

@mattrjacobs I thought about making a hystrix-archaius module but I couldn't figure out a way with out seriously breaking most of the unit tests and/or having a cyclic dependency since the hystrix-core unit tests would need hystrix-archaius and the hystrix-archaius module would need hystrix-core for the interfaces.

The other reason I didn't do it was that I was hoping not to have to wait till version 2.0 for this to be released. @benjchristensen had alluded that making sweeping changes in the config loading (and existing interfaces in general) would have to wait till 2.0 (something my company can't really wait for albeit we can just for fork for now). I'm not sure if the same applies for unit tests but I rather be safe and have a more incremental process (ie release more).

Speaking of which I'll be adding unit tests shortly to test the ServiceLoader part :) (the reflection is already heavily hit with the existing tests).

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #341 SUCCESS
This pull request looks good

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #343 FAILURE
Looks like there's a problem with this pull request

[agentgt]

@mattrjacobs I have finished adding Unit tests and Javadoc. The only thing I noticed/changed is how the HystrixPlugin is loaded as a singleton eagerly.

    //We should not load unless we are requested to. This avoids accidental initialization. @agentgt
    //See https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
    private static class LazyHolder { private static final HystrixPlugins INSTANCE = HystrixPlugins.create(); }

I prefer lazy loading so you don't accidentally kick off loading by just importing HystrixPlugin. There is a slight chance this could affect backward compatibility and or performance (but this is very dubious). This may have been even done on purpose. Perhaps @benjchristensen would know?

Let me know if and when you want me to squash the commits.

[mattrjacobs]

This change is already backwards incompatible with 1.4/1.5, due to the type hierarchy change. When I swapped out hystrix-core-1.4.23.jar with a jar built from this branch, I got a java.lang.VerifyError that included this:

Reason:
    Type 'com/netflix/hystrix/strategy/properties/HystrixPropertiesChainedArchaiusProperty$DynamicBooleanProperty' (current frame, stack[3]) is not assignable to 'com/netflix/config/PropertyWrapper'

Changing the type hierarchy is not permissible. So it's already looking like this might have to go to 2.0.

[agentgt]

@mattrjacobs If that is the case we should probably rename that class since its no longer tied to Archaius and make the other classes in there (HystrixPropertiesChainedArchaiusProperty) final and maybe even protected/default.

For now my company can just roll and release our own in our private repo.

[agentgt]

@mattrjacobs I think I can fix the hierarchy change easily (well somewhat) by just creating another HystrixPropertiesChainedArchaiusProperty and calling it something else probably HystrixPropertiesChainedProperty and then just update all the default strategies to that class. That is I would just leave HystrixPropertiesChainedArchaiusProperty alone.

I'll update the PR with this change shortly.

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #345 FAILURE
Looks like there's a problem with this pull request

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #346 SUCCESS
This pull request looks good

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #347 SUCCESS
This pull request looks good

[agentgt]

@mattrjacobs I tested the latest in isolation without Archaius in the classpath and it now seems to work fine. When you get a chance can you test on your end as I can't test the verify hierarchy issue you had (n.b. I accidentally amended some commits and force pushed).

The only change I want to make is perhaps simplify HystrixPropertiesChainedProperty (which replaces the existing archaius one but the archaius one remains for existing client code). It just has too many public code touch points and there doesn't appear to be an internal package for Hystrix core. For 2.0 there probably should be so we don't have these backward compatibility issues.

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #348 FAILURE
Looks like there's a problem with this pull request

[mattrjacobs]

@agentgt I talked to @elandau (owner of Archaius) and got some more clarity on where things stand.

Let me know if the following plans make sense for you:

1. Keep going with this PR. If we can ensure backwards-compatibility, then it can get released in 1.5.0. The functional change would be that Hystrix can work without Archaius on the classpath. This should solve your immediate need. As a side note, I'm still not sure about j.u.ServiceLoader - I need to do more research/experimentation to understand it better.

hystrix-core would still depend on Archauis, as some pieces of the inheritance hierarchy are in Archaius packages (com.netflix.config). However, we'd start establishing the path where configuration implementation is decoupled.

2. In Hystrix 2.0, remove Archaius from hystrix-core. Configuration objects would still exist, but the only implementation in hystrix-core would be one with no external library dependencies - something like j.u.ServiceLoader or some other strategy that pulls config from properties files at app start

Then, add in hystrix-archauis and hystrix-archaius2 submodules that can fulfill the config interfaces in hystrix-core. Archaius 2.0 was just released, and has a much tighter dependency set. If other configuration mechanisms are useful, submodules to integrate them could be introduced as well.

Let me know if this makes sense.

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #349 SUCCESS
This pull request looks good

[agentgt]

Thanks @mattrjacobs for the followup!

As for the ServiceLoader it does have some rather undeserved criticism on some stackoverflow post (that I can't seem to find) but the fact is its used heavily in the Java world during bootstrapping and thus the class is generally already loaded. In fact the logging library logback uses the ServiceLoader (I would know because I put it in 😃 ) .

You could certainly load using a System property but I believe this causes more issues and is not canonical. The service loader also has the advance benefit if you just put a jar with the right META-INF registration it will load it.. ie plugins on demand based on dependencies.

As for Archaius 2.0 I really hope it does well but I found it still to be rather buggy (I'm in fact surprised there is now a 2.0 tag... and it has been released).

I'm really hoping Archaius becomes the slf4j for configuration. I wrote my own configfacade lib for my company because there are so many libs/frameworks now that used a mixture of configuration formats/sources and I need an abstraction.

When we figure out the 1.5 stuff I'll continue to help port the code and/or fix all the unit tests that reference Archaius (directly or indirectly).

[mattrjacobs]

@agentgt I've just confirmed that this version is binary-compatible with 1.4.x. Thanks for getting that done! I'll take another pass through your changes to see if anything else requires review.

I've only tested the case in which Archaius is provided, and verified that this still works. Have you tested that excluding Archaius drops back to static configuration?

[agentgt]

@mattrjacobs Yes I have manually tested with Archaius not in the classpath and it works (it uses the system properties).

I'm thinking of checking a system property to disable loading archaius even if it is on the classpath so that we can at least have unit test coverage for that execution path of falling back to system properties (I already have a test for the service loader path).

That should be my last change.

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #354 FAILURE
Looks like there's a problem with this pull request

[agentgt]

@mattrjacobs I added some more unit tests and way to load the HystrixDynamicProperties via a system property ("hystrix.plugin.HystrixDynamicProperties.implementation").

This made unit testing a little easier as well as providing an option to completely avoid the service loader for those that might be concerned about the performance of initial resource checking (ie cmdline
-Dhystrix.plugin.HystrixDynamicProperties.implementation=someclass).

I don't think I have any more changes. Thank you!

[mattrjacobs]

Thanks, @agentgt . Reviewing now...

[mattrjacobs]

This doesn't resolve. Can you add the {@link XXX} syntax and either import HystrixArchaiusHelper or use the fully-qualified class name?

[agentgt]

I will remove the line altogether. HystrixArchaiusHelper now is private and internal anyway (hence why I moved it to the same package as HystrixPlugin).

[mattrjacobs]

I'm going to try to summarize the changes to make sure I understand them. Please correct me if I got any of the details wrong.

• The first time HystrixPlugins gets referenced, nothing happens, due to use of HystrixPlugins.LazyHolder
• The first time HystrixPlugins.getInstance() gets referenced, that kicks off the work to create an instance. This should happen either via a user calling HystrixPlugins.getInstance().registerXXX() to set up something environment-specific, or as the first command executes, when it tries to start resolving plugin instances
• The creation of the HystrixPlugins instance sets up the instance of HystrixDynamicProperties, by checking the following, in order:
  • Using System.properties to resolve "hystrix.plugin.HystrixDynamicProperties.implementation"
  • Using ServiceLoader to find an instance of HystrixDynamicProperties.
  • If com.netflix.config.ConfigurationManager is on the classpath:
    • Look in resource hystrix-plugins and use Archaius to load it
    • Create com.netflix.hystrix.strategy.properties.archaius.HystrixDynamicPropertiesArchaius
  • Use System.properties

Because of that loading algorithm and the use of lazy loading, Archaius is not referenced until you've had a chance to override it. And not finding it still allows the System properties to work as an implementation of HystrixDynamicProperties. That instance (however it is arrived at), is used in all other plugin lookups.

For command/threadpool/collapser/timer-threadpool properties, these are all instances of HystrixPropertiesChainedProperty. As a result, they can be chained and support an API that isn't tied to Archaius (nice work here!). Invocations of add() result in calls to HystrixPropertiesChainedProperty.getDynamicProperty(), which completes the loop by calling back to HystrixPlugins.getInstance().getDynamicProperties(). That was what got set up by the bullet-points above.

This all makes sense to me, and I think is a valuable addition. I left a couple of line-level comments as well, but wanted to make sure I reviewed the changeset as a whole somewhere (and that I understand how it hangs together).

Thanks for the contribution!

[mattrjacobs]

IMO, I think it's valuable to add a logger.info call here on each possible return. This will aid debugging as users can understand where the HystrixDynamicProperties instance is getting loaded from.

Something like :
`logger.info("Created HystrixDynamicProperties instance from System property named "hystrix.plugin.HystrixDynamicProperties.implementation");

or

logger.info("Created HystrixDynamicProperties instance by loading from ServiceLoader. Using class : " + hp.getClass().getSimpleName()");

As examples of verbosity.

What do you think?

[agentgt]

See my other line comment on logging. We have to be very careful about when we kick off logging. It needs to be after whatever configuration framework has loaded (HystrixDynamicProperties) because the configuration framework may want to participate in configuring logging. This is a long going chicken/egg problem with java config and logging. This appears to be after so we can probably do it but we cannot statically call getLogger or else we load the logging framework too early. I'll see if I can put in a safe change.

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #355 FAILURE
Looks like there's a problem with this pull request

[agentgt]

@mattrjacobs I added logging (safely) and fixed the javadoc issue. Let me know what you think.

[mattrjacobs]

Any reason for mixing debug/info level logging? My preference would be for all to be at info.

[mattrjacobs]

I just tested this again in an Archaius environment, and that worked as expected, including the new logging

[agentgt]

Any reason for mixing debug/info level logging? My preference would be for all to be at info.

Backward compatibility. Existing users might not like the noise... ie we are adding an info statement that didn't happen before. I made the system properties one info because its almost at warning state (hence the exclamation mark instead of a period).

Feel free to change it. I took my best guess and what I think I would have liked but I can see an argument made for any case.

[mattrjacobs]

OK, works for me. Thanks for all your work getting this done. I'll merge and cut RC3 shortly. I'd love any validation you could do on that.

[mattrjacobs]

Released in 1.5.0-rc.3 now.