CASSANDRA-20476 & CASSANDRA-20736 Handle CMS member addresses changing concurrently - #4613
CASSANDRA-20476 & CASSANDRA-20736 Handle CMS member addresses changing concurrently #4613beobal wants to merge 27 commits into
Conversation
beobal
changed the title
|
|
||
| public ImmutableSet<NodeId> joiningMembers() | ||
| { | ||
| return ImmutableSet.copyOf(joiningMembers); |
There was a problem hiding this comment.
nit; BTreeSet is already immutable, we probably don't need to copy it
|
|
||
| /** | ||
| * Used to derive a CMSMembership when deserializing a ClusterMetadata instance written with a metadata version | ||
| * prior to V7. At that time, CMS membership was always inferred from the data placements of the distributed |
| } | ||
|
|
||
| private final Map<NodeId, Pair<InetAddressAndPort, InetAddressAndPort>> overrides; | ||
| private final BiMap<InetAddressAndPort, InetAddressAndPort> addressMap; |
There was a problem hiding this comment.
addressMap is only used in the toString method
| return new InitialBuilder(metadata); | ||
| } | ||
|
|
||
| private final Map<NodeId, Pair<InetAddressAndPort, InetAddressAndPort>> overrides; |
There was a problem hiding this comment.
this should probably be an ImmutableMap for clarity?
and if we make InitialBuilder and rebuild below build immutablemaps we can avoid the copying
| return state == State.ACTIVE; | ||
| } | ||
|
|
||
| public InetAddressAndPort getAddressOverride(NodeId id) |
| else | ||
| { | ||
| // This cluster did not previously upgrade from a gossip based version (i.e. pre-6.0) but did at some point | ||
| // run a version prior to MetadataVersion.V7 where we started to encode CMS membership directly. This |
| // so we can derive the CMSMembership using the data placement and directory. | ||
| DataPlacement placement = placements.get(metadataKs.params.replication); | ||
| cmsMembership = CMSMembership.reconstruct(placement, dir); | ||
| placements = placements.unbuild().without(metadataKs.params.replication).build(); |
There was a problem hiding this comment.
I think this is unnecessary - we do the same thing directly after the if stmt
| int currentRound = 0; | ||
| long roundTimeNanos = Math.min(TimeUnit.SECONDS.toNanos(4), | ||
| DatabaseDescriptor.getDiscoveryTimeout(TimeUnit.NANOSECONDS) / maxRounds); | ||
| // TODO a non-CMS node only needs to be able to contact a single CMS member to commit its STARTUP |
There was a problem hiding this comment.
should we fix this? It feels like we'll most often discover the full CMS if its up
and if it is not yet up, it might be better to wait here before trying to commit Startup?
There was a problem hiding this comment.
I think discovering the full CMS in the common case is not really a problem.
To the second point, I don't think it makes an awful lot of difference but once we are sure we know a majority of CMS member addresses isn't it better to start trying to commit the Startup? If we wait for confirmation of addresses for the whole CMS we could end up being blocked unnecessarily by a single DOWN CMS member?
There was a problem hiding this comment.
right, I think I just wanted to remove the todo
|
|
||
| int maxRounds = 5; | ||
| int currentRound = 0; | ||
| long roundTimeNanos = Math.min(TimeUnit.SECONDS.toNanos(4), |
There was a problem hiding this comment.
is 4s enough here? Should we add another "discover survey" config setting?
| Map<NodeId, InetAddressAndPort> confirmedCMS = new HashMap<>(); | ||
|
|
||
| Set<InetAddressAndPort> candidates = new HashSet<>(previousCMS.values()); | ||
| candidates.add(newAddress); |
There was a problem hiding this comment.
any reason we don't add the seeds to candidates here? Feels like it could save us a discovery round
dc2fab7 to
2236ae1
Compare
krummas
left a comment
krummas
left a comment
There was a problem hiding this comment.
tiny comment inline, feel free to ignore
|
|
||
| public class NodeId implements Comparable<NodeId>, MultiStepOperation.SequenceKey | ||
| { | ||
| public static final NodeId UNREGISTERED = new NodeId(-1); |
There was a problem hiding this comment.
should we throw in toUUID below if we're unregistered? Just to make sure we don't accidentally write the unregistered UUID to a system table or something
2236ae1 to
b39dfd4
Compare
pmcfadin
added
the
needs-committer
…ed before allowing startup to proceed
…og processing is suspended
If two nodes restart with new broadcast addresses concurrently and one is a CMS member, the non-member may send a TCM_COMMIT_REQ containing its STARTUP transform to the CMS member's old address. If this is no longer reachable, the sender should time out quickly so it can resend to another CMS member, or try to discover the new CMS address(es).
…CEP-21, pre V9 version
…tructing a CMSLookup
b39dfd4 to
665839a
Compare
3 participants
Changing broadcast address has always been supported, but it requires the node to inform the CMS of the change at startup. If a majority of the CMS members attempt to do this concurrently, they have no way to establish the quorum required to make those metadata changes, leading to a deadlocked startup.
This is addressed by the combination of 2 patchsets: