Merge pull request #29 from Shopify/no_broker_equality

Remove Broker.Equals()

broker.go

@@ -107,18 +107,6 @@ func (b *Broker) Addr() string {
 	return b.addr
 }
 
-// Equals compares two brokers. Two brokers are considered equal if they have the same address and id,
-// or if they are both nil.
-func (b *Broker) Equals(a *Broker) bool {
-	switch {
-	case a == nil && b == nil:
-		return true
-	case (a == nil && b != nil) || (a != nil && b == nil):
-		return false
-	}
-	return a.id == b.id && a.addr == b.addr
-}
-
 func (b *Broker) GetMetadata(clientID string, request *MetadataRequest) (*MetadataResponse, error) {
 	response := new(MetadataResponse)
 

broker_test.go

@@ -161,42 +161,6 @@ func ExampleBroker() error {
 	return nil
 }
 
-func TestBrokerEquals(t *testing.T) {
-	var b1, b2 *Broker
-
-	b1 = nil
-	b2 = nil
-
-	if !b1.Equals(b2) {
-		t.Error("Two nil brokers didn't compare equal.")
-	}
-
-	b1 = NewBroker("abc:123")
-
-	if b1.Equals(b2) {
-		t.Error("Non-nil and nil brokers compared equal.")
-	}
-	if b2.Equals(b1) {
-		t.Error("Nil and non-nil brokers compared equal.")
-	}
-
-	b2 = NewBroker("abc:1234")
-	if b1.Equals(b2) || b2.Equals(b1) {
-		t.Error("Brokers with different addrs compared equal.")
-	}
-
-	b2 = NewBroker("abc:123")
-	b2.id = -2
-	if b1.Equals(b2) || b2.Equals(b1) {
-		t.Error("Brokers with different ids compared equal.")
-	}
-
-	b2.id = -1
-	if !b1.Equals(b2) || !b2.Equals(b1) {
-		t.Error("Similar brokers did not compare equal.")
-	}
-}
-
 func TestBrokerAccessors(t *testing.T) {
 
 	broker := NewBroker("abc:123")

client.go

@@ -257,27 +257,22 @@ func (client *Client) update(data *MetadataResponse) ([]string, error) {
 	client.lock.Lock()
 	defer client.lock.Unlock()
 
-	// First discard brokers that we already know about. This avoids bouncing TCP connections,
-	// and especially avoids closing valid connections out from under other code which may be trying
-	// to use them.
-	var newBrokers []*Broker
-	for _, broker := range data.Brokers {
-		if !broker.Equals(client.brokers[broker.ID()]) {
-			newBrokers = append(newBrokers, broker)
-		}
-	}
-
-	// Now asynchronously try to open connections to the new brokers. We don't care if they
+	// For all the brokers we received:
+	// - if it is a new ID, save it
+	// - if it is an existing ID, but the address we have is stale, discard the old one and save it
+	// - otherwise ignore it, replacing our existing one would just bounce the connection
+	// We asynchronously try to open connections to the new brokers. We don't care if they
 	// fail, since maybe that broker is unreachable but doesn't have a topic we care about.
 	// If it fails and we do care, whoever tries to use it will get the connection error.
-	// If we have an old broker with that ID (but a different host/port, since they didn't
-	// compare as equals above) then close and remove that broker before saving the new one.
-	for _, broker := range newBrokers {
-		if client.brokers[broker.ID()] != nil {
+	for _, broker := range data.Brokers {
+		if client.brokers[broker.ID()] == nil {
+			broker.Open()
+			client.brokers[broker.ID()] = broker
+		} else if broker.Addr() != client.brokers[broker.ID()].Addr() {
 			go client.brokers[broker.ID()].Close()
+			broker.Open()
+			client.brokers[broker.ID()] = broker
 		}
-		broker.Open()
-		client.brokers[broker.ID()] = broker
 	}
 
 	toRetry := make(map[string]bool)