CASSANDRA-21105 CEP-58: Add satellite replication strategy stub#4652
CASSANDRA-21105 CEP-58: Add satellite replication strategy stub#4652bdeggleston wants to merge 1 commit intoapache:cep-58-satellite-datacentersfrom
Conversation
| Preconditions.checkArgument(transientRF == 0 || transientRF < totalRF, | ||
| "Transient replicas must be zero, or less than total replication factor. For %s/%s", totalRF, transientRF); | ||
| Preconditions.checkArgument(transientRF <= totalRF, | ||
| "Transient replicas must be less than or equal to total replication factor. For %s/%s", totalRF, transientRF); |
There was a problem hiding this comment.
This is okay now, because a satellite DC is only witnesses? Does this conflict with the new check for non-zero fullReplicas above in ARS?
There was a problem hiding this comment.
Oh, is it because the ARS check is for the whole replication strategy, while this is for a single DC, which may be a satellite?
| int transientCount = Integer.parseInt(rfParts[1]); | ||
|
|
||
| if (transientCount != total) | ||
| cfgEx("Satellite replicas must all be witnesses. Expected '%s/%s' but got '%s'", total, total, value); |
There was a problem hiding this comment.
| cfgEx("Satellite replicas must all be witnesses. Expected '%s/%s' but got '%s'", total, total, value); | |
| cfgEx("Satellite replicas must all be witnesses. Expected '%d/%d' but got '%s'", total, total, value); |
| result = 31 * result + name.hashCode(); | ||
| result = 31 * result + rf.hashCode(); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Are there any tests that touch equals() and hashCode()?
| return false; | ||
|
|
||
| if (primary != null) | ||
| cfgEx("'primary' option specified multiple times"); |
There was a problem hiding this comment.
Superficially, test coverage looks pretty good. The only paths that aren't hit are the validation failures like this one, the similar things in validateExpectedOptions(), and client warnings in maybeWarnOnOptions(). We could test those explicitly if you think it would be valuable...
| if (!replicationType.isTracked()) | ||
| { | ||
| throw new ConfigurationException( | ||
| "SatelliteDatacenterReplicationStrategy requires tracked replication. " + |
There was a problem hiding this comment.
nit: Replace these w/ getClass().getSimpleName()?
| if (!knownDcs.contains(dc)) | ||
| { | ||
| throw new ConfigurationException(format( | ||
| "Unrecognized datacenter '%s'", dc)); |
There was a problem hiding this comment.
nit: We could include the DC names that do exist in the message.
| Guardrails.minimumReplicationFactor.guard( | ||
| rf.fullReplicas, keyspaceName, false, state); | ||
| Guardrails.maximumReplicationFactor.guard( | ||
| rf.fullReplicas, keyspaceName, false, state); |
There was a problem hiding this comment.
nit: I'd one-line these...100-wide is okay
|
|
||
| ReplicaGroups built = builder.build(); | ||
| return new DataPlacement(built, built); | ||
| } |
There was a problem hiding this comment.
I think the most pressing design question in my head here is whether there's more we can do to deduplicate with respect to the NTS stuff that already exists. I can see why you might not want to directly extend NetworkTopologyStrategy given how its constructor assigns datacenters and aggregateRf, but if we could share those bits of state, it seems like calculateNaturalReplicas(), calculateDataPlacement(), getReplicationFactor(), etc. could be shared.
maybeWarnOnOptions() has enough subtle differences that it might not be sharable directly, but I'd say the RF to node count comparison stuff might be.
There was a problem hiding this comment.
AbstractNetworkTopologyStrategy kind of sucks as a name...maybe DatacenterAwareReplicationStrategy? That would at least capture the major shared detail...
| * | ||
| * Uses NetworkTopologyStrategy replica selection algorithm for compatibility with existing NTS keyspaces | ||
| */ | ||
| public class SatelliteDatacenterReplicationStrategy extends AbstractReplicationStrategy |
There was a problem hiding this comment.
Let's bike-shed on this name, since it's going to be publicly facing, right? Alternatives:
SatelliteReplicationStrategy - Shorter, but would you get the impression it was only for satellite DCs themselves?
SatelliteTopologyStrategy - Going for a "topology that contains satellites" vibe
| this.primaryDC = opts.primary; | ||
| this.disabledDCs = Collections.unmodifiableSet(opts.disabledDCs); | ||
|
|
||
| validate(); |
There was a problem hiding this comment.
What if we moved this validate() method to the StrategyOptions class itself, given that all operates over fields in that class anyway? We could also move that opts.validate() call before the assignments above.
| return true; | ||
| } | ||
|
|
||
| boolean setDisabled(String key, String value) |
There was a problem hiding this comment.
nit: What about renaming setSatellite/setDisabled -> addSatellite/addDisabled?
|
|
||
| allDatacenters.putAll(fullDatacenters); | ||
| satellites.forEach((dc, info) -> allDatacenters.put(dc, info.rf)); | ||
| } |
There was a problem hiding this comment.
nit: What if we just had a StrategyOptions.fromConfigMap() or something that just fed immutable stuff to the actual StrategyOptions constructor. Not sure if we really care about creating StrategyOptions directly for tests or whatever, but that would be easier too...
There was a problem hiding this comment.
i.e. StrategyOptions would have final fields and could even be used directly as a field for the strategy class itself?
| String satelliteName = parts[1]; | ||
|
|
||
| if (parentDc.contains(".") || satelliteName.contains(".")) | ||
| cfgEx("Datacenter names cannot contain dots: '%s'", key); |
There was a problem hiding this comment.
nit: Message is duplicated on 243
|
|
||
| String[] rfParts = value.split("/"); | ||
| if (rfParts.length != 2) | ||
| cfgEx("Invalid replication factor format for satellite '%s': '%s'", satelliteName, value); |
There was a problem hiding this comment.
nit: Message duplicated on 229
maedhroz
left a comment
maedhroz
left a comment
There was a problem hiding this comment.
Left a bunch of comments that are mostly structural in nature and not correctness-related. The flat configuration structure is fine with me, but I don't know if we need to vet it elsewhere.
2 participants
No description provided.