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

ThreadPoolSize inspected at runtime #789

Closed
genshapiro opened this issue May 8, 2015 · 7 comments
Closed

ThreadPoolSize inspected at runtime #789

genshapiro opened this issue May 8, 2015 · 7 comments

Comments

@genshapiro
Copy link

Is there any way to inspect ThreadPoolSize at runtime? There appears to be no API to get a reference to command's ThreadPool.

@genshapiro
Copy link
Author

HystrixCommand's java doc is clear on PropertiesDefaults - they are immutable. Yet, the Setter with its numerous build methods (+ custom property loading, derivation logic, etc) is executed on every command instantiation. This amounts to creating a large number of unnecessary objects over VM lifetime. What is the reason for this design? Thank you

@benjchristensen
Copy link
Contributor

You don't need to instantiate the Setter inside the constructor every time. You can define it once statically and then just reference it from within the constructor. This is a better general practice.

Pseudocode like this ...

public static class MyCommand {
  private final static Setter s = ...

  public MyCommand(...) {
   super(s)
  }
}

@genshapiro
Copy link
Author

Thanks for fast response.
You would expect creating commands with different keys all the time and execution parameters (max_concurrent or timeout) may be different for commands in the same group, in that case caching your Setters is becoming a bit more complicated but still workable.
Consider updating your how-to-dos with this pattern, because today all online code samples have Setters instances in the constructors.

@genshapiro
Copy link
Author

A follow up question - in your sample you are caching the HystrixCommand.Setter, not HystrixCommandProperties.Setter...is that something that can be done? javadoc does not say that command setter attributes get cached.

did you mean something like (below)?

public static class MyCommand {
private final static Setter s = ...
public MyCommand(...) {
super(Setter.withGroupKey().andCommandKey().andCommandPropertiesDefaults(s);
}
}

What would happen if you really cached HystrixCommand.Setter? would that not interfere with metrics, etc?

@mattrjacobs
Copy link
Contributor

@genshapiro These are good questions. Sorry for the delay in noticing that you had unanswered questions in this thread.

If you were to cache the HystrixCommand.Setter, that does not pose a problem because the HystrixCommand constructor which takes a Setter immediately takes out the relevant pieces here: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommand.java#L128. It's then expected that the commandProperties / threadPoolProperties get cached.

@mattrjacobs
Copy link
Contributor

I will take a look at the docs to make this style of command construction the recommended way. I also noticed 3 constructors in HystrixCommand not using the preferred style:

https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommand.java#L56
https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommand.java#L74
https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommand.java#L91.

I will fix that and add a jmh test to see how many allocations are saved in #850

@mattrjacobs
Copy link
Contributor

Added documentation of the cached Setter pattern to: https://github.com/Netflix/Hystrix/wiki/How-To-Use#command-name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants
@mattrjacobs @benjchristensen @genshapiro and others