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

Updates to Hystrix timeout setting are not applied #31

Closed
cb372 opened this issue Sep 25, 2014 · 8 comments
Closed

Updates to Hystrix timeout setting are not applied #31

cb372 opened this issue Sep 25, 2014 · 8 comments
Assignees
Labels

Comments

@cb372
Copy link
Contributor

cb372 commented Sep 25, 2014

  1. Register a slow endpoint that takes 1.5 seconds to respond. Give it a Hystrix timeout of 1 second.
  2. Make one or more requests to the endpoint. They will time out.
  3. Edit the endpoint in the admin UI. Change the Hystrix timeout to 2 seconds.

Expected result: Hystrix timeout changes to 2 seconds, so requests no longer timeout

Actual result: Hystix timeout is still 1 second

@lloydmeta
Copy link
Contributor

Maybe related to #18

@mauhiz
Copy link
Contributor

mauhiz commented Sep 25, 2014

It looks like a new handler (and so a new HystrixExecutor) is made on each processWithConfig. There must be some failure to clear the cache on admin screen update.

@mauhiz mauhiz added the bug label Sep 25, 2014
@lloydmeta lloydmeta self-assigned this Sep 27, 2014
@lloydmeta
Copy link
Contributor

I looked into this a bit and it seems like the configs caches are being cleared properly and subsequent retrievals from cache show the updated timeout values.

Will look into this further to see if this is something on the Hystrix-level that we didn't know about/understand.

@cb372
Copy link
Contributor Author

cb372 commented Sep 30, 2014

As @lloydmeta guessed, the old timeout value is being cached by Hystrix. Specifically the HystrixPropertiesFactory class. We are correctly passing the updated timeout value to the HystrixCommand constructor, but it is being ignored.

Hystrix does not provide any methods to clear/manually update this cache, so the options are:

  • hack around the private modifier using reflection (NO!!!)
  • change the Hystrix command key whenever the timeout setting is changed. This is the key that is used to cache the settings (tricky, as the command key is currently user-configurable)
  • use a Hystrix plugin to customise how the properties cache key is generated (also tricky - would need to store the cache key as a DB field)
  • send a PR to Hystrix and pray for their blessing
  • close as WONTFIX and add a note to the docs

@lloydmeta
Copy link
Contributor

change the Hystrix command key whenever the timeout setting is changed. This is the key that is used to cache the settings (tricky, as the command key is currently user-configurable)

Another issue with this is that it will make a mess of the Hystrix dashboard because you'll get a new "circuit" widget whenever you use a new command key.

It seems like ones with the least number of disadvantages are the last 3 (plugin, PR, or wontfix).

@mauhiz
Copy link
Contributor

mauhiz commented Sep 30, 2014

Voting for the reflection trick (at least while preparing a PR). We are not
building a piece if art, and it looks like an acceptable workaround for
such a corner case.

@lloydmeta
Copy link
Contributor

It looks like we can simply implement a Hystrix plugin that has getCommandPropertiesCacheKey return null and leave everything else the same as the default. Sure, we lose caching of the properties for a given command key, but it's another alternative.

@mauhiz
Copy link
Contributor

mauhiz commented Oct 8, 2014

Fixed by #36

@mauhiz mauhiz closed this as completed Oct 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants