Consul based service discovery #202

mlosiewicz-pl · 2018-05-16T15:38:44Z

Fixes #198

…uration comments update

ktoso · 2018-05-17T06:20:02Z

discovery-consul/src/main/resources/reference.conf

+  # Set the following in your application.conf if you want to use this discovery mechanism:
+  # impl = akka-consul
+
+  # configured the akka-consul provider


configured => configure

…nfigure

…ce for embedded consul test

mlosiewicz-pl · 2018-05-17T12:29:22Z

Does anyone have any idea how to fix:

[error] (akka-discovery-aws-api-async/compile:compileIncremental) scala.reflect.internal.FatalError: Error accessing /home/travis/.ivy2/cache/org.apache.httpcomponents/httpcore/jars/httpcore-4.4.7.jar

in travis CI?

raboof · 2018-05-17T12:35:21Z

That seems like some sort of infra problem - I restarted the build.

ktoso · 2018-05-20T00:32:52Z

Thanks a lot, I'll review in depth on monday :-)

ktoso · 2018-05-24T13:46:58Z

Well, 5 days late but reviewing now;
We're releasing 0.14 now, though this will be able to be released in an ad-hoc release whenever it is ready btw :)

ktoso

Lookin good! Some comments in line and two general ones I'd like to ask:

we need some docs about this, could you add a note how one would use this in the discovery.md file under it's own section? We likely will refactor the docs a bit in the future but a small note in there would be good already :-)
Since this assumes that apps register in consul, it also means that we need the "register myself in consul" side right? This is I think the first instance of such integration we have here in akka-management so let's make sure we set a good example. It seems to me that such application on startup should write it's keys to consul- so others will find it. Could we / should we add such register() method to the discovery class there and show how to use it? I think it's pretty powerful if we then make it a general style, so regardless if one uses zk or consul or something else the style would be the same (but that's for later).

ktoso · 2018-05-24T14:12:09Z

bootstrap-joining-demo/aws-api-ec2/build.sbt

@@ -1,6 +1,6 @@
 enablePlugins(JavaAppPackaging)

-packageName in Universal := "app" // should produce app.zip
+com.typesafe.sbt.SbtNativePackager.autoImport.packageName in Universal := "app" // should produce app.zip


was this needed?

My sbt was complaining about, but it's probably a local issue.

ktoso · 2018-05-25T01:31:34Z

discovery-consul/src/main/resources/reference.conf

@@ -0,0 +1,20 @@
+######################################################
+# Akka Service Discovery Consul Config                  #
+######################################################


nitpick about the # position ;-)

ktoso · 2018-05-25T01:51:37Z

discovery-consul/src/main/resources/reference.conf

+    consul-host = "127.0.0.1"
+    consul-port = 8500
+
+    application-name-tag-prefix = "system:"


Could you add a small comment above each of the keys what it's about?

So here we would have to explain that we expect values to be present under "system:name-of-my-actor-system" right?

ktoso · 2018-05-25T01:53:42Z

discovery-consul/src/main/scala/akka/discovery/consul/ConsulSimpleServiceDiscovery.scala

+    Consul.builder().withHostAndPort(HostAndPort.fromParts(settings.consulHost, settings.consulPort)).build()
+
+  override def lookup(name: String, resolveTimeout: FiniteDuration): Future[SimpleServiceDiscovery.Resolved] = {
+    import system.dispatcher


we try to use implicit val ec = system.dispatchet everywhere for safety -- the import if executed in a future technically could reach a null (rarely, and only during system shutdown etc etc, but we try to use the val anyway)

ktoso · 2018-05-25T01:53:52Z

discovery-consul/src/main/scala/akka/discovery/consul/ConsulSimpleServiceDiscovery.scala

+
+  private lazy val settings = ConsulSettings.get(system)
+  private lazy val consul =
+    Consul.builder().withHostAndPort(HostAndPort.fromParts(settings.consulHost, settings.consulPort)).build()


do they have to be lazy?

ktoso · 2018-05-25T02:07:33Z

discovery-consul/src/test/scala/akka/cluster/bootstrap/discovery/ConsulDiscoverySpec.scala

+  override def beforeAll(): Unit = {
+    super.beforeAll()
+    consul = ConsulStarterBuilder.consulStarter().withHttpPort(8500).build().start()
+  }


please move the beforeAll above the tests (right next to the var)

it works without var

ktoso · 2018-05-25T02:08:02Z

discovery-consul/src/test/scala/akka/cluster/bootstrap/discovery/ConsulDiscoverySpec.scala

+
+  override def beforeAll(): Unit = {
+    super.beforeAll()
+    consul = ConsulStarterBuilder.consulStarter().withHttpPort(8500).build().start()


seems there's no reason to keep it a var, can we make it a val and innit in constructor?

ktoso · 2018-05-25T02:08:44Z

project/Dependencies.scala

+      Seq(
+        "com.orbitz.consul" % "consul-client" % "1.1.2",
+        "com.pszymczyk.consul" % "embedded-consul" % "1.0.2" % "test"
+      )


Please add a comment with the license next to each of those lines, we have to make sure they are apache compatible (i.e. no GPL code)

ktoso · 2018-05-25T02:13:40Z

discovery-consul/src/main/scala/akka/discovery/consul/ConsulSimpleServiceDiscovery.scala

+    ResolvedTarget(catalogService.getServiceAddress, Some(port.getOrElse(catalogService.getServicePort)))
+  }
+
+  private val getServicesWithTags = ((callback: ConsulResponseCallback[util.Map[String, util.List[String]]]) =>


could this be a normal method instead?

ktoso · 2018-05-25T02:15:36Z

discovery-consul/src/main/scala/akka/discovery/consul/ConsulSimpleServiceDiscovery.scala

+  implicit class ConsulResponseFutureDecorator[T](f: ConsulResponseCallback[T] => Unit) {
+    def asFuture: Future[ConsulResponse[T]] = {
+      val callback = new ConsulResponseFutureCallback[T]
+      f(callback)


could f throw? Let's make sure that then the Future is failed rather than the asFuture throwing

I think I did what you asked for, but please take another look

…omments addressed

mlosiewicz-pl · 2018-05-28T08:59:55Z

About the registration method - I assume that most of the people doing this already have something in place like ConstructR and have a way of registering services in Consul ( for example if you use Nomad then that part is done for you ) so I would advise against adding a method for registration. The tags assigned to services are not key-value pairs but a set of strings, that's why I need prefixes. Also if we add a method for registration in this case then I would have to make sure that what I register is properly handled by everything set up by consul-template (it is common to set up load balancers based on services in consul) so that there is no duplication. Consul template is also the reason I added the port for akka-management, I don't want it to be accessible through load balancer.

I based the decisions on implementation on an actual production running service and it worked in my case (I mean, I deployed it on prod already ;) to check if it actually work) but in other cases it might not be accurate, but the generic approach with tags should work most of the times.

I will add the discovery.md as soon as I find some spare time and write it all down.

ktoso · 2018-05-28T09:14:18Z

About the registration method - I assume that most of the people doing this already have something in place like ConstructR and have a way of registering services in Consul ( for example if you use Nomad then that part is done for you ) so I would advise against adding a method for registration.

Note that this project replaces ConstructR, which is already in archived mode even: hseeberger/constructr#174

So, no, we can not say that people will have ConstructR or anything similar to it. We also assume deployments which are no-extra-tooling -- plain jars that someone has run somewhere so there should be standard way to register in such registries, even if many people will not use them because some other mechanism performs the registration for them.

mlosiewicz-pl · 2018-05-28T09:28:45Z

I meant that most of the setup for existing application is already done somewhere and since ConstructR does not register the service in Consul there has to be another mechanism in place already. Adding a registration method in my opinion is not easy (you have to create health checks and know what is exposed where in application - you probably have all the information anyway but still a generic solution would be hard to implement) and will be omitted most of the time.

Said that, I agree that this would be a powerful tool and would allow for quick set up of the cluster with low effort and very quickly. While implementing registration in Consul a lot of configuration options would be have to be added, but it's doable.

ktoso · 2018-06-01T02:03:45Z

That's a good summary – I think it's valuable, but we'll see when/if we end up providing it or not.

As for this PR, I would like to merge it but it needs a bit of documentation -- would you be able to add it? Thanks a lot

…tempt

mlosiewicz-pl · 2018-06-05T14:21:05Z

Sorry, I was quite busy with some other stuff.

I suck at writing documentation so please check it and if it does not make sense I'll try to write it in some other way.

ktoso · 2018-06-06T01:39:14Z

discovery-consul/discovery.md

@@ -0,0 +1,36 @@
+How to set up Consul based discovery for Akka Cluster


Docs in general are under the docs project :-)
https://github.com/akka/akka-management/blob/master/docs/src/main/paradox/discovery.md
Here no one would see the documentation after all :-)

Would you want to give adjusting it a try and putting in that spot?

Oops, I knew something was not right ... will add it there

…xed?

ktoso · 2018-06-10T10:32:14Z

Thanks! It's a good start :) I'll merge and we'll see what feedback we get. I'll also want to mark it somehow as community contriubted / "not supported" or something similar until we're confident in it... hope that's fine. Exact wording for these things we'll figure out during this week (we got a team meeting about it :))

Michal Losiewicz added 3 commits May 16, 2018 17:34

Consul based service discovery for Akka Management

e4f77cd

Consul based service discovery for Akka Management - reference config…

fd37b93

…uration comments update

Consul based service discovery for Akka Management - reformatted code

552d9a2

ktoso reviewed May 17, 2018

View reviewed changes

Michal Losiewicz added 3 commits May 17, 2018 08:55

Consul based service discovery for Akka Management - configured => co…

dc4542b

…nfigure

Consul based service discovery for Akka Management - increased patien…

370691b

…ce for embedded consul test

Consul based service discovery for Akka Management - increased patien…

4aa8a2a

…ce for embedded consul test

ktoso mentioned this pull request May 22, 2018

Proposing to deprecate this project lightbend/service-locator-dns#21

Closed

ktoso reviewed May 25, 2018

View reviewed changes

Consul based service discovery for Akka Management - review related c…

7f7702b

…omments addressed

Consul based service discovery for Akka Management - Documentation at…

9a17b9e

…tempt

ktoso reviewed Jun 6, 2018

View reviewed changes

Consul based service discovery for Akka Management - Documentation fi…

ba29496

…xed?

ktoso approved these changes Jun 10, 2018

View reviewed changes

ktoso merged commit d267ecf into akka:master Jun 10, 2018

		@@ -0,0 +1,36 @@
		How to set up Consul based discovery for Akka Cluster

Consul based service discovery #202

Consul based service discovery #202

Conversation

mlosiewicz-pl commented May 16, 2018

Choose a reason for hiding this comment

mlosiewicz-pl commented May 17, 2018

raboof commented May 17, 2018

ktoso commented May 20, 2018

ktoso commented May 24, 2018 • edited Loading

ktoso 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

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

mlosiewicz-pl commented May 28, 2018

ktoso commented May 28, 2018 • edited Loading

mlosiewicz-pl commented May 28, 2018

ktoso commented Jun 1, 2018

mlosiewicz-pl commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Jun 10, 2018

ktoso commented May 24, 2018 •

edited

Loading

ktoso commented May 28, 2018 •

edited

Loading