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

Added supports for setting threadPoolProperties through @HystrixCommand annotation #326

Merged
merged 5 commits into from
Nov 9, 2014

Conversation

yuhanz
Copy link

@yuhanz yuhanz commented Oct 18, 2014

The @HystrixCommand annotation is creating the threadPool using the default settings. we'd like to make the threadPool configurable as part of the annotation.

@cloudbees-pull-request-builder

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

@yuhanz
Copy link
Author

yuhanz commented Oct 18, 2014

The build inherited the failed test case since last pull request. The test case is inside hystrix-core, which wasn't modified in this pull request (only at hystrix-avanica).

@benjchristensen
Copy link
Contributor

@dmgcodevil can you please review these changes?

@dmgcodevil
Copy link
Contributor

I added comments for the code lines. Ben could you please check my comments (maybe you can suggest better way)? Also I was planning add ability to annotate whole class to implement "default" properties feature. I gonna start implementation in the near future.

…f passing ThreadPoolPropertiesSetter into the constructor
@cloudbees-pull-request-builder

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

@yuhanz
Copy link
Author

yuhanz commented Oct 21, 2014

ok I just made the change accordingly to use ConfigurationManager, and got rid of the if-else ladder.

I tried locally with debugger. it works. Please review. thanks

@dmgcodevil
Copy link
Contributor

Thanks, looks better. Update README as well. But I still see few places for improvements, please fix them.

…s. problem: metricsRollingStatisticalWindowInMilliseconds, and metricsRollingStatisticalWindowBuckets wouldn't be set correctly
@cloudbees-pull-request-builder

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

@yuhanz
Copy link
Author

yuhanz commented Oct 22, 2014

I did the refactoring accordingly, and added some test cases.

It ended up discovering that metricsRollingStatisticalWindowInMilliseconds and metricsRollingStatisticalWindowBuckets wouldn't be set correctly.
"hystrix.threadpool.{}.metricsRollingStatisticalWindowInMilliseconds" doesn't seem to work.. I'm guessing metricsRollingStatisticalWindow* might be using a different prefix in the ConfigurationManager.

so what's the correct prefix for metrixRollingStatisticalWindow* ?

Thank you.

@dmgcodevil
Copy link
Contributor

As I know this is property of Hystrix command:

hystrix.command.default.metrics.rollingStats.timeInMilliseconds

@cloudbees-pull-request-builder

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

@dmgcodevil
Copy link
Contributor

@benjchristensen I finished reviewing of this PL, could you please merge it ? Thanks.
@yuhanz Seems like you need also make same pool request for https://github.com/Netflix/Hystrix/tree/1.3.x branch.

@yuhanz
Copy link
Author

yuhanz commented Oct 23, 2014

cool thanks. I made another pull request at branch 1.3.x: #329

@benjchristensen benjchristensen added this to the 1.4 milestone Oct 27, 2014
benjchristensen added a commit that referenced this pull request Nov 9, 2014
Added supports for setting threadPoolProperties through @HystrixCommand annotation
@benjchristensen benjchristensen merged commit 020b8d9 into Netflix:master Nov 9, 2014
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.

None yet

4 participants