Implement a custom HystrixPropertiesStrategy plugin #36

lloydmeta · 2014-10-01T12:29:07Z

Add a custom HystrixPropertiesStrategy implementation where getCommandPropertiesCacheKey is overridden to return null so that HystrixPropertiesFactory will always generate a new HystrixCommandProperties for each new HystrixCommand instantiation.

lloydmeta · 2014-10-01T12:29:52Z

Should fix #31

Apparently you can only register a strategy once inside a single Java process and subsequent attempts will cause exceptions to be thrown. Instead of registering in Global, which, depending on libs/other singletons/w/e, may already be too late, use the hystrix.plugin.HystrixPropertiesStrategy.implementation system property and name our class.

coveralls · 2014-10-01T13:13:48Z

Coverage increased (+9.63%) when pulling d77bbde on lloydmeta:feature/cacheless-hystrix-properties-strategy into 4bd7b80 on m3dev:develop.

Might be a good idea to try to make sure CachelessHystrixPropertiesStrategy is used in Global during onStart if the system property is not set

coveralls · 2014-10-01T13:50:53Z

Coverage increased (+9.3%) when pulling 5153fef on lloydmeta:feature/cacheless-hystrix-properties-strategy into 4bd7b80 on m3dev:develop.

lloydmeta · 2014-10-02T00:52:47Z

app/com/m3/octoparts/Global.scala

+   * Resist the temptation to do a HystrixPlugins.getInstance().getPropertiesStrategy first to do
+   * checking, as that actually also sets the strategy if it isn't already set.
+   */
+  def setHystrixPropertiesStrategy(app: Application): Unit = {


I'm unsure if this is a good idea, or we should just make end-users responsible for always sending the hystrix.plugin.HystrixPropertiesStrategy.implementation system property. Suggestions on other alternatives are welcome.

As Octoparts should mostly run out-of-the-box, I'd suggest to not use this hystrix.plugin.HystrixPropertiesStrategy.implementation env variable, and use this new one in any case.

mauhiz · 2014-10-03T00:55:44Z

I woud like to clarify :

Will a change in timeout affect currently running commands?
Is there an issue on hystrix github or at least some ml thread that recommends using this approach?

lloydmeta · 2014-10-03T01:03:02Z

It shouldn't affect currently running commands, as those are instantiated with these properties
Javadoc of that method say For example, null can be returned which would cause it to not cache and invoke {@link #getCommandProperties} for each {@link HystrixCommand} instantiation (not recommended) I think the "not recommended" is for performance reasons.

cb372 · 2014-10-03T01:38:42Z

Also of note is this Hystrix issue: Netflix/Hystrix#34

Looks like they don't want people to be changing properties at runtime.

mauhiz · 2014-10-03T01:50:08Z

app/com/m3/octoparts/hystrix/CachelessHystrixPropertiesStrategy.scala

+ * generate a new [[HystrixCommandProperties]] for each new [[com.netflix.hystrix.HystrixCommand]] instantiation
+ */
+class CachelessHystrixPropertiesStrategy extends HystrixPropertiesStrategy {
+  override def getCommandPropertiesCacheKey(commandKey: HystrixCommandKey, builder: HystrixCommandProperties.Setter): String = null


We could build the cache key with commandKey + (a hash of) builder, this should avoid the forewarned performance issues.

Ah, so maintain our own Map as a cache. Sounds like a good idea.

Reflecting on this now, I guess can get the hashCode of builder, combine it with the commandKey.name in a string and return that.

HystrixPropertiesFactory will be responsible for caching the properties using the returned string anyways.

The disadvantage is that theoretically, the cache could grow out of hand.

@mauhiz @cb372 what do you think?

Looks like the least invasive solution so far 👍
On Oct 3, 2014 7:21 PM, "Lloyd" [email protected] wrote:

In app/com/m3/octoparts/hystrix/CachelessHystrixPropertiesStrategy.scala:

@@ -0,0 +1,13 @@
+package com.m3.octoparts.hystrix
+
+import com.netflix.hystrix.{ HystrixCommandKey, HystrixCommandProperties }
+import com.netflix.hystrix.strategy.properties.HystrixPropertiesStrategy
+
+/**

* Custom [[HystrixPropertiesStrategy]] implementation where getCommandPropertiesCacheKey is overridden

* to return null so that [[com.netflix.hystrix.strategy.properties.HystrixPropertiesFactory]] will always

* generate a new [[HystrixCommandProperties]] for each new [[com.netflix.hystrix.HystrixCommand]] instantiation

*/
+class CachelessHystrixPropertiesStrategy extends HystrixPropertiesStrategy {

override def getCommandPropertiesCacheKey(commandKey: HystrixCommandKey, builder: HystrixCommandProperties.Setter): String = null

Reflecting on this now, I guess can get the hashCode of builder, combine
it with the commandKey.name in a string and return that.

`HystrixPropertiesFactory will be responsible for caching the properties
using the returned string anyways.

The disadvantage is that theoretically, the cache could grow out of hand.

@mauhiz https://github.com/mauhiz @cb372 https://github.com/cb372
what do you think?

—
Reply to this email directly or view it on GitHub
https://github.com/m3dev/octoparts/pull/36/files#r18388802.

Hmm, builder is not a case class and hashCode for the builder is not overridden to be based on the values of the builder, meaning we get new hashCodes for every single newly instantiated builder..

I suppose one way is to manually write a function that grabs values from the builder's accessors (there are at least 20 now), but I don't want to go down that route without exploring other solutions (maybe even just continue returning null)

Nevermind, as of cf0507a, I've changed it to return a string that is a combination of the CommandKey name and the builder JSON string value. Human-readable too now as a bonus.

lloydmeta · 2014-10-03T01:59:02Z

Interesting. I think that PR relates to not allowing injecting different property strategies (via a HystrixCommand) depending on code path, not properties themselves ("property strategy" is not the same as "property"). Since then, the project has changed so that once you set a property strategy, you can no longer change it

Regarding CircuitBreakers, we currently don't allow customisation of circuit breaker settings, so it doesn't directly affect us (yet), and a quick look through the code shows that CircuitBreakers are cached in HystrixCircuitBreaker.java on a per-CommandKey-name basis, so those don't get to change anyways.

I think with Octoparts being essentially something that adds runtime definition support of Hystrix-backed operations, our use-case is slightly different and maybe we might have to ignore some of the recommendations they give. The alternative is we say you can't change Hystrix-related stuff once it's been defined (or at least without restarting the service), which is doesn't seem very friendly.

mauhiz · 2014-10-06T00:28:28Z

.travis.yml

@@ -14,4 +14,4 @@ before_script:
  - psql -c "CREATE USER octoparts_app WITH PASSWORD '';" -U postgres
  - psql -c "GRANT ALL PRIVILEGES ON DATABASE octoparts_test to octoparts_app;" -U postgres

-script: "sbt coveralls"
+script: "sbt coveralls test"


didn't coveralls already run tests?

Not sure if it was the way we were using it, but the coveralls task is troublesome in that if the compilation fails (like this), the exit status is 0 if the report is uploaded to coveralls properly and the whole build is marked as successful 😆

There may be a more elegant solution to adding test after coveralls but I'm not too sure.

coveralls · 2014-10-07T02:05:34Z

Coverage increased (+9.95%) when pulling e0686f1 on lloydmeta:feature/cacheless-hystrix-properties-strategy into 4bd7b80 on m3dev:develop.

…rties-strategy Implement a custom HystrixPropertiesStrategy plugin

Check and try to set Properties Strategy in Global

5153fef

Might be a good idea to try to make sure CachelessHystrixPropertiesStrategy is used in Global during onStart if the system property is not set

lloydmeta reviewed Oct 2, 2014
View reviewed changes

lloydmeta added the bugfix label Oct 2, 2014

mauhiz reviewed Oct 3, 2014
View reviewed changes

lloydmeta added 2 commits October 3, 2014 23:34

Switch to KeyAndBuilderValuesHystrixPropertiesStrategy

cf0507a

Make the mapper ignore null values

fbac667

mauhiz reviewed Oct 6, 2014
View reviewed changes

Be less controlling about HystrixPropertiesStrategy

e0686f1

lloydmeta added this to the 2.2 milestone Oct 7, 2014

mauhiz added a commit that referenced this pull request Oct 8, 2014

Merge pull request #36 from lloydmeta/feature/cacheless-hystrix-prope…

91cb2e0

…rties-strategy Implement a custom HystrixPropertiesStrategy plugin

mauhiz merged commit 91cb2e0 into m3dev:develop Oct 8, 2014

mauhiz mentioned this pull request Oct 8, 2014

Updates to Hystrix timeout setting are not applied #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a custom HystrixPropertiesStrategy plugin #36

Implement a custom HystrixPropertiesStrategy plugin #36

lloydmeta commented Oct 1, 2014

lloydmeta commented Oct 1, 2014

coveralls commented Oct 1, 2014

coveralls commented Oct 1, 2014

lloydmeta Oct 2, 2014

mauhiz Oct 3, 2014

mauhiz commented Oct 3, 2014

lloydmeta commented Oct 3, 2014

cb372 commented Oct 3, 2014

mauhiz Oct 3, 2014

lloydmeta Oct 3, 2014

lloydmeta Oct 3, 2014

mauhiz Oct 3, 2014

lloydmeta Oct 3, 2014

lloydmeta Oct 3, 2014

lloydmeta commented Oct 3, 2014

mauhiz Oct 6, 2014

lloydmeta Oct 6, 2014

coveralls commented Oct 7, 2014

Implement a custom HystrixPropertiesStrategy plugin #36

Implement a custom HystrixPropertiesStrategy plugin #36

Conversation

lloydmeta commented Oct 1, 2014

lloydmeta commented Oct 1, 2014

coveralls commented Oct 1, 2014

coveralls commented Oct 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauhiz commented Oct 3, 2014

lloydmeta commented Oct 3, 2014

cb372 commented Oct 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloydmeta commented Oct 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 7, 2014