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

Check if the request log is enabled before requiring the context is initialized. #724

Merged
merged 1 commit into from
Mar 25, 2015

Conversation

axiak
Copy link
Contributor

@axiak axiak commented Mar 25, 2015

In 706a0f2,
the guard to initialize the request log based on the property was removed. This
causes a dependency on hystrix context initialization for all custom concurrency
strategies.

Here we add that back in, along with a test to ensure that a custom strategy could
still run without an initialized context.

This PR supercedes #718

/cc @mattrjacobs

…nitialized.

In Netflix@706a0f2,
the guard to initialize the request log based on the property was removed. This
causes a dependency on hystrix context initialization for all custom concurrency
strategies.

Here we add that back in, along with a test to ensure that a custom strategy could
still run without an initialized context.
@cloudbees-pull-request-builder

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

@cloudbees-pull-request-builder

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

@mattrjacobs
Copy link
Contributor

Looks great - thanks for adding the unit test as well that exercises this case. I've gotten Cloudbees happy, so I'm merging this and will release it ASAP as 1.4.2

mattrjacobs added a commit that referenced this pull request Mar 25, 2015
Check if the request log is enabled before requiring the context is initialized.
@mattrjacobs mattrjacobs merged commit e4b71da into Netflix:master Mar 25, 2015
@axiak
Copy link
Contributor Author

axiak commented Mar 25, 2015

Thanks!

@axiak
Copy link
Contributor Author

axiak commented Mar 26, 2015

Hey @mattrjacobs, how long do you think it'll be until 1.4.2 is published? I want to upgrade all of our stuff ASAP. Thanks again

@mattrjacobs
Copy link
Contributor

This was released yesterday afternoon: https://github.com/Netflix/Hystrix/releases/tag/v1.4.2

@axiak
Copy link
Contributor Author

axiak commented Mar 26, 2015

Doh. Our nexus proxy was caching the absence of it. Thanks again!

@axiak axiak deleted the check-request-log-enabled branch March 26, 2015 15:08
@axiak
Copy link
Contributor Author

axiak commented Mar 26, 2015

@mattrjacobs we're having an issue where the pom is defining hystrix's deps as scope runtime:
https://repo1.maven.org/maven2/com/netflix/hystrix/hystrix-core/1.4.2/hystrix-core-1.4.2.pom

The gradle.properties files claims these are at compile. Did something break in the release?

@mattrjacobs
Copy link
Contributor

This also just got reported in #730 . AFAIK, I didn't change anything that would affect this in the Hystrix code itself. I'm looking through the CI configs to see if anything changed there.

@restfulhead
Copy link
Contributor

A suggestion: It would be good to add a note about this here: https://github.com/Netflix/Hystrix/wiki/Plugins#concurrencystrategy

If I wouldn't have found this pull request, I would have never guessed that I have to set

ConfigurationManager.getConfigInstance().setProperty("hystrix.command.default.requestLog.enabled", "false");

to make a custom concurrency strategy work...

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.

4 participants