[Feature] Identity carry over part 5 / Multi ring checksum #192
Conversation
also make sure the local labels map is immutable
… is reflected on the ring
thanodnl
force-pushed
the
feature/identity-carry-over
branch
from
4226109
to
92cde55
Compare
thanodnl
force-pushed
the
feature/ring-multi-checksum
branch
from
c0e2e08
to
b5b7a47
Compare
| type Checksum interface { | ||
| // Compute calculates the checksum for the hashring that is passed in. | ||
| // Compute will be called while having atleast a readlock on the hashring so | ||
| // it is safe to read from the ring, but not safe to chagne the ring. There |
| ring.tree.root.walk(func(node *redBlackNode) bool { | ||
| buffer.WriteString(strconv.Itoa(node.key.(replicaPoint).point)) | ||
| buffer.WriteString("-") | ||
| buffer.WriteString(node.value.(string)) |
There was a problem hiding this comment.
Doesn't it make more sense to use node.key.identity instead of node.value here?
ignore this
| if r.checksum != old { | ||
| func (r *HashRing) Checksums() (checksums map[string]uint32) { | ||
| r.RLock() | ||
| // even though the map is immutable the pointer to it is not so it requires |
There was a problem hiding this comment.
Should we add a check if checksums are already calculated and compute them if not?
| r.logger.WithFields(bark.Fields{ | ||
| "checksum": r.checksum, | ||
| "checksum": r.legacyChecksum, | ||
| "oldChecksum": old, |
There was a problem hiding this comment.
Do we want to log the other checksums?
| r.checksums[name] = checksummer.Compute(r) | ||
| } | ||
|
|
||
| // calculate the legacy identy only based checksum |
| // Compute will be called while having atleast a readlock on the hashring so | ||
| // it is safe to read from the ring, but not safe to chagne the ring. There | ||
| // might be multiple Checksum Computes initiated at the same time, but every | ||
| // Checksum will only be called once per hashring at once |
There was a problem hiding this comment.
every Checksum will only be called once per hashring at once
not sure if we should define this behaviour. Why restrict us when not needed?
|
|
||
| // addressChecksum calculates checksums for all addresses that are added to the | ||
| // hashring. | ||
| type addressChecksum struct{} |
There was a problem hiding this comment.
I don't think we use nor need the old style addressChecksum
|
|
||
| // Checksum computes a checksum for an instance of a HashRing. The | ||
| // checksum can be used to compare two rings for equality. | ||
| type Checksum interface { |
There was a problem hiding this comment.
I don't think we should have an interface here. A simple func Compute(ring *HashRing) function would be sufficient. Any reason we want this interface?
Also the interface name Checksumer with a Checksum method would be more idiomatic I think.
There was a problem hiding this comment.
I don't agree here. A function can also implement an interface when needed, but if you want to use a struct it is harder if you don't use an interface as input.
The naming you are right on, we should call the interface Checksummer.
moved addressChecksummer to a test file since it’s only used for validation backwards compatibility.
mennopruijssers
force-pushed
the
feature/ring-multi-checksum
branch
from
f07b83c
to
74de8fe
Compare
mennopruijssers
changed the base branch from
feature/identity-carry-over
to
dev
This PR is part 5 for identity carry over.
In this PR a new mechanism for calculating ring checksums is added. It aims to be forward compatible by naming checksums after the algorithm used so that new algorithms can be added and old algorithms can be removed when appropriate.
At the same time the old algorithm will start its EOL and all references to that are renamed to indicate legacy. The old algorithm does not capture all aspects of the hashring and will not warn about programmer error in the hashring.
The newly added
replicachecksummer will walk the ring from the beginning to the end and capture all significant datapoints of areplicapointin its checksum that should prevent two rings that are out of sync to provide the same checksum.