More cleanup from code review

agent/consul/rpc.go

@@ -238,7 +238,7 @@ func (s *Server) getLeader() (bool, *metadata.Server) {
 	}
 
 	// Lookup the server
-	server, _ := s.serverLookup.GetServer(leader)
+	server := s.serverLookup.Server(leader)
 
 	// Server could be nil
 	return false, server

agent/consul/serf.go

@@ -261,13 +261,13 @@ func (s *Server) maybeBootstrap() {
 // lanNodeFailed is used to handle fail events on the LAN pool.
 func (s *Server) lanNodeFailed(me serf.MemberEvent) {
 	for _, m := range me.Members {
-		ok, parts := metadata.IsConsulServer(m)
+		ok, serverMeta := metadata.IsConsulServer(m)
 		if !ok {
 			continue
 		}
-		s.logger.Printf("[INFO] consul: Removing LAN server %s", parts)
+		s.logger.Printf("[INFO] consul: Removing LAN server %s", serverMeta)
 
 		// Update id to address map
-		s.serverLookup.RemoveServer(parts)
+		s.serverLookup.RemoveServer(serverMeta)
 	}
 }

agent/consul/server.go

@@ -700,9 +700,9 @@ func (s *Server) setupRPC(tlsWrap tlsutil.DCWrapper) error {
 			return true
 		}
 
-		server, ok := s.serverLookup.GetServer(address)
+		server := s.serverLookup.Server(address)
 
-		if !ok {
+		if server == nil {
 			return false
 		}
 

agent/consul/server_address_lookup.go

@@ -1,56 +0,0 @@
-package consul
-
-import (
-	"fmt"
-	"sync"
-
-	"github.com/hashicorp/consul/agent/metadata"
-	"github.com/hashicorp/raft"
-)
-
-// ServerLookup encapsulates looking up servers by id and address
-type ServerLookup struct {
-	lock            sync.RWMutex
-	addressToServer map[raft.ServerAddress]*metadata.Server
-	IdToServer      map[raft.ServerID]*metadata.Server
-}
-
-func NewServerLookup() *ServerLookup {
-	return &ServerLookup{addressToServer: make(map[raft.ServerAddress]*metadata.Server), IdToServer: make(map[raft.ServerID]*metadata.Server)}
-}
-
-func (sa *ServerLookup) AddServer(server *metadata.Server) {
-	sa.lock.Lock()
-	defer sa.lock.Unlock()
-	sa.addressToServer[raft.ServerAddress(server.Addr.String())] = server
-	sa.IdToServer[raft.ServerID(server.ID)] = server
-}
-
-func (sa *ServerLookup) RemoveServer(server *metadata.Server) {
-	sa.lock.Lock()
-	defer sa.lock.Unlock()
-	delete(sa.addressToServer, raft.ServerAddress(server.Addr.String()))
-	delete(sa.IdToServer, raft.ServerID(server.ID))
-}
-
-// Implements the ServerAddressProvider interface
-func (sa *ServerLookup) ServerAddr(id raft.ServerID) (raft.ServerAddress, error) {
-	sa.lock.RLock()
-	defer sa.lock.RUnlock()
-	svr, ok := sa.IdToServer[id]
-	if !ok {
-		return "", fmt.Errorf("Could not find address for server id %v", id)
-	}
-	return raft.ServerAddress(svr.Addr.String()), nil
-}
-
-// GetServer looks up the server by address, returns a boolean if not found
-func (sa *ServerLookup) GetServer(addr raft.ServerAddress) (*metadata.Server, bool) {
-	sa.lock.RLock()
-	defer sa.lock.RUnlock()
-	svr, ok := sa.addressToServer[addr]
-	if !ok {
-		return nil, false
-	}
-	return svr, true
-}

agent/consul/server_lookup.go

@@ -0,0 +1,66 @@
+package consul
+
+import (
+	"fmt"
+	"sync"
+
+	"github.com/hashicorp/consul/agent/metadata"
+	"github.com/hashicorp/raft"
+)
+
+// ServerLookup encapsulates looking up servers by id and address
+type ServerLookup struct {
+	lock            sync.RWMutex
+	addressToServer map[raft.ServerAddress]*metadata.Server
+	idToServer      map[raft.ServerID]*metadata.Server
+}
+
+func NewServerLookup() *ServerLookup {
+	return &ServerLookup{
+		addressToServer: make(map[raft.ServerAddress]*metadata.Server),
+		idToServer:      make(map[raft.ServerID]*metadata.Server),
+	}
+}
+
+func (sl *ServerLookup) AddServer(server *metadata.Server) {
+	sl.lock.Lock()
+	defer sl.lock.Unlock()
+	sl.addressToServer[raft.ServerAddress(server.Addr.String())] = server
+	sl.idToServer[raft.ServerID(server.ID)] = server
+}
+
+func (sl *ServerLookup) RemoveServer(server *metadata.Server) {
+	sl.lock.Lock()
+	defer sl.lock.Unlock()
+	delete(sl.addressToServer, raft.ServerAddress(server.Addr.String()))
+	delete(sl.idToServer, raft.ServerID(server.ID))
+}
+
+// Implements the ServerAddressProvider interface
+func (sl *ServerLookup) ServerAddr(id raft.ServerID) (raft.ServerAddress, error) {
+	sl.lock.RLock()
+	defer sl.lock.RUnlock()
+	svr, ok := sl.idToServer[id]
+	if !ok {
+		return "", fmt.Errorf("Could not find address for server id %v", id)
+	}
+	return raft.ServerAddress(svr.Addr.String()), nil
+}
+
+// Server looks up the server by address, returns a boolean if not found
+func (sl *ServerLookup) Server(addr raft.ServerAddress) *metadata.Server {
+	sl.lock.RLock()
+	defer sl.lock.RUnlock()
+	svr, _ := sl.addressToServer[addr]
+	return svr
+}
+
+func (sl *ServerLookup) Servers() []*metadata.Server {
+	sl.lock.RLock()
+	defer sl.lock.RUnlock()
+	var ret []*metadata.Server
+	for _, svr := range sl.addressToServer {
+		ret = append(ret, svr)
+	}
+	return ret
+}

agent/consul/server_lookup_test.go

@@ -36,8 +36,8 @@ func TestServerLookup(t *testing.T) {
 		t.Fatalf("Expected %v but got %v", addr, got)
 	}
 
-	server, ok := lookup.GetServer(raft.ServerAddress(addr))
+	server := lookup.Server(raft.ServerAddress(addr))
-	if !ok {
+	if server == nil {
 		t.Fatalf("Expected lookup to return true")
 	}
 	if server.Addr.String() != addr {

agent/consul/server_test.go

@@ -342,7 +342,7 @@ func TestServer_JoinSeparateLanAndWanAddresses(t *testing.T) {
 		if len(s2.router.GetDatacenters()) != 2 {
 			r.Fatalf("remote consul missing")
 		}
-		if len(s2.serverLookup.addressToServer) != 2 {
+		if len(s2.serverLookup.Servers()) != 2 {
 			r.Fatalf("local consul fellow s3 for s2 missing")
 		}
 	})
@@ -667,7 +667,7 @@ func testVerifyRPC(s1, s2 *Server, t *testing.T) (bool, error) {
 
 	// Have s2 make an RPC call to s1
 	var leader *metadata.Server
-	for _, server := range s2.serverLookup.addressToServer {
+	for _, server := range s2.serverLookup.Servers() {
 		if server.Name == s1.config.NodeName {
 			leader = server
 		}