-
Notifications
You must be signed in to change notification settings - Fork 18
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
Implement a custom HystrixPropertiesStrategy plugin #36
Changes from all commits
b7aed25
d77bbde
5153fef
cf0507a
fbac667
e0686f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,11 @@ import _root_.controllers.ControllersModule | |
import com.kenshoo.play.metrics.MetricsFilter | ||
import com.m3.octoparts.cache.CacheModule | ||
import com.m3.octoparts.http.HttpModule | ||
import com.m3.octoparts.hystrix.{ HystrixMetricsLogger, HystrixModule } | ||
import com.m3.octoparts.hystrix.{ KeyAndBuilderValuesHystrixPropertiesStrategy, HystrixMetricsLogger, HystrixModule } | ||
import com.m3.octoparts.logging.PartRequestLogger | ||
import com.beachape.logging.LTSVLogger | ||
import com.m3.octoparts.repository.RepositoriesModule | ||
import com.netflix.hystrix.strategy.HystrixPlugins | ||
import com.typesafe.config.ConfigFactory | ||
import com.wordnik.swagger.config.{ ConfigFactory => SwaggerConfigFactory } | ||
import com.wordnik.swagger.model.ApiInfo | ||
|
@@ -22,6 +23,7 @@ import scaldi.play.ScaldiSupport | |
|
||
import scala.collection.concurrent.TrieMap | ||
import scala.concurrent.duration._ | ||
import scala.util.control.NonFatal | ||
|
||
object Global extends WithFilters(MetricsFilter) with ScaldiSupport { | ||
|
||
|
@@ -80,8 +82,8 @@ object Global extends WithFilters(MetricsFilter) with ScaldiSupport { | |
} | ||
|
||
override def onStart(app: Application) = { | ||
setHystrixPropertiesStrategy(app) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please make this a separate trait There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason for this request (e.g. testing)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a matter of preference, but I think we want to separate concerns as In the unlikely event we should be unregistering something, or adding more On Mon, Oct 6, 2014 at 10:29 AM, Lloyd [email protected] wrote:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern is that since there is actually no way of unregistering anything and registering the same kind of plugin twice actually causes errors, having this in a trait would imply that it can be reused somewhere else, when for all intents and purposes, this should be called only once during the application life-cycle: at it's start. That's why I think it should belong in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough.
|
||
super.onStart(app) | ||
|
||
startPeriodicTasks(app) | ||
} | ||
|
||
|
@@ -97,4 +99,25 @@ object Global extends WithFilters(MetricsFilter) with ScaldiSupport { | |
HystrixMetricsLogger.logHystrixMetrics() | ||
} | ||
} | ||
|
||
/** | ||
* Tries to set the Hystrix properties strategy to [[KeyAndBuilderValuesHystrixPropertiesStrategy]] | ||
* | ||
* 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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure if this is a good idea, or we should just make end-users responsible for always sending the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As Octoparts should mostly run out-of-the-box, I'd suggest to not use this |
||
// If it's defined, we don't need to set anything | ||
if (sys.props.get("hystrix.plugin.HystrixPropertiesStrategy.implementation").isEmpty) { | ||
LTSVLogger.info("-Dhystrix.plugin.HystrixPropertiesStrategy.implementation is not set. Defaulting to" -> "com.m3.octoparts.hystrix.KeyAndBuilderValuesHystrixPropertiesStrategy") | ||
try { | ||
HystrixPlugins.getInstance().registerPropertiesStrategy(new KeyAndBuilderValuesHystrixPropertiesStrategy) | ||
} catch { | ||
case NonFatal(e) => { | ||
val currentStrategy = HystrixPlugins.getInstance().getPropertiesStrategy.getClass | ||
LTSVLogger.info(e, "Current Hystrix Properties Strategy:" -> currentStrategy) | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,7 +63,7 @@ trait PartResponseCachingSupport extends PartRequestServiceBase with Logging { | |
|
||
private def onCacheFailure(ci: HttpPartConfig, | ||
partRequestInfo: PartRequestInfo, | ||
params: Map[ShortPartParam, String]): PartialFunction[Throwable, Future[PartResponse]] = { | ||
params: Map[ShortPartParam, Seq[String]]): PartialFunction[Throwable, Future[PartResponse]] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this change? It looks correct now but I am not sure why it was wrong on develop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to the adding of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, thanks On Mon, Oct 6, 2014 at 10:16 AM, Lloyd [email protected] wrote:
|
||
case ce: CacheException => { | ||
ce.getCause match { | ||
case te: shade.TimeoutException => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package com.m3.octoparts.hystrix | ||
|
||
import com.fasterxml.jackson.annotation.JsonInclude | ||
import com.fasterxml.jackson.databind.ObjectMapper | ||
import com.netflix.hystrix.{ HystrixCommandKey, HystrixCommandProperties } | ||
import com.netflix.hystrix.strategy.properties.HystrixPropertiesStrategy | ||
|
||
/** | ||
* Custom [[HystrixPropertiesStrategy]] implementation | ||
*/ | ||
class KeyAndBuilderValuesHystrixPropertiesStrategy extends HystrixPropertiesStrategy { | ||
private val mapper = new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL) | ||
|
||
/** | ||
* Overriden to return a [[String]] that is a combination of the commandKey name and a JSON string | ||
* of the builder values | ||
*/ | ||
override def getCommandPropertiesCacheKey(commandKey: HystrixCommandKey, builder: HystrixCommandProperties.Setter): String = | ||
s"${commandKey.name()}-${mapper.writeValueAsString(builder)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, as neither A cacheKey should be faster to produce than the actual object to be cached, so I think that defaulting to null is not that bad actually (and will avoid the ever-growing cache map). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed. To assess whether it was worth the effort (maybe this is where we should do more microbenchmarking ? ;) ), but I did a quick trace through the code needed to produce a fresh properties object. It did not look cheap, so I felt that Jackson's reflection-based to-JSON to get a cachekey might be faster. Please take a look and let me know what you think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Back with Scalameter benches :)
So even with the use of reflection, generating the cachekey is still 3 times faster than generating a fresh Hystrix properties object through a call to Edit: Reran the benchmarks with 10 times more warmups and benchruns and there is even more discrepancy, with the cachekey generation 20x faster.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fascinating. Hurrah for json then.
|
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package com.m3.octoparts.hystrix | ||
|
||
import com.netflix.hystrix.strategy.HystrixPlugins | ||
import com.netflix.hystrix.strategy.properties.HystrixPropertiesFactory | ||
import com.netflix.hystrix.{ HystrixCommandProperties, HystrixCommandKey } | ||
import org.scalatest.{ Matchers, FunSpec } | ||
|
||
class KeyAndBuilderValuesHystrixPropertiesStrategySpec extends FunSpec with Matchers { | ||
|
||
val subject = new KeyAndBuilderValuesHystrixPropertiesStrategy | ||
val commandKey = HystrixCommandKey.Factory.asKey("hello") | ||
val commandProps = HystrixCommandProperties.Setter() | ||
describe("getCommandPropertiesCacheKey") { | ||
it("should return a combination of the commandKey name and commandProps JSON value") { | ||
val r1 = subject.getCommandPropertiesCacheKey(commandKey, commandProps) | ||
r1 should be("""hello-{}""") | ||
val r2 = subject.getCommandPropertiesCacheKey(commandKey, HystrixCommandProperties.Setter().withExecutionIsolationThreadTimeoutInMilliseconds(100)) | ||
r2 should be("""hello-{"executionIsolationThreadTimeoutInMilliseconds":100}""") | ||
} | ||
} | ||
|
||
// The following works if this test is run by itself, but | ||
describe("after registering with HystrixPlugins") { | ||
|
||
it("should allow HystrixPropertiesFactory.getCommandProperties to instantiate different HystrixCommandProperties for the same command key") { | ||
if (HystrixPlugins.getInstance().getPropertiesStrategy.getClass != subject.getClass) { | ||
fail("HystrixPlugins.getPropertiesStrategy did not return KeyAndBuilderValuesHystrixPropertiesStrategy") | ||
} | ||
val properties1 = HystrixPropertiesFactory.getCommandProperties( | ||
commandKey, | ||
HystrixCommandProperties.Setter().withExecutionIsolationThreadTimeoutInMilliseconds(300)) | ||
val properties2 = HystrixPropertiesFactory.getCommandProperties( | ||
commandKey, | ||
HystrixCommandProperties.Setter().withExecutionIsolationThreadTimeoutInMilliseconds(600)) | ||
properties1.executionIsolationThreadTimeoutInMilliseconds.get should be(300) | ||
properties2.executionIsolationThreadTimeoutInMilliseconds.get should be(600) | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't coveralls already run tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
aftercoveralls
but I'm not too sure.