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

Implement a custom HystrixPropertiesStrategy plugin #36

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

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?

Copy link
Contributor Author

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 after coveralls but I'm not too sure.

31 changes: 29 additions & 2 deletions app/com/m3/octoparts/Global.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.{ CachelessHystrixPropertiesStrategy, 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
Expand All @@ -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 {

Expand Down Expand Up @@ -80,8 +82,8 @@ object Global extends WithFilters(MetricsFilter) with ScaldiSupport {
}

override def onStart(app: Application) = {
setHystrixPropertiesStrategy(app)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this a separate trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for this request (e.g. testing)?

Copy link
Contributor

Choose a reason for hiding this comment

The 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
much as possible; having a trait SetHystrixPropertiesStrategy extends GlobalSettings makes it clear this is a separate behaviour which is
injected here.

In the unlikely event we should be unregistering something, or adding more
stuff onStart, this will keep things clean.

On Mon, Oct 6, 2014 at 10:29 AM, Lloyd [email protected] wrote:

In app/com/m3/octoparts/Global.scala:

@@ -80,8 +82,8 @@ object Global extends WithFilters(MetricsFilter) with ScaldiSupport {
}

override def onStart(app: Application) = {

  • setHystrixPropertiesStrategy(app)

Any particular reason for this request (e.g. testing)?


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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Global : it is very much global (!), hooks into the application lifecycle, and shouldn't/can't be used anywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.
On Oct 6, 2014 10:57 AM, "Lloyd" [email protected] wrote:

In app/com/m3/octoparts/Global.scala:

@@ -80,8 +82,8 @@ object Global extends WithFilters(MetricsFilter) with ScaldiSupport {
}

override def onStart(app: Application) = {

  • setHystrixPropertiesStrategy(app)

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 global : it is very much global
(!), hooks into the application lifecycle, and shouldn't/can't be used
anywhere else.


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

super.onStart(app)

startPeriodicTasks(app)
}

Expand All @@ -97,4 +99,29 @@ object Global extends WithFilters(MetricsFilter) with ScaldiSupport {
HystrixMetricsLogger.logHystrixMetrics()
}
}

/**
* Tries to set the Hystrix properties strategy to CachelessHystrixPropertiesStrategy
*
* 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 = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 hystrix.plugin.HystrixPropertiesStrategy.implementation system property. Suggestions on other alternatives are welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 hystrix.plugin.HystrixPropertiesStrategy.implementation env variable, and use this new one in any case.

// If it's defined, we don't need to set anything
if (sys.props.get("hystrix.plugin.HystrixPropertiesStrategy.implementation").isEmpty) {
LTSVLogger.warn("-Dhystrix.plugin.HystrixPropertiesStrategy.implementation is not set. It should be" -> "com.m3.octoparts.hystrix.CachelessHystrixPropertiesStrategy")
try {
HystrixPlugins.getInstance().registerPropertiesStrategy(new CachelessHystrixPropertiesStrategy)
} catch {
case NonFatal(e) => {
val currentStrategy = HystrixPlugins.getInstance().getPropertiesStrategy.getClass
if (currentStrategy != classOf[CachelessHystrixPropertiesStrategy]) {
LTSVLogger.error(e, "Current Hystrix Properties Strategy:" -> currentStrategy)
if (Play.mode(app) != Mode.Test)
sys.error("Tried but failed to set CachelessHystrixPropertiesStrategy as HystrixPropertiesStrategy")
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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]] = {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is related to the adding of test. The project actually didn't compile without this (possibly because there was a faulty merge somewhere).

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

In app/com/m3/octoparts/cache/PartResponseCachingSupport.scala:

@@ -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]] = {
    

This is related to the adding of test. The project actually didn't
compile without this (possibly because there was a faulty merge somewhere).


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

case ce: CacheException => {
ce.getCause match {
case te: shade.TimeoutException =>
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
1 change: 1 addition & 0 deletions project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ object OctopartsBuild extends Build {
sonatypeSettings ++
coverallsSettings ++
Seq(
javaOptions += "-Dhystrix.plugin.HystrixPropertiesStrategy.implementation=com.m3.octoparts.hystrix.CachelessHystrixPropertiesStrategy",
publishArtifact := false,
libraryDependencies ++= Seq(
// Logging
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
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 CachelessHystrixPropertiesStrategySpec extends FunSpec with Matchers {

val subject = new CachelessHystrixPropertiesStrategy
val commandKey = HystrixCommandKey.Factory.asKey("hello")
val commandProps = HystrixCommandProperties.Setter()
describe("getCommandPropertiesCacheKey") {
it("should return null") {
val r = subject.getCommandPropertiesCacheKey(commandKey, commandProps)
r should be(null)
}
}

// 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 CachelessHystrixPropertiesStrategy")
}
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)
}
}

}