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

WIP - Cluster bootstrap remoting probe method #546

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jroper
Copy link
Contributor

@jroper jroper commented May 20, 2019

Some initial work on #453. This has been done in an effort to try and slim down cluster bootstrap, both in terms of complexity and the amount of resources it uses.

This works, but there are a few things to think about.

  1. With this in place, we could decouple cluster bootstrap and Akka management HTTP. This would require pulling the HTTP probing method out into its own module, which would effectively make remoting the default.
  2. Currently the serializer for the seed nodes reuses the JSON serializers - we might need to reconsider this and use protobuf instead, this would in particular be a good idea if we decouple from Akka management HTTP as the JSON serialization means keeping a dependency on spray-json which could otherwise be dropped.
  3. While I've implemented unit tests that show that bootstrapping a cluster of 3 actor systems in the one process works, I haven't implemented any integration tests, eg on k8s, yet.

One nice thing is that remoting doesn't need the same hack that is used to discover the Akka management HTTP bind address.

@jroper
Copy link
Contributor Author

jroper commented May 29, 2019

Any thoughts on the points raised above? Should this go the full way and decouple cluster bootstrap from Akka management HTTP? Should the spray-json serializers used by remoting be replaced with protobuf serializers?

Also, currently the probing method has two hard coded options and is not pluggable, but it could be made such that we extract an interface that the two options can implement, and then allow other probing methods to be introduced. Not sure if that's a good idea or not, it would increase the effective public API and required semantics, whereas if we keep it as two hardcoded possibilities then that stuff can all stay internal which allows us to modify it as we see fit going forward.

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

Thanks for taking a first stab. I think in the long run we want to do this change but currently the Akka team would like to wait with this some more because we are busy with other things and it would be good to let akka-management be stable for a while before doing more substantial changes like this.

Regarding your questions:

Should this go the full way and decouple cluster bootstrap from Akka management HTTP?

I think we should go even further and remove the HTTP probing unless we have a good reason for supporting both. We would do change to remove complexity and that is not achieved by having two ways.

Should the spray-json serializers used by remoting be replaced with protobuf serializers?

Yes, less dependencies.

@@ -99,9 +99,14 @@ akka.management {
# Configured how we communicate with the contact point once it is discovered
contact-point {

# The probe method. Valid values are akka-management and remoting. If akka-management, will use akka-management
# HTTP interface to discover seed nodes. If remoting, will use Akka remoting to discover seed nodes.
probe-method = "akka-management"
Copy link
Member

Choose a reason for hiding this comment

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

why do we want to support both methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You won't be able to do a rolling upgrade to the new method if you don't support both concurrently.

.orElse(cluster.selfAddress.port)
.getOrElse(throw new IllegalArgumentException("Cannot infer port for contact point"))
val address = cluster.selfAddress.copy(host = Some(contactPoint.host), port = Some(targetPort))
(self.path.parent.parent / RemotingContactPoint.RemotingContactPointActorName).toStringWithAddress(address)
Copy link
Member

Choose a reason for hiding this comment

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

might be better with RootActorPath:

RootActorPath(address) / "system" / RemotingContactPoint.RemotingContactPointActorName

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants