Add sticky port #291
Add sticky port #291
Conversation
|
In general this looks good, I'm curious though what the use case for this is? Is there a reason you need the port to be sticky? I worry that strengthening the assumption that a port may not change (although not specifically set as so) could cause friction/confusion for new users. |
| @@ -289,12 +301,14 @@ object Broker { | |||
| class Stickiness(_period: Period = new Period("10m")) { | |||
| var period: Period = _period | |||
| @volatile var hostname: String = null | |||
| @volatile var port: Integer = null | |||
There was a problem hiding this comment.
I would prefer an Option[Int] here rather than a nullable to make it more idiomatic scala.
| @volatile var stopTime: Date = null | ||
|
|
||
| def expires: Date = if (stopTime != null) new Date(stopTime.getTime + period.ms) else null | ||
|
|
||
| def registerStart(hostname: String): Unit = { | ||
| def registerStart(hostname: String, port: Integer): Unit = { |
There was a problem hiding this comment.
prefer Int over Integer
| @@ -175,11 +175,11 @@ class RangeDeserializer extends StdDeserializer[Range](classOf[Range]) { | |||
| } | |||
| } | |||
|
|
|||
| case class StickinessModel(period: Period, stopTime: Date, hostname: String) | |||
| case class StickinessModel(period: Period, stopTime: Date, hostname: String, port: Integer) | |||
There was a problem hiding this comment.
Ditto on Option[Int] over Integer
| @@ -19,7 +19,8 @@ | |||
| "syslog": false, | |||
| "stickiness": { | |||
| "period": "10m", | |||
| "hostname": "host1" | |||
| "hostname": "host1", | |||
| "port": 1234 | |||
There was a problem hiding this comment.
Leave this file as it is, its to test backwards compat with older (pre 0.10.0) serialized JSON.
| val usablePorts: List[Range] = | ||
| if (port == null) availablePorts.toList.sortBy(_.start) | ||
| else availablePorts.toList.flatMap { range => | ||
| if (range.overlap(port) == null) None else Some(range.overlap(port)) |
There was a problem hiding this comment.
this can be just Option(range.overlap(port))
|
@steveniemitz Thanks for the review, the reason we implemented this was that we wanted to minimize the need for clients to re-lookup the brokers from our service discovery + re-init the client after the brokers have been restarted, which may take a while in some cases. With sticky address the client can usually reconnect as soon as the broker is back up. I think it's quite reasonable to the user that they can either force the port they want to use in the config or they can let the framework pick an arbitrary port which may or may not be the same. I know it is a bit inconsistent when the hostname stickiness is enforced and the port one is not, but we thought it was practically more convenient. I'll update the doc to explain the differences so people don't get confused. I'll make the Option changes you suggested when I got some time (quite busy atm) and come back in a few days. |
This change will make broker remember previous port and prefer to restart on same port.
It is slightly different to the current hostname stickiness in that it is soft-stickiness, if the port is not available then it simply chooses the first port offered (same as current behavior) rather than rejecting the offer. It does not use the stickiness period to avoid overloading the config with unnecessary complexity.
Basically the existing hostname stickiness and config will behave exactly the same, but it will always pick previous port if available before choosing other ports.