search

typesafehub / conductr-lib

Other
8 stars 13 forks source link

Remove injection of metrics provide in lagom scala #145

Closed ignasi35 closed 7 years ago

ignasi35 commented 7 years ago

Since Lagom 1.3.3 a Lagom Scala service include a circuitBreakerMetricsProvider so the conductr-lib doesn't need to inject one.

This currently has a workaround.

markusjura commented 7 years ago

@ignasi35 Considering that lagom1-scala-conductr-bundle-lib needs to be compatible with all Lagom 1.x versions how do we want to deal with this change in regards to conductr-lib?

ignasi35 commented 7 years ago

It's an issue we'll need to address for Lagom 1.3.0, 1.3.1 and 1.3.2 since Lagom 1.3.3+ include the circuitBreakerMetricsProvider(I edited the description to fix the version when that behavior changed).

ignasi35 commented 7 years ago

As to how to fix that, let me think about it a bit more. In my previous comment I wanted to point out that the number of affected version is small since it only impacts the Scala API.

ignasi35 commented 7 years ago

See also https://github.com/lagom/lagom/issues/755

TimMoore commented 7 years ago

Could this be resolved by making ConductRServiceLocatorComponents extend LagomServiceClientComponents and leaving the circuitBreakerMetricsProvider val in place?

I think this would mean that in pre-1.3.3 versions, ConductRServiceLocatorComponents would be providing circuitBreakerMetricsProvider as before, and with 1.3.3+ versions, it would be overriding the definition in LagomServiceClientComponents with an identical definition, resolving the ambiguity.

Is there any scenario where you would want to mix in ConductRServiceLocatorComponents but not LagomServiceClientComponents?

ignasi35 commented 7 years ago

Could this be resolved by making ConductRServiceLocatorComponents extend LagomServiceClientComponents and leaving the circuitBreakerMetricsProvider val in place?

I think this makes ConductRServiceLocatorComponents unnecessary big but it looks like the right choice to maintain backwards compatibility.

I've done a quick smoke test and the resulting sbt-conductr plugin is usable in online-auction-scala for both laogm 1.3.2 and 1.3.5 (which is good :P)

Finally, implementing this approach we're invalidating #147, #148 and #149.