From 6eca742333b194a5e980463ba4de287d087f36b4 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Mon, 28 Nov 2016 19:07:31 +0100 Subject: [PATCH 01/19] abstract checksums into multiple named checksums. --- hashring/checksum.go | 34 +++++++++++++++++++++ hashring/hashring.go | 71 ++++++++++++++++++++++++++++---------------- stats_handler.go | 5 ++-- 3 files changed, 83 insertions(+), 27 deletions(-) create mode 100644 hashring/checksum.go diff --git a/hashring/checksum.go b/hashring/checksum.go new file mode 100644 index 00000000..3b0443b8 --- /dev/null +++ b/hashring/checksum.go @@ -0,0 +1,34 @@ +package hashring + +import ( + "sort" + "strings" + + "github.com/dgryski/go-farm" +) + +// Checksum computes a checksum for an instance of a HashRing. The +// checksum can be used to compare two rings for equality. +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 + // might be multiple Checksum Computes initiated at the same time, but every + // Checksum will only be called once per hashring at once + Compute(ring *HashRing) (checksum uint32) +} + +// addressChecksum calculates checksums for all addresses that are added to the +// hashring. +// TODO calculate the checksum on identities. This will be backwards compatible. +// The best case would be to iterate the rbtree in order and hash its content so +// we have more certainty that the rings are looking up the same way but that +// will be a backwards incompatible change +type addressChecksum struct{} + +func (i *addressChecksum) Compute(ring *HashRing) uint32 { + addresses := ring.copyServersNoLock() + sort.Strings(addresses) + bytes := []byte(strings.Join(addresses, ";")) + return farm.Fingerprint32(bytes) +} diff --git a/hashring/hashring.go b/hashring/hashring.go index ee620de7..251ad02f 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -24,16 +24,12 @@ package hashring import ( "fmt" - "sort" - "strings" "sync" "github.com/uber-common/bark" "github.com/uber/ringpop-go/events" "github.com/uber/ringpop-go/logging" "github.com/uber/ringpop-go/membership" - - "github.com/dgryski/go-farm" ) // Configuration is a configuration struct that can be passed to the @@ -67,7 +63,17 @@ type HashRing struct { serverSet map[string]struct{} tree *redBlackTree - checksum uint32 + + // checksummers is map of named Checksum calculators for the hashring + checksummers map[string]Checksum + // checksums is a map containing the checksums that are representing this + // hashring. The map should never be altered in place so it is safe to pass + // a copy to components that need the checksums + checksums map[string]uint32 + // defaultChecksum is the name of the Checksum calculator that is used as + // the default checksum for the hashring. The default checksum is returned + // when asked for only 1 checksum and is present for legacy reasons + defaultChecksum string logger bark.Logger } @@ -79,9 +85,13 @@ func New(hashfunc func([]byte) uint32, replicaPoints int) *HashRing { hashfunc: func(str string) int { return int(hashfunc([]byte(str))) }, - logger: logging.Logger("ring"), + logger: logging.Logger("ring"), + checksummers: make(map[string]Checksum), } + r.checksummers["address"] = &addressChecksum{} + r.defaultChecksum = "address" + r.serverSet = make(map[string]struct{}) r.tree = &redBlackTree{} return r @@ -91,30 +101,41 @@ func New(hashfunc func([]byte) uint32, replicaPoints int) *HashRing { // Use this value to find out if the HashRing is mutated. func (r *HashRing) Checksum() uint32 { r.RLock() - checksum := r.checksum + checksum := r.checksums[r.defaultChecksum] r.RUnlock() return checksum } -// computeChecksum computes checksum of all servers in the ring. -// This function isn't thread-safe, only call it when the HashRing is locked. -func (r *HashRing) computeChecksumNoLock() { - addresses := r.copyServersNoLock() - sort.Strings(addresses) - bytes := []byte(strings.Join(addresses, ";")) - old := r.checksum - r.checksum = farm.Fingerprint32(bytes) - - 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 + // a readlock + checksums = r.checksums + r.RUnlock() + return +} + +// computeChecksumsNoLock re-computes all configured checksums for this hashring +// and updates the in memory map with a new map containing the new checksums. +func (r *HashRing) computeChecksumsNoLock() { + oldChecksums := r.checksums + r.checksums = make(map[string]uint32) + + // calculate all configured checksums + for name, checksummer := range r.checksummers { + r.checksums[name] = checksummer.Compute(r) + } + + if r.checksums[r.defaultChecksum] != oldChecksums[r.defaultChecksum] { r.logger.WithFields(bark.Fields{ - "checksum": r.checksum, - "oldChecksum": old, + "checksum": r.checksums[r.defaultChecksum], + "oldChecksum": oldChecksums[r.defaultChecksum], }).Debug("ringpop ring computed new checksum") } r.EmitEvent(events.RingChecksumEvent{ - OldChecksum: old, - NewChecksum: r.checksum, + OldChecksum: oldChecksums[r.defaultChecksum], + NewChecksum: r.checksums[r.defaultChecksum], }) } @@ -132,7 +153,7 @@ func (r *HashRing) AddServer(server membership.Member) bool { r.Lock() ok := r.addServerNoLock(server) if ok { - r.computeChecksumNoLock() + r.computeChecksumsNoLock() r.EmitEvent(events.RingChangedEvent{ ServersAdded: []string{server.GetAddress()}, ServersRemoved: nil, @@ -168,7 +189,7 @@ func (r *HashRing) RemoveServer(server membership.Member) bool { r.Lock() ok := r.removeServerNoLock(server) if ok { - r.computeChecksumNoLock() + r.computeChecksumsNoLock() r.EmitEvent(events.RingChangedEvent{ ServersAdded: nil, ServersRemoved: []string{server.GetAddress()}, @@ -222,7 +243,7 @@ func (r *HashRing) ProcessMembershipChangesServers(changes []membership.MemberCh // recompute checksums on changes if changed { - r.computeChecksumNoLock() + r.computeChecksumsNoLock() } r.Unlock() @@ -257,7 +278,7 @@ func (r *HashRing) addRemoveServersNoLock(add []membership.Member, remove []memb } if changed { - r.computeChecksumNoLock() + r.computeChecksumsNoLock() r.EmitEvent(events.RingChangedEvent{ ServersAdded: added, ServersRemoved: removed, diff --git a/stats_handler.go b/stats_handler.go index 0f8de9c0..d8e11b7c 100644 --- a/stats_handler.go +++ b/stats_handler.go @@ -52,8 +52,9 @@ func handleStats(rp *Ringpop) map[string]interface{} { }, "protocol": rp.node.ProtocolStats(), "ring": stats{ - "servers": servers, - "checksum": rp.ring.Checksum(), + "servers": servers, + "checksum": rp.ring.Checksum(), + "checksums": rp.ring.Checksums(), }, "version": "???", // TODO: version! "timestamp": time.Now().Unix(), From ae6966577348e19c8e650dfd17d50ecbb23b6962 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Mon, 28 Nov 2016 19:10:01 +0100 Subject: [PATCH 02/19] Add replica hashring checksummed. --- hashring/checksum.go | 27 +++++++++++++++++++++++++++ hashring/hashring.go | 3 ++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/hashring/checksum.go b/hashring/checksum.go index 3b0443b8..ef2b810c 100644 --- a/hashring/checksum.go +++ b/hashring/checksum.go @@ -1,7 +1,9 @@ package hashring import ( + "bytes" "sort" + "strconv" "strings" "github.com/dgryski/go-farm" @@ -32,3 +34,28 @@ func (i *addressChecksum) Compute(ring *HashRing) uint32 { bytes := []byte(strings.Join(addresses, ";")) return farm.Fingerprint32(bytes) } + +type replicapointChecksum struct{} + +func (r *replicapointChecksum) Compute(ring *HashRing) uint32 { + buffer := bytes.Buffer{} + + visitNodes(ring.tree.root, func(node *redBlackNode) { + buffer.WriteString(strconv.Itoa(node.key.(replicapoint).point)) + buffer.WriteString("-") + buffer.WriteString(node.value.(string)) + buffer.WriteString(";") + }) + + return farm.Fingerprint32(buffer.Bytes()) +} + +func visitNodes(node *redBlackNode, visitor func(node *redBlackNode)) { + if node == nil { + return + } + + visitNodes(node.left, visitor) + visitor(node) + visitNodes(node.right, visitor) +} diff --git a/hashring/hashring.go b/hashring/hashring.go index 251ad02f..4a733998 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -89,8 +89,9 @@ func New(hashfunc func([]byte) uint32, replicaPoints int) *HashRing { checksummers: make(map[string]Checksum), } - r.checksummers["address"] = &addressChecksum{} r.defaultChecksum = "address" + r.checksummers["address"] = &addressChecksum{} + r.checksummers["replica"] = &replicapointChecksum{} r.serverSet = make(map[string]struct{}) r.tree = &redBlackTree{} From e99f2d7c5e03d949c4994942fc15e19bccac996d Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Mon, 28 Nov 2016 14:25:17 +0100 Subject: [PATCH 03/19] Add ring checksummer that is compatible with current released version. --- hashring/checksum.go | 28 +++++++++++++++++++++------- hashring/hashring.go | 14 ++++++++------ 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/hashring/checksum.go b/hashring/checksum.go index ef2b810c..ae22eab4 100644 --- a/hashring/checksum.go +++ b/hashring/checksum.go @@ -22,10 +22,6 @@ type Checksum interface { // addressChecksum calculates checksums for all addresses that are added to the // hashring. -// TODO calculate the checksum on identities. This will be backwards compatible. -// The best case would be to iterate the rbtree in order and hash its content so -// we have more certainty that the rings are looking up the same way but that -// will be a backwards incompatible change type addressChecksum struct{} func (i *addressChecksum) Compute(ring *HashRing) uint32 { @@ -35,13 +31,31 @@ func (i *addressChecksum) Compute(ring *HashRing) uint32 { return farm.Fingerprint32(bytes) } -type replicapointChecksum struct{} +type identityChecksum struct{} -func (r *replicapointChecksum) Compute(ring *HashRing) uint32 { +func (i *identityChecksum) Compute(ring *HashRing) uint32 { + identitySet := make(map[string]struct{}) + visitNodes(ring.tree.root, func(node *redBlackNode) { + identitySet[node.key.(replicaPoint).identity] = struct{}{} + }) + + identities := make([]string, 0, len(identitySet)) + for identity := range identitySet { + identities = append(identities, identity) + } + + sort.Strings(identities) + bytes := []byte(strings.Join(identities, ";")) + return farm.Fingerprint32(bytes) +} + +type replicaPointChecksum struct{} + +func (r *replicaPointChecksum) Compute(ring *HashRing) uint32 { buffer := bytes.Buffer{} visitNodes(ring.tree.root, func(node *redBlackNode) { - buffer.WriteString(strconv.Itoa(node.key.(replicapoint).point)) + buffer.WriteString(strconv.Itoa(node.key.(replicaPoint).point)) buffer.WriteString("-") buffer.WriteString(node.value.(string)) buffer.WriteString(";") diff --git a/hashring/hashring.go b/hashring/hashring.go index 4a733998..4a337803 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -85,13 +85,15 @@ func New(hashfunc func([]byte) uint32, replicaPoints int) *HashRing { hashfunc: func(str string) int { return int(hashfunc([]byte(str))) }, - logger: logging.Logger("ring"), - checksummers: make(map[string]Checksum), - } + logger: logging.Logger("ring"), - r.defaultChecksum = "address" - r.checksummers["address"] = &addressChecksum{} - r.checksummers["replica"] = &replicapointChecksum{} + defaultChecksum: "address", + checksummers: map[string]Checksum{ + "address": &addressChecksum{}, + "identity": &identityChecksum{}, + "replica": &replicaPointChecksum{}, + }, + } r.serverSet = make(map[string]struct{}) r.tree = &redBlackTree{} From ef8a2e49bf5c6ac81f6f04e9abccdc55fce17674 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Mon, 28 Nov 2016 19:12:58 +0100 Subject: [PATCH 04/19] Use tree walker to calculate checksums. --- hashring/checksum.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/hashring/checksum.go b/hashring/checksum.go index ae22eab4..8568ec6b 100644 --- a/hashring/checksum.go +++ b/hashring/checksum.go @@ -35,8 +35,9 @@ type identityChecksum struct{} func (i *identityChecksum) Compute(ring *HashRing) uint32 { identitySet := make(map[string]struct{}) - visitNodes(ring.tree.root, func(node *redBlackNode) { + ring.tree.root.walk(func(node *redBlackNode) bool { identitySet[node.key.(replicaPoint).identity] = struct{}{} + return true }) identities := make([]string, 0, len(identitySet)) @@ -54,22 +55,13 @@ type replicaPointChecksum struct{} func (r *replicaPointChecksum) Compute(ring *HashRing) uint32 { buffer := bytes.Buffer{} - visitNodes(ring.tree.root, func(node *redBlackNode) { + ring.tree.root.walk(func(node *redBlackNode) bool { buffer.WriteString(strconv.Itoa(node.key.(replicaPoint).point)) buffer.WriteString("-") buffer.WriteString(node.value.(string)) buffer.WriteString(";") + return true }) return farm.Fingerprint32(buffer.Bytes()) } - -func visitNodes(node *redBlackNode, visitor func(node *redBlackNode)) { - if node == nil { - return - } - - visitNodes(node.left, visitor) - visitor(node) - visitNodes(node.right, visitor) -} From 3c4c437f1eded57e9170ca0a7ad7a83773e2d065 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Mon, 28 Nov 2016 15:03:34 +0100 Subject: [PATCH 05/19] removed the legacy checksum from the map and call it legacy explicitly. --- hashring/hashring.go | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/hashring/hashring.go b/hashring/hashring.go index 4a337803..7f76b425 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -64,16 +64,16 @@ type HashRing struct { serverSet map[string]struct{} tree *redBlackTree + // legacyChecksum is the old checksum that is calculated only from the + // identities being added. + legacyChecksum uint32 + // checksummers is map of named Checksum calculators for the hashring checksummers map[string]Checksum // checksums is a map containing the checksums that are representing this // hashring. The map should never be altered in place so it is safe to pass // a copy to components that need the checksums checksums map[string]uint32 - // defaultChecksum is the name of the Checksum calculator that is used as - // the default checksum for the hashring. The default checksum is returned - // when asked for only 1 checksum and is present for legacy reasons - defaultChecksum string logger bark.Logger } @@ -87,11 +87,8 @@ func New(hashfunc func([]byte) uint32, replicaPoints int) *HashRing { }, logger: logging.Logger("ring"), - defaultChecksum: "address", checksummers: map[string]Checksum{ - "address": &addressChecksum{}, - "identity": &identityChecksum{}, - "replica": &replicaPointChecksum{}, + "replica": &replicaPointChecksum{}, }, } @@ -102,11 +99,11 @@ func New(hashfunc func([]byte) uint32, replicaPoints int) *HashRing { // Checksum returns the checksum of all stored servers in the HashRing // Use this value to find out if the HashRing is mutated. -func (r *HashRing) Checksum() uint32 { +func (r *HashRing) Checksum() (checksum uint32) { r.RLock() - checksum := r.checksums[r.defaultChecksum] + checksum = r.legacyChecksum r.RUnlock() - return checksum + return } func (r *HashRing) Checksums() (checksums map[string]uint32) { @@ -121,7 +118,6 @@ func (r *HashRing) Checksums() (checksums map[string]uint32) { // computeChecksumsNoLock re-computes all configured checksums for this hashring // and updates the in memory map with a new map containing the new checksums. func (r *HashRing) computeChecksumsNoLock() { - oldChecksums := r.checksums r.checksums = make(map[string]uint32) // calculate all configured checksums @@ -129,16 +125,21 @@ func (r *HashRing) computeChecksumsNoLock() { r.checksums[name] = checksummer.Compute(r) } - if r.checksums[r.defaultChecksum] != oldChecksums[r.defaultChecksum] { + // calculate the legacy identy only based checksum + legacy := identityChecksum{} + old := r.legacyChecksum + r.legacyChecksum = legacy.Compute(r) + + if old != r.legacyChecksum { r.logger.WithFields(bark.Fields{ - "checksum": r.checksums[r.defaultChecksum], - "oldChecksum": oldChecksums[r.defaultChecksum], + "checksum": r.legacyChecksum, + "oldChecksum": old, }).Debug("ringpop ring computed new checksum") } r.EmitEvent(events.RingChecksumEvent{ - OldChecksum: oldChecksums[r.defaultChecksum], - NewChecksum: r.checksums[r.defaultChecksum], + OldChecksum: old, + NewChecksum: r.legacyChecksum, }) } From b5b7a470d47073dfc2bee160a283b60679793af6 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Mon, 28 Nov 2016 15:54:24 +0100 Subject: [PATCH 06/19] emit named checksum periodically --- ringpop.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/ringpop.go b/ringpop.go index acfa4bab..1153497c 100644 --- a/ringpop.go +++ b/ringpop.go @@ -230,7 +230,8 @@ func (rp *Ringpop) startTimers() { rp.statter.UpdateGauge( rp.getStatKey("membership.checksum-periodic"), nil, - int64(rp.node.GetChecksum())) + int64(rp.node.GetChecksum()), + ) } }() } @@ -243,7 +244,17 @@ func (rp *Ringpop) startTimers() { rp.statter.UpdateGauge( rp.getStatKey("ring.checksum-periodic"), nil, - int64(rp.ring.Checksum())) + int64(rp.ring.Checksum()), + ) + + // emit all named checksums aswell + for name, checksum := range rp.ring.Checksums() { + rp.statter.UpdateGauge( + rp.getStatKey("ring.checksum-periodic."+name), + nil, + int64(checksum), + ) + } } }() } From 68f0b862b179eed68e89ee4408075e98c9cb6ad1 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Fri, 2 Dec 2016 15:07:13 +0100 Subject: [PATCH 07/19] Add documentation to make 'make lint' happy --- hashring/hashring.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hashring/hashring.go b/hashring/hashring.go index 4e984c39..18e2da7f 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -107,6 +107,8 @@ func (r *HashRing) Checksum() (checksum uint32) { return } +// Checksums returns a map of checksums named by the algorithm used to compute +// the checksum. 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 From 265c6c2e16331942dce71d7bea38eb350a6c6371 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Mon, 5 Dec 2016 14:43:57 +0100 Subject: [PATCH 08/19] Fixes after merge --- hashring/checksum.go | 2 +- hashring/hashring.go | 10 ++++++---- swim/member_test.go | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/hashring/checksum.go b/hashring/checksum.go index 8568ec6b..a5749df0 100644 --- a/hashring/checksum.go +++ b/hashring/checksum.go @@ -56,7 +56,7 @@ func (r *replicaPointChecksum) Compute(ring *HashRing) uint32 { buffer := bytes.Buffer{} ring.tree.root.walk(func(node *redBlackNode) bool { - buffer.WriteString(strconv.Itoa(node.key.(replicaPoint).point)) + buffer.WriteString(strconv.Itoa(node.key.(replicaPoint).hash)) buffer.WriteString("-") buffer.WriteString(node.value.(string)) buffer.WriteString(";") diff --git a/hashring/hashring.go b/hashring/hashring.go index 5278d73b..19de9549 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -44,8 +44,9 @@ type Configuration struct { } type replicaPoint struct { - hash int - address string + hash int + identity string + address string } func (r replicaPoint) Compare(other interface{}) int { @@ -148,8 +149,9 @@ func (r *HashRing) computeChecksumsNoLock() { func (r *HashRing) replicaPointForServer(server membership.Member, replica int) replicaPoint { replicaStr := fmt.Sprintf("%s%v", server.Identity(), replica) return replicaPoint{ - hash: r.hashfunc(replicaStr), - address: server.GetAddress(), + hash: r.hashfunc(replicaStr), + identity: server.Identity(), + address: server.GetAddress(), } } diff --git a/swim/member_test.go b/swim/member_test.go index 3bfe4d0d..a93d0b0b 100644 --- a/swim/member_test.go +++ b/swim/member_test.go @@ -175,7 +175,7 @@ func TestIdentityWithoutLabel(t *testing.T) { } func TestIdentityWithIdentityLabel(t *testing.T) { - m := Member{ + m := Member{ Address: "192.0.2.1:1234", Labels: LabelMap{ "__identity": "identityFromLabel", From a047ef84b2e79b900306827e1cef10e7c541e3a3 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Mon, 5 Dec 2016 14:49:00 +0100 Subject: [PATCH 09/19] =?UTF-8?q?typo=E2=80=99s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- hashring/checksum.go | 4 ++-- hashring/hashring.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hashring/checksum.go b/hashring/checksum.go index a5749df0..e17102ac 100644 --- a/hashring/checksum.go +++ b/hashring/checksum.go @@ -13,8 +13,8 @@ import ( // checksum can be used to compare two rings for equality. 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 + // Compute will be called while having at least a read-lock on the hashring so + // it is safe to read from the ring, but not safe to change 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 Compute(ring *HashRing) (checksum uint32) diff --git a/hashring/hashring.go b/hashring/hashring.go index 19de9549..5d4d4af8 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -128,7 +128,7 @@ func (r *HashRing) computeChecksumsNoLock() { r.checksums[name] = checksummer.Compute(r) } - // calculate the legacy identy only based checksum + // calculate the legacy identity only based checksum legacy := identityChecksum{} old := r.legacyChecksum r.legacyChecksum = legacy.Compute(r) From 399aa96bf4381b88f7fdf353a80962885a54396c Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Mon, 5 Dec 2016 14:49:12 +0100 Subject: [PATCH 10/19] also log checksums --- hashring/hashring.go | 1 + 1 file changed, 1 insertion(+) diff --git a/hashring/hashring.go b/hashring/hashring.go index 5d4d4af8..d92a9b49 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -137,6 +137,7 @@ func (r *HashRing) computeChecksumsNoLock() { r.logger.WithFields(bark.Fields{ "checksum": r.legacyChecksum, "oldChecksum": old, + "checksums": r.checksums, }).Debug("ringpop ring computed new checksum") } From 6c030dc7ba8448549f1dd5640ce0ccee1724f9a2 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Mon, 5 Dec 2016 16:05:43 +0100 Subject: [PATCH 11/19] Add checksums to RingChecksumEvent --- events/events.go | 6 ++++++ hashring/hashring.go | 33 ++++++++++++++++++++++++--------- ringpop.go | 3 +++ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/events/events.go b/events/events.go index b9bc5336..e92e24fd 100644 --- a/events/events.go +++ b/events/events.go @@ -166,8 +166,14 @@ type RingChangedEvent struct { // RingChecksumEvent is sent when a server is removed or added and a new checksum // for the ring is calculated type RingChecksumEvent struct { + // OldChecksum contains the previous legacy checksum. Note: might be deprecated in the future. OldChecksum uint32 + // NewChecksum contains the new legacy checksum. Note: might be deprecated in the future. NewChecksum uint32 + // OldChecksums contains the map of previous checksums + OldChecksums map[string]uint32 + // NewChecksums contains the map with new checksums + NewChecksums map[string]uint32 } // A LookupEvent is sent when a lookup is performed on the Ringpop's ring diff --git a/hashring/hashring.go b/hashring/hashring.go index d92a9b49..c8af79f5 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -121,29 +121,44 @@ func (r *HashRing) Checksums() (checksums map[string]uint32) { // computeChecksumsNoLock re-computes all configured checksums for this hashring // and updates the in memory map with a new map containing the new checksums. func (r *HashRing) computeChecksumsNoLock() { - r.checksums = make(map[string]uint32) + oldChecksums := r.checksums + r.checksums = make(map[string]uint32) + changed := false // calculate all configured checksums for name, checksummer := range r.checksummers { - r.checksums[name] = checksummer.Compute(r) + oldChecksum := oldChecksums[name] + newChecksum := checksummer.Compute(r) + r.checksums[name] = newChecksum + + if oldChecksum != newChecksum { + changed = true + } } // calculate the legacy identity only based checksum - legacy := identityChecksum{} - old := r.legacyChecksum - r.legacyChecksum = legacy.Compute(r) + legacyChecksummer := identityChecksum{} + oldChecksum := r.legacyChecksum + newChecksum := legacyChecksummer.Compute(r) + r.legacyChecksum = newChecksum + + if oldChecksum != newChecksum { + changed = true + } - if old != r.legacyChecksum { + if changed { r.logger.WithFields(bark.Fields{ "checksum": r.legacyChecksum, - "oldChecksum": old, + "oldChecksum": oldChecksum, "checksums": r.checksums, }).Debug("ringpop ring computed new checksum") } r.EmitEvent(events.RingChecksumEvent{ - OldChecksum: old, - NewChecksum: r.legacyChecksum, + OldChecksum: oldChecksum, + NewChecksum: r.legacyChecksum, + OldChecksums: oldChecksums, + NewChecksums: r.checksums, }) } diff --git a/ringpop.go b/ringpop.go index e8acb5ac..b4879e88 100644 --- a/ringpop.go +++ b/ringpop.go @@ -575,6 +575,9 @@ func (rp *Ringpop) HandleEvent(event events.Event) { case events.RingChecksumEvent: rp.statter.IncCounter(rp.getStatKey("ring.checksum-computed"), nil, 1) rp.statter.UpdateGauge(rp.getStatKey("ring.checksum"), nil, int64((event.NewChecksum))) + for key, value := range event.NewChecksums { + rp.statter.UpdateGauge(rp.getStatKey("ring.checksums."+key), nil, int64(value)) + } case events.RingChangedEvent: added := int64(len(event.ServersAdded)) From 938be77c367a4ba965504c1eb76ade99401c6434 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Tue, 6 Dec 2016 11:22:39 +0100 Subject: [PATCH 12/19] Add checksum tests --- hashring/checksum_test.go | 85 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 hashring/checksum_test.go diff --git a/hashring/checksum_test.go b/hashring/checksum_test.go new file mode 100644 index 00000000..62344cea --- /dev/null +++ b/hashring/checksum_test.go @@ -0,0 +1,85 @@ +// Copyright (c) 2015 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package hashring + +import ( + "sort" + "strings" + "testing" + + "code.uber.internal/rt/cag.git/Godeps/_workspace/src/code.uber.internal/go-common.git/Godeps/_workspace/src/github.com/dgryski/go-farm" + "github.com/stretchr/testify/assert" +) + +func TestAddressChecksum_Compute(t *testing.T) { + members := genMembers(1, 1, 10, false) + ring := New(farm.Fingerprint32, 1) + ring.AddMembers(members...) + checksum := &addressChecksum{} + + addresses := make([]string, 0, 10) + for _, members := range members { + addresses = append(addresses, members.GetAddress()) + } + + sort.Strings(addresses) + bytes := []byte(strings.Join(addresses, ";")) + + expected := farm.Fingerprint32(bytes) + actual := checksum.Compute(ring) + + assert.Equal(t, expected, actual) +} + +func TestIdentityChecksum_Compute(t *testing.T) { + identityChecksummer := &identityChecksum{} + + ringWithoutIdentities := New(farm.Fingerprint32, 1) + ringWithoutIdentities.AddMembers(genMembers(1, 1, 10, false)...) + + legacyChecksum := (&addressChecksum{}).Compute(ringWithoutIdentities) + identityChecksum := identityChecksummer.Compute(ringWithoutIdentities) + + assert.Equal(t, legacyChecksum, identityChecksum, "Identity checksum should be the same as legacy on ring without identities") + + ringWithIdentities := New(farm.Fingerprint32, 1) + ringWithIdentities.AddMembers(genMembers(1, 1, 10, true)...) + + identityChecksum = identityChecksummer.Compute(ringWithIdentities) + + assert.NotEqual(t, legacyChecksum, identityChecksum, "IdentityChecksummer should not match legacy checksummer on ring with identites ") +} + +func TestReplicaPointChecksum_Compute(t *testing.T) { + replicaPointChecksummer := &replicaPointChecksum{} + members := genMembers(1, 1, 10, false) + + ring1ReplicaPoint := New(farm.Fingerprint32, 1) + ring1ReplicaPoint.AddMembers(members...) + + ring2ReplicaPoints := New(farm.Fingerprint32, 2) + ring2ReplicaPoints.AddMembers(members...) + + checksum1ReplicaPoint := replicaPointChecksummer.Compute(ring1ReplicaPoint) + checksum2ReplicaPoints := replicaPointChecksummer.Compute(ring2ReplicaPoints) + + assert.NotEqual(t, checksum1ReplicaPoint, checksum2ReplicaPoints, "Checksum should not match with different replica point counts") +} From 273d946fe27a1e1b9451db16ac68a05d2c9e2762 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Tue, 6 Dec 2016 12:02:11 +0100 Subject: [PATCH 13/19] Add test hashring.checksums --- hashring/hashring_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/hashring/hashring_test.go b/hashring/hashring_test.go index 2aac5352..d321d278 100644 --- a/hashring/hashring_test.go +++ b/hashring/hashring_test.go @@ -112,6 +112,37 @@ func TestChecksumChanges(t *testing.T) { assert.NotEqual(t, checksum, ring.Checksum(), "expected checksum to have changed on server remove") } +func TestHashRing_Checksums(t *testing.T) { + getSortedKeys := func(m map[string]uint32) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys + } + + ring := New(farm.Fingerprint32, 10) + ring.checksummers = map[string]Checksum{ + "identity": &identityChecksum{}, + "address": &addressChecksum{}, + "replicaPoint": &replicaPointChecksum{}, + } + + checkSummers := []string{"identity", "address", "replicaPoint"} + sort.Strings(checkSummers) + + ring.AddMembers(fakeMember{address: "server1"}) + ring.AddMembers(fakeMember{address: "server2"}) + + checksums := ring.Checksums() + assert.Equal(t, checkSummers, getSortedKeys(checksums), "Expected all checksums to be computed") + ring.RemoveMembers(fakeMember{address: "server1"}) + + assert.NotEqual(t, checksums, ring.Checksums(), "expected checksums to have changed on server remove") + assert.Equal(t, checkSummers, getSortedKeys(checksums), "Expected all checksums to be computed") +} + func TestServerCount(t *testing.T) { ring := New(farm.Fingerprint32, 10) assert.Equal(t, 0, ring.ServerCount(), "expected one server to be in ring") From e4926c9a2f5b1e04aa6a58b979e6aa1a2778f110 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Tue, 6 Dec 2016 13:07:21 +0100 Subject: [PATCH 14/19] fix import --- hashring/checksum_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hashring/checksum_test.go b/hashring/checksum_test.go index 62344cea..5f137aea 100644 --- a/hashring/checksum_test.go +++ b/hashring/checksum_test.go @@ -25,7 +25,7 @@ import ( "strings" "testing" - "code.uber.internal/rt/cag.git/Godeps/_workspace/src/code.uber.internal/go-common.git/Godeps/_workspace/src/github.com/dgryski/go-farm" + "github.com/dgryski/go-farm" "github.com/stretchr/testify/assert" ) From 49b5026cb040b9e3e8e139a21d6d083ca76a3b77 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Tue, 6 Dec 2016 13:41:54 +0100 Subject: [PATCH 15/19] switch it-test branch --- test/lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lib.sh b/test/lib.sh index a6b89702..de96534e 100644 --- a/test/lib.sh +++ b/test/lib.sh @@ -2,7 +2,7 @@ declare project_root="${0%/*}/.." declare ringpop_common_dir="${0%/*}/ringpop-common" -declare ringpop_common_branch="master" +declare ringpop_common_branch="menno/allow-multi-checksum" # # Clones or updates the ringpop-common repository. From 31e630b389d13a9b0644f833c116769af70f8737 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Thu, 15 Dec 2016 17:01:15 +0100 Subject: [PATCH 16/19] Renamed checksum -> checksummer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit moved addressChecksummer to a test file since it’s only used for validation backwards compatibility. --- hashring/{checksum.go => checksummer.go} | 27 +++++------------ .../{checksum_test.go => checksummer_test.go} | 29 +++++++++++++------ hashring/hashring.go | 12 ++++---- hashring/hashring_test.go | 8 ++--- 4 files changed, 38 insertions(+), 38 deletions(-) rename hashring/{checksum.go => checksummer.go} (61%) rename hashring/{checksum_test.go => checksummer_test.go} (73%) diff --git a/hashring/checksum.go b/hashring/checksummer.go similarity index 61% rename from hashring/checksum.go rename to hashring/checksummer.go index e17102ac..5335b5a7 100644 --- a/hashring/checksum.go +++ b/hashring/checksummer.go @@ -9,31 +9,20 @@ import ( "github.com/dgryski/go-farm" ) -// Checksum computes a checksum for an instance of a HashRing. The +// Checksummer computes a checksum for an instance of a HashRing. The // checksum can be used to compare two rings for equality. -type Checksum interface { - // Compute calculates the checksum for the hashring that is passed in. +type Checksummer interface { + // Checksum calculates the checksum for the hashring that is passed in. // Compute will be called while having at least a read-lock on the hashring so // it is safe to read from the ring, but not safe to change 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 - Compute(ring *HashRing) (checksum uint32) + Checksum(ring *HashRing) (checksum uint32) } -// addressChecksum calculates checksums for all addresses that are added to the -// hashring. -type addressChecksum struct{} +type identityChecksummer struct{} -func (i *addressChecksum) Compute(ring *HashRing) uint32 { - addresses := ring.copyServersNoLock() - sort.Strings(addresses) - bytes := []byte(strings.Join(addresses, ";")) - return farm.Fingerprint32(bytes) -} - -type identityChecksum struct{} - -func (i *identityChecksum) Compute(ring *HashRing) uint32 { +func (i *identityChecksummer) Checksum(ring *HashRing) uint32 { identitySet := make(map[string]struct{}) ring.tree.root.walk(func(node *redBlackNode) bool { identitySet[node.key.(replicaPoint).identity] = struct{}{} @@ -50,9 +39,9 @@ func (i *identityChecksum) Compute(ring *HashRing) uint32 { return farm.Fingerprint32(bytes) } -type replicaPointChecksum struct{} +type replicaPointChecksummer struct{} -func (r *replicaPointChecksum) Compute(ring *HashRing) uint32 { +func (r *replicaPointChecksummer) Checksum(ring *HashRing) uint32 { buffer := bytes.Buffer{} ring.tree.root.walk(func(node *redBlackNode) bool { diff --git a/hashring/checksum_test.go b/hashring/checksummer_test.go similarity index 73% rename from hashring/checksum_test.go rename to hashring/checksummer_test.go index 5f137aea..aa2ff5f9 100644 --- a/hashring/checksum_test.go +++ b/hashring/checksummer_test.go @@ -29,11 +29,22 @@ import ( "github.com/stretchr/testify/assert" ) +// addressChecksum implements the now obsolete checksum method that didn't support identities. +// It's moved to this test file to test backwards compatibility of the identityChecksummer +type addressChecksummer struct{} + +func (i *addressChecksummer) Checksum(ring *HashRing) uint32 { + addresses := ring.copyServersNoLock() + sort.Strings(addresses) + bytes := []byte(strings.Join(addresses, ";")) + return farm.Fingerprint32(bytes) +} + func TestAddressChecksum_Compute(t *testing.T) { members := genMembers(1, 1, 10, false) ring := New(farm.Fingerprint32, 1) ring.AddMembers(members...) - checksum := &addressChecksum{} + checksum := &addressChecksummer{} addresses := make([]string, 0, 10) for _, members := range members { @@ -44,32 +55,32 @@ func TestAddressChecksum_Compute(t *testing.T) { bytes := []byte(strings.Join(addresses, ";")) expected := farm.Fingerprint32(bytes) - actual := checksum.Compute(ring) + actual := checksum.Checksum(ring) assert.Equal(t, expected, actual) } func TestIdentityChecksum_Compute(t *testing.T) { - identityChecksummer := &identityChecksum{} + identityChecksummer := &identityChecksummer{} ringWithoutIdentities := New(farm.Fingerprint32, 1) ringWithoutIdentities.AddMembers(genMembers(1, 1, 10, false)...) - legacyChecksum := (&addressChecksum{}).Compute(ringWithoutIdentities) - identityChecksum := identityChecksummer.Compute(ringWithoutIdentities) + legacyChecksum := (&addressChecksummer{}).Checksum(ringWithoutIdentities) + identityChecksum := identityChecksummer.Checksum(ringWithoutIdentities) assert.Equal(t, legacyChecksum, identityChecksum, "Identity checksum should be the same as legacy on ring without identities") ringWithIdentities := New(farm.Fingerprint32, 1) ringWithIdentities.AddMembers(genMembers(1, 1, 10, true)...) - identityChecksum = identityChecksummer.Compute(ringWithIdentities) + identityChecksum = identityChecksummer.Checksum(ringWithIdentities) assert.NotEqual(t, legacyChecksum, identityChecksum, "IdentityChecksummer should not match legacy checksummer on ring with identites ") } func TestReplicaPointChecksum_Compute(t *testing.T) { - replicaPointChecksummer := &replicaPointChecksum{} + replicaPointChecksummer := &replicaPointChecksummer{} members := genMembers(1, 1, 10, false) ring1ReplicaPoint := New(farm.Fingerprint32, 1) @@ -78,8 +89,8 @@ func TestReplicaPointChecksum_Compute(t *testing.T) { ring2ReplicaPoints := New(farm.Fingerprint32, 2) ring2ReplicaPoints.AddMembers(members...) - checksum1ReplicaPoint := replicaPointChecksummer.Compute(ring1ReplicaPoint) - checksum2ReplicaPoints := replicaPointChecksummer.Compute(ring2ReplicaPoints) + checksum1ReplicaPoint := replicaPointChecksummer.Checksum(ring1ReplicaPoint) + checksum2ReplicaPoints := replicaPointChecksummer.Checksum(ring2ReplicaPoints) assert.NotEqual(t, checksum1ReplicaPoint, checksum2ReplicaPoints, "Checksum should not match with different replica point counts") } diff --git a/hashring/hashring.go b/hashring/hashring.go index c8af79f5..10cdfbc7 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -70,7 +70,7 @@ type HashRing struct { legacyChecksum uint32 // checksummers is map of named Checksum calculators for the hashring - checksummers map[string]Checksum + checksummers map[string]Checksummer // checksums is a map containing the checksums that are representing this // hashring. The map should never be altered in place so it is safe to pass // a copy to components that need the checksums @@ -88,8 +88,8 @@ func New(hashfunc func([]byte) uint32, replicaPoints int) *HashRing { }, logger: logging.Logger("ring"), - checksummers: map[string]Checksum{ - "replica": &replicaPointChecksum{}, + checksummers: map[string]Checksummer{ + "replica": &replicaPointChecksummer{}, }, } @@ -128,7 +128,7 @@ func (r *HashRing) computeChecksumsNoLock() { // calculate all configured checksums for name, checksummer := range r.checksummers { oldChecksum := oldChecksums[name] - newChecksum := checksummer.Compute(r) + newChecksum := checksummer.Checksum(r) r.checksums[name] = newChecksum if oldChecksum != newChecksum { @@ -137,9 +137,9 @@ func (r *HashRing) computeChecksumsNoLock() { } // calculate the legacy identity only based checksum - legacyChecksummer := identityChecksum{} + legacyChecksummer := identityChecksummer{} oldChecksum := r.legacyChecksum - newChecksum := legacyChecksummer.Compute(r) + newChecksum := legacyChecksummer.Checksum(r) r.legacyChecksum = newChecksum if oldChecksum != newChecksum { diff --git a/hashring/hashring_test.go b/hashring/hashring_test.go index d321d278..15b64044 100644 --- a/hashring/hashring_test.go +++ b/hashring/hashring_test.go @@ -123,10 +123,10 @@ func TestHashRing_Checksums(t *testing.T) { } ring := New(farm.Fingerprint32, 10) - ring.checksummers = map[string]Checksum{ - "identity": &identityChecksum{}, - "address": &addressChecksum{}, - "replicaPoint": &replicaPointChecksum{}, + ring.checksummers = map[string]Checksummer{ + "identity": &identityChecksummer{}, + "address": &addressChecksummer{}, + "replicaPoint": &replicaPointChecksummer{}, } checkSummers := []string{"identity", "address", "replicaPoint"} From 101e4f895abf814d705a0e73e9b2cc2cbca03b0b Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Thu, 15 Dec 2016 17:02:35 +0100 Subject: [PATCH 17/19] switch it-tests back to master after merg --- test/lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lib.sh b/test/lib.sh index de96534e..80889a8e 100644 --- a/test/lib.sh +++ b/test/lib.sh @@ -1,7 +1,7 @@ # Common functions for test code declare project_root="${0%/*}/.." -declare ringpop_common_dir="${0%/*}/ringpop-common" +declare ringpop_common_dir="${0%/*}/master" declare ringpop_common_branch="menno/allow-multi-checksum" # From fac34e489316fe4ded99e40b3ca18a0d8f8fbe9f Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Thu, 15 Dec 2016 18:04:49 +0100 Subject: [PATCH 18/19] fix ringpop-common --- test/lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/lib.sh b/test/lib.sh index 80889a8e..a6b89702 100644 --- a/test/lib.sh +++ b/test/lib.sh @@ -1,8 +1,8 @@ # Common functions for test code declare project_root="${0%/*}/.." -declare ringpop_common_dir="${0%/*}/master" -declare ringpop_common_branch="menno/allow-multi-checksum" +declare ringpop_common_dir="${0%/*}/ringpop-common" +declare ringpop_common_branch="master" # # Clones or updates the ringpop-common repository. From c7ef3e71bcaa13426d1b0c206b27b22e37181637 Mon Sep 17 00:00:00 2001 From: Menno Pruijssers Date: Fri, 16 Dec 2016 10:51:37 +0100 Subject: [PATCH 19/19] fix after merge --- hashring/checksummer.go | 4 ++-- hashring/hashring.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hashring/checksummer.go b/hashring/checksummer.go index 5335b5a7..d5af2ce7 100644 --- a/hashring/checksummer.go +++ b/hashring/checksummer.go @@ -24,7 +24,7 @@ type identityChecksummer struct{} func (i *identityChecksummer) Checksum(ring *HashRing) uint32 { identitySet := make(map[string]struct{}) - ring.tree.root.walk(func(node *redBlackNode) bool { + ring.tree.root.traverseWhile(func(node *redBlackNode) bool { identitySet[node.key.(replicaPoint).identity] = struct{}{} return true }) @@ -44,7 +44,7 @@ type replicaPointChecksummer struct{} func (r *replicaPointChecksummer) Checksum(ring *HashRing) uint32 { buffer := bytes.Buffer{} - ring.tree.root.walk(func(node *redBlackNode) bool { + ring.tree.root.traverseWhile(func(node *redBlackNode) bool { buffer.WriteString(strconv.Itoa(node.key.(replicaPoint).hash)) buffer.WriteString("-") buffer.WriteString(node.value.(string)) diff --git a/hashring/hashring.go b/hashring/hashring.go index 8717687b..192cbf91 100644 --- a/hashring/hashring.go +++ b/hashring/hashring.go @@ -48,8 +48,8 @@ type replicaPoint struct { // hash of the point in the hashring hash int - // identity of the member that owns this replicaPoint. - identity string + // identity of the member that owns this replicaPoint. + identity string // address of the member that owns this replicaPoint. address string