From e960a0e03c90f8726dc8088a6d5b58cf0c3821e0 Mon Sep 17 00:00:00 2001
From: Yicheng Qin <qycqycqycqycqyc@gmail.com>
Date: Thu, 8 May 2014 13:15:10 -0700
Subject: [PATCH] chore(client): minor changes based on comments

The changes are made on error handling, comments and constant.
---
 etcd/etcd.go          | 14 +++++++-------
 server/client.go      |  7 ++++++-
 server/peer_server.go | 25 ++++++-------------------
 3 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/etcd/etcd.go b/etcd/etcd.go
index cd79c2a56..0ee505fc1 100644
--- a/etcd/etcd.go
+++ b/etcd/etcd.go
@@ -38,7 +38,13 @@ import (
 	"github.com/coreos/etcd/store"
 )
 
-const extraTimeout = 1000
+// TODO(yichengq): constant extraTimeout is a hack.
+// Current problem is that there is big lag between join command
+// execution and join success.
+// Fix it later. It should be removed when proper method is found and
+// enough tests are provided. It is expected to be calculated from
+// heartbeatInterval and electionTimeout only.
+const extraTimeout = time.Duration(1000) * time.Millisecond
 
 type Etcd struct {
 	Config       *config.Config     // etcd config
@@ -140,12 +146,6 @@ func (e *Etcd) Run() {
 	dialTimeout := (3 * heartbeatInterval) + electionTimeout
 	responseHeaderTimeout := (3 * heartbeatInterval) + electionTimeout
 
-	// TODO(yichengq): constant extraTimeout is a hack here.
-	// Current problem is that there is big lag between join command
-	// execution and join success.
-	// Fix it later. It should be removed when proper method is found and
-	// enough tests are provided. It is expected to be calculated from
-	// heartbeatInterval and electionTimeout only.
 	clientTransporter := &httpclient.Transport{
 		ResponseHeaderTimeout: responseHeaderTimeout + extraTimeout,
 		// This is a workaround for Transport.CancelRequest doesn't work on
diff --git a/server/client.go b/server/client.go
index fea52ac76..6690e319d 100644
--- a/server/client.go
+++ b/server/client.go
@@ -16,6 +16,8 @@ import (
 
 // Client sends various requests using HTTP API.
 // It is different from raft communication, and doesn't record anything in the log.
+// The argument url is required to contain scheme and host only, and
+// there is no trailing slash in it.
 // Public functions return "etcd/error".Error intentionally to figure out
 // etcd error code easily.
 // TODO(yichengq): It is similar to go-etcd. But it could have many efforts
@@ -55,7 +57,10 @@ func (c *Client) GetVersion(url string) (int, *etcdErr.Error) {
 	}
 
 	// Parse version number.
-	version, _ := strconv.Atoi(string(body))
+	version, err := strconv.Atoi(string(body))
+	if err != nil {
+		return 0, clientError(err)
+	}
 	return version, nil
 }
 
diff --git a/server/peer_server.go b/server/peer_server.go
index e37471b11..2c9fc6321 100644
--- a/server/peer_server.go
+++ b/server/peer_server.go
@@ -485,31 +485,21 @@ func (s *PeerServer) joinByPeer(server raft.Server, peer string, scheme string)
 	// Our version must match the leaders version
 	version, err := s.client.GetVersion(u)
 	if err != nil {
-		log.Debugf("fail checking join version")
-		return err
+		return fmt.Errorf("fail checking join version: %v", err)
 	}
 	if version < store.MinVersion() || version > store.MaxVersion() {
-		log.Infof("fail passing version compatibility(%d-%d) using %d", store.MinVersion(), store.MaxVersion(), version)
-		return fmt.Errorf("incompatible version")
+		return fmt.Errorf("fail passing version compatibility(%d-%d) using %d", store.MinVersion(), store.MaxVersion(), version)
 	}
 
 	// Fetch current peer list
 	machines, err := s.client.GetMachines(u)
 	if err != nil {
-		log.Debugf("fail getting machine messages")
-		return err
+		return fmt.Errorf("fail getting machine messages: %v", err)
 	}
 	exist := false
 	for _, machine := range machines {
 		if machine.Name == server.Name() {
 			exist = true
-			// TODO(yichengq): cannot set join index for it.
-			// Need discussion about the best way to do it.
-			//
-			// if machine.PeerURL == s.Config.URL {
-			// 	log.Infof("has joined the cluster(%v) before", machines)
-			// 	return nil
-			// }
 			break
 		}
 	}
@@ -517,12 +507,10 @@ func (s *PeerServer) joinByPeer(server raft.Server, peer string, scheme string)
 	// Fetch cluster config to see whether exists some place.
 	clusterConfig, err := s.client.GetClusterConfig(u)
 	if err != nil {
-		log.Debugf("fail getting cluster config")
-		return err
+		return fmt.Errorf("fail getting cluster config: %v", err)
 	}
 	if !exist && clusterConfig.ActiveSize <= len(machines) {
-		log.Infof("stop joining because the cluster is full with %d nodes", len(machines))
-		return fmt.Errorf("out of quota")
+		return fmt.Errorf("stop joining because the cluster is full with %d nodes", len(machines))
 	}
 
 	joinIndex, err := s.client.AddMachine(u,
@@ -534,8 +522,7 @@ func (s *PeerServer) joinByPeer(server raft.Server, peer string, scheme string)
 			EtcdURL:    s.server.URL(),
 		})
 	if err != nil {
-		log.Debugf("fail on join request")
-		return err
+		return fmt.Errorf("fail on join request: %v", err)
 	}
 
 	s.joinIndex = joinIndex