Netflix / Hystrix

Hystrix is a latency and fault tolerance library designed to isolate points of access to remote systems, services and 3rd party libraries, stop cascading failure and enable resilience in complex distributed systems where failure is inevitable.
24.12k stars 4.7k forks source link

Support for archaius2 #1248

Open wg-tsuereth opened 8 years ago

wg-tsuereth commented 8 years ago

Hystrix's "soft" dependency on Archaius for loading configuration properties is exclusively compatible with Archaius "V1" releases; HystrixArchaiusHelper distinctly detects and uses two classes that no longer exist in archaius2. The Archaius 2.x series broke out of RC in February.

Is there any intent to support archaius2 in Hystrix?

The existing Archaius integration relies primarily on ConfigurationManager to statically access configuration, and this class has been removed in 2.x.

wg-tsuereth commented 8 years ago

N.B. Someone just pointed me at a "bridge" module from Archaius that can magically solve this problem. See: https://github.com/Netflix/archaius/pull/303

mattrjacobs commented 8 years ago

There's definitely intent to support Archaius2, but no progress yet. Because there are a few Archaius1 classes in public signatures (an inadvertent mistake), it will require breaking backward-compatibility.

The internal application I work on at Netflix hasn't moved to Archaius2 yet, so any sort of experience report you can give would be helpful to me and likely other Hystrix users.

wg-tsuereth commented 8 years ago

The archaius2-archaius1-bridge library seems (in my limited usage so far) to satisfy Hystrix's integration requirements with legacy Archaius, so there's that; although I doubt the Archaius team would intend to maintain this migration path indefinitely.

As I've indicated in some other reports/requests which reference this issue, configuring Hystrix without this Archaius integration is a much less pleasant experience -- not only painful to use, but also missing dynamic configurability.

mattrjacobs commented 8 years ago

Thanks @wg-tsuereth. The intent is definitely to move to Archaius 2, but having the bridge solution is excellent, as we can decouple Hystrix/Archaius migrations.

agentgt commented 8 years ago

@wg-tsuereth I don't recommend the bridge as I had some issues with it (I will post a link if I can find the issue).

You should be able to use Archaius 2 now by writing your own HystrixDynamicProperties plugin. The plugin should be fairly trivial to write. I'll see if I can write the plugin today.

There is one gigantic caveat with this approach. There are several contrib addons that blindly use Archaius (particularly the metric addons). If you don't use these addons you should be fine (that is anything in hystrix-core should be using the new HystrixDynamicProperties if its not it is a bug). You will also need to exclude old Archaius from the classpath other wise it will load.

It would be fairly easy to add a separate module with archaius2 support but there was some reason why that was not done yet.

@mattrjacobs we should probably update the wiki that you can plugin in your own configuration system.

agentgt commented 8 years ago

So to iterate how you would do this:

  1. Implement your own version of HystrixDynamicProperties
  2. Register the plugin of which there are two ways covered in the HystrixPlugin javadoc
  3. Optionally remove Archaius from classpath using whatever your build systems dependency exclusion mechanism is.

The system properties way would be -Dhystrix.plugin.HystrixDynamicProperties.implementation=canonicalNameOfClass.

The service loader would be by creating a file called META-INF/services/com.netflix.hystrix.strategy.properties.HystrixDynamicProperties and putting the binary name of the implementation class in that file. For example: com.example.MyHystrixDynamicProperties (beware of inner classes as you will have to use $ hence the binary name).

The service loader mechanism is extremely convenient as you can make your own custom jar with an implementation of any plugin and Hystrix will automatically load it and I believe it will take precedence over old Archaius even if it is in the classpath.

The tricky part of Archaius2 implementation and one of the major reasons why I haven't implemented is deciding on how to get access to it directly or indirectly (consequently I'm not sure if a builtin plugin of archaius2 will ever be a good idea). Probably the ideal way would be to use Google Guice and use Google Guice as your main point of initialization. The easiest way though would probably be to make your own static singleton of Archaius2 (albeit nasty it is the easiest).

In the long run Hystrix should probably not even have a plugin strategy but rather an IoC strategy (ie injection strategy) but it was way too much work at the time as well as out of scope.

wg-tsuereth commented 8 years ago

@agentgt: Thanks for your advice. I finally got some time today to work on the HystrixDynamicProperties plugin you suggested. Unfortunately, this was quite annoying to hack the configuration dependency into.

Since my HystrixDynamicProperties implementation is loaded and instantiated by the Hystrix plugin loader, I can't inject dependencies into it using my existing DI framework (Guice in this case). Instead, similar to the static singleton idea you mentioned, I ended up linking the plugin to a static pseudo-factory for Hystrix properties, which was itself injected with the configuration dependency.

The plugin, HystrixDynamicPropertiesArchaius2.java:

package <...>;

import com.netflix.hystrix.strategy.properties.HystrixDynamicProperties;
import com.netflix.hystrix.strategy.properties.HystrixDynamicProperty;

public class HystrixDynamicPropertiesArchaius2 implements HystrixDynamicProperties {

    public HystrixDynamicPropertiesArchaius2() {}

    public HystrixDynamicProperty<String> getString(final String name, final String fallback) {
        return HystrixArchaius2PropertyFactory.createString(name, fallback);
    }

    public HystrixDynamicProperty<Integer> getInteger(final String name, final Integer fallback) {
        return HystrixArchaius2PropertyFactory.createInteger(name, fallback);
    }

    public HystrixDynamicProperty<Long> getLong(final String name, final Long fallback) {
        return HystrixArchaius2PropertyFactory.createLong(name, fallback);
    }

    public HystrixDynamicProperty<Boolean> getBoolean(final String name, final Boolean fallback) {
        return HystrixArchaius2PropertyFactory.createBoolean(name, fallback);
    }
}

(Plugged-in using a META-INF services file as you suggested.)

And the "factory," HystrixArchaius2PropertyFactory.java:

package <...>;

import com.google.inject.Inject;
import com.netflix.archaius.api.Config;
import com.netflix.hystrix.strategy.properties.HystrixDynamicProperty;

public class HystrixArchaius2PropertyFactory {
    @Inject
    static private Config config;

    static public HystrixDynamicProperty<String> createString(String name, String fallback) {
        return new HystrixDynamicProperty<String>() {
            @Override
            public String getName() { return name; }
            @Override
            public String get() {
                if(config == null) {
                    return fallback;
                }
                return config.getString(name, fallback);
            }
            @Override
            public void addCallback(Runnable callback) {}
        };
    }

    <... Repeating this pattern for createInteger, createLong, and createBoolean ...>
}

Finally, the archaius2 Config (exposed elsewhere in the project, via a Guice ArchaiusModule) gets injected by Guice like so:

public class HystrixArchaius2Module extends AbstractModule {
    @Override
    protected void configure() {
        requestStaticInjection(HystrixArchaius2PropertyFactory.class);
    }
}

This implementation seems to work fine, and by all appearances should be fairly resilient to updates in both Hystrix and Archaius. The DI implementation is just a bit messier than I would have hoped.

I don't recommend the bridge as I had some issues with it (I will post a link if I can find the issue).

I'm curious about these issues, if you're able to find that information again.

agentgt commented 8 years ago

This implementation seems to work fine, and by all appearances should be fairly resilient to updates in both Hystrix and Archaius. The DI implementation is just a bit messier than I would have hoped.

Any recommendations would be appreciated. Probably the ServiceLoader should load a factory interface instead but I didn't want to have to make yet another interface. For future major versions of Hystrix I would have the ServiceLoader first load a strategy to load plugins (ie a meta plugin). The interface would be analogous to Archaius IoCContainer SPI.

But really the fundamental problem is that Hystrix does a lot of static initialization because it follows the traditional command pattern. For it to be DI friendly you would want the DI framework to create a Command for you every time or at the minimum some sort of factory instead of using plain anonymous class constructors. The class constructors (commands) then use static singletons to resolve configuration (this is called the Service Locator pattern btw which is a DI anti-pattern).

Consequently I'm not a fan of the command pattern because of this and prefer the message bus pattern (ie bus.request(SomeRequest) -> SomeResponse). The command pattern just glues too much together. It is (un)/fortunate that Hystrix is really two frameworks: a command pattern framework, and a thread/execution management framework.

Initialization is inherently pretty nasty in Java. You have three things all competing to be first: Dependency Injection, Configuration, Logging. Sadly logging usually wins this battle. I have talked about this in numerous places (I really need to consolidate it all in a blog post).

I'm curious about these issues, if you're able to find that information again.

I suppose this was one of the issues I was thinking about: https://github.com/Netflix/archaius/issues/386 There was another weird issue with initialization as well but I think this might have been specific to our implementation.

wg-tsuereth commented 8 years ago

As far as recommendations, all I can immediately think of is like you mentioned, a factory interface between ServiceLoader and the plugin. Speaking more broadly--

For future major versions of Hystrix I would have the ServiceLoader first load a strategy to load plugins (ie a meta plugin).

From my admittedly limited experience trying to configure Hystrix, I've repeatedly butted up against code plumbing that looks to accomplish little more than getting in my way. I illustrated this more pointedly in issue #1249 (and a little less so in #1252).

So from my backseat-architect perspective, I would generally recommend less meta-code, in favor of keeping the surface simple and concise. Personally I would rather see that my use-case isn't supported and make a fork, than dig through dense callstacks trying to figure out where the "correct" configuration or injection vector might be.