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

Use Archaius for Hystrix plugin setup #711

Merged

Conversation

eirslett
Copy link
Contributor

Use Archaius for Hystrix plugin setup

Currently, Hystrix lets you define custom strategies in two ways:

  1. with System properties
  2. with bootstrapping via HystrixPlugins.getInstance().registerXXX
    If neither is specified, the default strategy is used.

This change replaces hardwired System.getProperty calls with lookup
via Archaius. So one can override the plugin strategies used with
any configuration provider Archaius is configured with.

Most importantly, it has the effect of checking any file called
"config.properties" on the classpath as well as using system properties.
This lets you configure plugin strategies without having to write
Java code for it, or having to run the application with additional
system properties.

Fixes #92

According to #92 (comment), Hystrix is supposed to load its configuration from Archaius, but apparently that is not the case with HystrixPlugins. This PR fixes that.

Currently, Hystrix lets you define custom strategies in two ways:
1) with System properties
2) with bootstrapping via HystrixPlugins.getInstance().registerXXX
If neither is specified, the default strategy is used.

This change replaces hardwired System.getProperty calls with lookup
via Archaius. So one can override the plugin strategies used with
any configuration provider Archaius is configured with.

Most importantly, it has the effect of checking any file called
"config.properties" on the classpath as well as using system properties.
This lets you configure plugin strategies without having to write
Java code for it, or having to run the application with additional
system properties.

Fixes Netflix#92
@cloudbees-pull-request-builder

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

@eirslett
Copy link
Contributor Author

Ideally, it would also check for "hystrix.properties", and use that if found on the classpath?

If the file hystrix-plugins.properties exists on the class path, its
configuration will be used to setup HystrixPlugins.

This makes it possible to bundle bootstrap configuration for Hystrix
in higher-level libraries without polluting the global namespace with
a 'config.properties' file; that name should better be reserved
for top-level applications only.
@cloudbees-pull-request-builder

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

@eirslett
Copy link
Contributor Author

@michaelsembwever @chriswk @hennings would this help us integrate Zipkin and Hystrix without boilerplate in the userland code?

@mattrjacobs
Copy link
Contributor

Thanks for the contribution, @eirslett . This looks like a good change, but I need to play around with it and understand how I would use it before I merge. Along those lines, if you could include some sample setup on how you intend to use it, that would probably be helpful (for me and for the docs)

@eirslett
Copy link
Contributor Author

Here's a simple demo library,

https://github.com/eirslett/pull-request-illustration

The idea is that you would put this library in your classpath, and it would bootstrap everything for you.
(The alternative would be to set up the ConcurrencyStrategy manually in Java land, but that would be more intrusive, and violate Zipkin's "It Should Just Work" philosophy)
Does that clear things up?

@mattrjacobs
Copy link
Contributor

This looks like a great change. Thanks for the patience, @eirslett . I'll add some documentation around this to the Wiki shortly.

mattrjacobs added a commit that referenced this pull request Mar 25, 2015
…-lookup

Use Archaius for Hystrix plugin setup
@mattrjacobs mattrjacobs merged commit 1484e70 into Netflix:master Mar 25, 2015
@mattrjacobs
Copy link
Contributor

@eirslett I just added some documentation around this change, as I'm about to release it in 1.4.3. For now, I've got a pointer to your illustrative repo - thanks very much for that! At some point, I will add some examples of this to the Hystrix repo.

Can you take a look and see if it makes sense to you? : https://github.com/Netflix/Hystrix/wiki/Plugins#how-to-use

/cc @DavidMGross

@eirslett
Copy link
Contributor Author

Sure, looks good to me!

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.

Is there a way to automatically pass thread static information to the Hystrix Command thread pool?
3 participants