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
Redirect traffic to slaves when master is down #284
Conversation
new Replica(token,member.node,member.status == MemberStatus.Up) :: explicitTokenMapping.get(token).getOrElse(List()).filterNot(_ == member.node).map(new Replica(token, _)) | ||
val masterReplica = new Replica(member.token, member.node, selected = member.status == MemberStatus.Up) | ||
val slaveReplicas = explicitTokenMapping.get(member.token).map { nodes => | ||
nodes.filterNot(_ == member.node).map(new Replica(member.token, _)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodes collect {
case node if node != member.node => new Replica(member.token, node)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. I just broke down existing code without really looking at it.
I feel like there might be too many synchronized blocks in the same class file. Starting to feel like it's code smell... |
def start(): Unit = { | ||
synchronized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, there should be no concurrency at this point, so the synchronized
was pointless.
Agents looks even better that I would have guessed! |
@alexbergeron Agents are great, but I'm frustrated that it doesn't give a way of encapsulating update methods in it. I came up with an |
Looks good to me, waiting for @mlaflamm's review |
private val childWatches = CacheBuilder.newBuilder().weakValues.build[Function1[NodeChildrenChanged, Unit], Watcher] | ||
private val dataWatches = CacheBuilder.newBuilder().weakValues.build[NodeValueChanged => Unit, Watcher] | ||
private val childWatches = CacheBuilder.newBuilder().weakValues.build[NodeChildrenChanged => Unit, Watcher] | ||
private val createWatches = CacheBuilder.newBuilder().weakValues.build[NodeStatusChanged => Unit, Watcher] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createWatches
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like any of those 3 val names. I think they should be consistent with the event types, but nodeStatusChangedWatches
would be a bit java-ish… what's your take?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps existsWatches
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suggest renaming dataWatches
to valueWatches
("data" is such a generic term).
+1 |
Redirect traffic to slaves when master is down
Avoids sending traffic to stale replicas using the replication lag stored in Zookeeper.
Manually tested since we don't have integration tests for
ConsistencyMasterSlave
… yet.