Support the Scaladsl for Lagom #13

huntc · 2017-08-23T03:27:11Z

Introduces support for Lagom's Scala DSL.

Also refreshes dependencies for Lagom 1.3 and renames the Lagom libraries and packages in accordance with the conventions required for binary compatibility moving forward.

Fixes #8

Will be released as 2.0.0 given the breaking changes.

fsat · 2017-08-23T04:03:34Z

...ocator-dns/src/main/scala/com/lightbend/lagom/scaladsl/dns/DnsServiceLocatorComponents.scala

+ * Provides the DNS service locator.
+ */
+trait DnsServiceLocatorComponents extends CircuitBreakerComponents {
+  def serviceLocatorService: ActorRef


Shall we instantiate the serviceLocatorService:ActorRef by default for the sake of convenience? I don't think there could be any other ActorRef other than actorSystem.actorOf(com.lightbend.dns.locator.ServiceLocator.props).

Sounds like a reasonable thing to do.

fsat · 2017-08-23T04:04:22Z

...-service-locator-dns/src/main/scala/com/lightbend/lagom/scaladsl/dns/DnsServiceLocator.scala

+
+  override def locate(name: String, serviceCall: Descriptor.Call[_, _]): Future[Option[URI]] =
+    serviceLocatorService
+      .ask(ServiceLocatorService.GetAddress(name))(settings.resolveTimeout1 + settings.resolveTimeout1 + settings.resolveTimeout2)


Why are we appending these timeout settings?

Code from before... multiple lookup ops.

fsat

Overall code lgtm. I have question about why we append the timeout settings, and also suggestion to consider instantiating serviceLocatorService: ActorRef in the DnsServiceLocatorComponents.

TimMoore

Would also be good if @rcavalcanti could look at this, since he recently worked with the service locator code in Lagom.

TimMoore · 2017-08-23T07:01:28Z

...ocator-dns/src/main/scala/com/lightbend/lagom/scaladsl/dns/DnsServiceLocatorComponents.scala

+/**
+ * Provides the DNS service locator.
+ */
+trait DnsServiceLocatorComponents extends CircuitBreakerComponents {


As discussed in Slack, I'm not sure if it's better to extend CircuitBreakerComponents, or declare abstract members that you require, but I think the latter would be better.

OK, but again, the conductr-lib ones that @jroper wrote also inherit from CircuitBreakerComponents...

I don't know the exact context here, but my reason for extending CircuitBreakerComponents in ConductR was that, from a conceptual point of view, the service locator was providing the circuit breakers - it's the thing that does the circuit breaking, and although it does allow you to override the circuit breakers it provides, it's not really conceptually asking for them to be providing them, as far as it's concerned it owns them. At least, that's my justification for it, the real reason is that it's less code for people to just mix in the service locator rather than having to mix in both the service locator and the components that provide the circuit breakers.

the real reason is that it's less code for people to just mix in the service locator rather than having to mix in both the service locator and the components that provide the circuit breakers

Sold.

Works for me

TimMoore · 2017-08-23T07:05:34Z

project/Dependencies.scala

+  val akkaDns            = "ru.smslv.akka"       %% "akka-dns"              % Version.akkaDns
+  val akkaTestkit        = "com.typesafe.akka"   %% "akka-testkit"          % Version.akka
+  val lagom13JavaClient  = "com.lightbend.lagom" %% "lagom-javadsl-client"  % Version.lagom13
+  val lagom13ScalaClient = "com.lightbend.lagom" %% "lagom-scaladsl-client" % Version.lagom13


I think the Lagom dependencies should be "provided". Otherwise, there's the potential that a service that depends on the latest service-locator-dns but an older Lagom version will end up with mixed versions of different Lagom modules.

An older Lagom version in the old 1.3.x series should be binary compatible though - recall that the name of this library is prefix with lagom13. As per conductr-lib, we'll issue a lagom14 on its arrival (or soon after).

Yeah, but you wouldn't want to mix Lagom client 1.3.7 with, say Lagom server 1.3.5.

I thought we had changed the Lagom dependencies in conductr-lib to provided for this reason, but I checked again and it looks like not. We definitely discussed it at some point.

I guess it was tried and rejected in typesafehub/conductr-lib#129

In Lagom 1.4.0, CircuitBreakers will be deprecated in favour of CircuitBreakersPanel (lagom/lagom#804).

We will also have support for multiple uris (lagom/lagom#731) and implemented here (lagom/lagom#900).

But that's a topic for another PR.

Thanks for the advice @rcalvalcanti

TimMoore · 2017-08-23T07:06:16Z

lagom13-java-service-locator-dns/build.sbt


 libraryDependencies ++= Seq(
-  Library.lagom,
+  Library.lagom13JavaClient,
  Library.akkaTestkit,


Should Akka TestKit also be in "test" scope?

TimMoore · 2017-08-23T07:06:29Z

lagom13-scala-service-locator-dns/build.sbt

+
+libraryDependencies ++= Seq(
+  Library.lagom13ScalaClient,
+  Library.akkaTestkit,


Should Akka TestKit also be in "test" scope?

The Lagom projects also now support the Lagom13 designators as we treat each minor release of Lagom as being binary incompatible with the previous one. (+2 squashed commits)

huntc · 2017-08-23T10:14:23Z

Yeah, but you wouldn't want to mix Lagom client 1.3.7 with, say Lagom server 1.3.5.

... but why not @tmoore? Are you implying that they won't be binary compatible?

TimMoore · 2017-08-23T23:28:48Z

Correct, we make no guarantees about binary compatibility of internal interfaces between different Lagom modules, only the public interface used by Lagom services. Mixing different versions of different Lagom modules into one classpath is not something we test, support, or expect to work.

huntc · 2017-08-23T23:57:59Z

@TimMoore

#13 (comment)

...then we must ensure that internal APIs aren't necessary to implement things such as the ServiceLocator... I think that part of the issue here has been that we've needed to use internal APIs (which I believe you've addressed given the latest service locator).

TimMoore · 2017-08-24T00:58:01Z

Agreed. Regardless of that, I think it makes sense to declare the dependencies as "provided". Is there a drawback?

TimMoore · 2017-08-24T01:17:25Z

Here's a previous discussion for context typesafehub/conductr-lib#127

@jroper's conclusion at the time was that the best solution is to have the Lagom sbt plugin enforce consistency. This is not yet implemented, and I'm not fully convinced that this is the best solution.

I'd like to understand why marking the dependency on Lagom as provided is not workable (for this project or for conductr-lib), as this seems to be the exact use case that "provided" scope was invented for. I think of compile vs. provided scope as being roughly aligned to the library vs. framework distinction. Do you want your artifact to pull the dependency in, or do you expect it to already be there?

huntc · 2017-08-24T03:09:05Z

@tmoore I think there's also an arg to be made for "my lib is verified to work with its dependencies at the stated coordinates". Perhaps the real issue here is with the semver assumptions made by dependency management tooling. Npm may have this right, where each lib had its own dependency graph ie transitive dependencies are not shared.

Perhaps there's also consideration of the scope of "internal" eg should play-json have direct dependencies on internally scoped types within play-core...

TimMoore · 2017-08-24T03:23:51Z

Agree, but we have to work with the tools we have. This is a sore point for me, and maybe a conversation best had over a beer 😄

Play JSON and Play core is a bit of a different issue, because Play JSON has a completely different lifecycle than Play core. It is part of an independent build and has its own versioning.

This is not true of the Lagom modules (at least right now): they are all part of one build and one version lifecycle. They are partitioned into separate modules mostly to allow downstream users to depend only on the parts that they need, but it is not allowed to mix and match different versions of different Lagom modules.

A more apt comparison would be between different core Play modules (e.g., play-java and play-server) or different Akka core modules (e.g., akka-actor and akka-stream). As far as I understand, you're not able to mix different versions of those, either. In fact, this is one reason why we can't easily allow users to mix different versions of different Lagom modules, as they might bring in different versions of those transitive dependencies.

huntc requested a review from fsat August 23, 2017 03:27

huntc force-pushed the scaladsl branch 2 times, most recently from 963ee7a to e527612 Compare August 23, 2017 03:34

huntc mentioned this pull request Aug 23, 2017

Provide Scala API for service-locator-dns #8

Closed

fsat reviewed Aug 23, 2017

View reviewed changes

huntc force-pushed the scaladsl branch from e527612 to 6ac16d0 Compare August 23, 2017 05:23

fsat approved these changes Aug 23, 2017

View reviewed changes

TimMoore reviewed Aug 23, 2017

View reviewed changes

Scala DSL

b5498a7

The Lagom projects also now support the Lagom13 designators as we treat each minor release of Lagom as being binary incompatible with the previous one. (+2 squashed commits)

huntc force-pushed the scaladsl branch from 6ac16d0 to b5498a7 Compare August 23, 2017 07:20

TimMoore approved these changes Aug 23, 2017

View reviewed changes

Remove modules - not required for Scala

0ed33b7

huntc changed the title ~~WIP - DO NOT MERGE - Support the Scaladsl for Lagom~~ Support the Scaladsl for Lagom Aug 23, 2017

huntc merged commit 5557854 into lightbend:master Aug 23, 2017

huntc deleted the scaladsl branch August 23, 2017 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the Scaladsl for Lagom #13

Support the Scaladsl for Lagom #13

huntc commented Aug 23, 2017 •

edited

Loading

fsat Aug 23, 2017

huntc Aug 23, 2017

fsat Aug 23, 2017

huntc Aug 23, 2017

fsat left a comment

TimMoore left a comment

TimMoore Aug 23, 2017

huntc Aug 23, 2017

jroper Aug 23, 2017

huntc Aug 23, 2017

TimMoore Aug 23, 2017

TimMoore Aug 23, 2017

huntc Aug 23, 2017

TimMoore Aug 23, 2017

octonato Aug 23, 2017

huntc Aug 23, 2017

TimMoore Aug 23, 2017

huntc Aug 23, 2017

TimMoore Aug 23, 2017

huntc commented Aug 23, 2017

TimMoore commented Aug 23, 2017

huntc commented Aug 23, 2017

TimMoore commented Aug 24, 2017

TimMoore commented Aug 24, 2017

huntc commented Aug 24, 2017

TimMoore commented Aug 24, 2017

Support the Scaladsl for Lagom #13

Support the Scaladsl for Lagom #13

Conversation

huntc commented Aug 23, 2017 • edited Loading

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

fsat left a comment

Choose a reason for hiding this comment

TimMoore left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntc commented Aug 23, 2017

TimMoore commented Aug 23, 2017

huntc commented Aug 23, 2017

TimMoore commented Aug 24, 2017

TimMoore commented Aug 24, 2017

huntc commented Aug 24, 2017

TimMoore commented Aug 24, 2017

huntc commented Aug 23, 2017 •

edited

Loading