[FAB-8019] CouchDB Retry count is misleading

The CouchDB retry currently assumes the first pass through is actually retry number 1. It also does not currently allow a setting of zero retries. This can be confusing to the end user and will prevent some potential use cases that may need to wrap the request handler with zero retry logic. Change-Id: I74a5628f19fd6d73a24091bad66f955046e28190 Signed-off-by: Chris Elder <[email protected]>
hyperledger · Feb 13, 2018 · 04e95e1 · 04e95e1
1 parent 0cb7692
commit 04e95e1
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 37 deletions.
diff --git a/core/ledger/util/couchdb/couchdb.go b/core/ledger/util/couchdb/couchdb.go
@@ -227,8 +227,10 @@ type IndexResult struct {
 // closeResponseBody discards the body and then closes it to enable returning it to
 // connection pool
 func closeResponseBody(resp *http.Response) {
-	io.Copy(ioutil.Discard, resp.Body) // discard whatever is remaining of body
-	resp.Body.Close()
+	if resp != nil {
+		io.Copy(ioutil.Discard, resp.Body) // discard whatever is remaining of body
+		resp.Body.Close()
+	}
 }
 
 //CreateConnectionDefinition for a new client connection
@@ -686,6 +688,10 @@ func createAttachmentPart(couchDoc *CouchDoc, defaultBoundary string) (bytes.Buf
 
 func getRevisionHeader(resp *http.Response) (string, error) {
 
+	if resp == nil {
+		return "", fmt.Errorf("No response was received from CouchDB")
+	}
+
 	revision := resp.Header.Get("Etag")
 
 	if revision == "" {
@@ -1436,7 +1442,7 @@ func (dbclient *CouchDatabase) handleRequestWithRevisionRetry(id, method string,
 	//attempt the http request for the max number of retries
 	//In this case, the retry is to catch problems where a client timeout may miss a
 	//successful CouchDB update and cause a document revision conflict on a retry in handleRequest
-	for attempts := 0; attempts < maxRetries; attempts++ {
+	for attempts := 0; attempts <= maxRetries; attempts++ {
 
 		//if the revision was not passed in, or if a revision conflict is detected on prior attempt,
 		//query CouchDB for the document revision
@@ -1479,12 +1485,16 @@ func (couchInstance *CouchInstance) handleRequest(method, connectURL string, dat
 	//set initial wait duration for retries
 	waitDuration := retryWaitTime * time.Millisecond
 
-	if maxRetries < 1 {
-		return nil, nil, fmt.Errorf("Number of retries must be greater than zero.")
+	if maxRetries < 0 {
+		return nil, nil, fmt.Errorf("Number of retries must be zero or greater.")
 	}
 
 	//attempt the http request for the max number of retries
-	for attempts := 0; attempts < maxRetries; attempts++ {
+	// if maxRetries is 0, the database creation will be attempted once and will
+	//    return an error if unsuccessful
+	// if maxRetries is 3 (default), a maximum of 4 attempts (one attempt with 3 retries)
+	//    will be made with warning entries for unsuccessful attempts
+	for attempts := 0; attempts <= maxRetries; attempts++ {
 
 		//Set up a buffer for the payload data
 		payloadData := new(bytes.Buffer)
@@ -1555,40 +1565,44 @@ func (couchInstance *CouchInstance) handleRequest(method, connectURL string, dat
 			break
 		}
 
-		//if this is an unexpected golang http error, log the error and retry
-		if errResp != nil {
+		// If the maxRetries is greater than 0, then log the retry info
+		if maxRetries > 0 {
 
-			//Log the error with the retry count and continue
-			logger.Warningf("Retrying couchdb request in %s. Attempt:%v  Error:%v",
-				waitDuration.String(), attempts+1, errResp.Error())
+			//if this is an unexpected golang http error, log the error and retry
+			if errResp != nil {
 
-			//otherwise this is an unexpected 500 error from CouchDB. Log the error and retry.
-		} else {
-			//Read the response body and close it for next attempt
-			jsonError, err := ioutil.ReadAll(resp.Body)
-			closeResponseBody(resp)
-			if err != nil {
-				return nil, nil, err
-			}
+				//Log the error with the retry count and continue
+				logger.Warningf("Retrying couchdb request in %s. Attempt:%v  Error:%v",
+					waitDuration.String(), attempts+1, errResp.Error())
 
-			errorBytes := []byte(jsonError)
+				//otherwise this is an unexpected 500 error from CouchDB. Log the error and retry.
+			} else {
+				//Read the response body and close it for next attempt
+				jsonError, err := ioutil.ReadAll(resp.Body)
+				closeResponseBody(resp)
+				if err != nil {
+					return nil, nil, err
+				}
+
+				errorBytes := []byte(jsonError)
+				//Unmarshal the response
+				err = json.Unmarshal(errorBytes, &couchDBReturn)
+				if err != nil {
+					return nil, nil, err
+				}
+
+				//Log the 500 error with the retry count and continue
+				logger.Warningf("Retrying couchdb request in %s. Attempt:%v  Couch DB Error:%s,  Status Code:%v  Reason:%v",
+					waitDuration.String(), attempts+1, couchDBReturn.Error, resp.Status, couchDBReturn.Reason)
 
-			//Unmarshal the response
-			err = json.Unmarshal(errorBytes, &couchDBReturn)
-			if err != nil {
-				return nil, nil, err
 			}
+			//sleep for specified sleep time, then retry
+			time.Sleep(waitDuration)
 
-			//Log the 500 error with the retry count and continue
-			logger.Warningf("Retrying couchdb request in %s. Attempt:%v  Couch DB Error:%s,  Status Code:%v  Reason:%v",
-				waitDuration.String(), attempts+1, couchDBReturn.Error, resp.Status, couchDBReturn.Reason)
+			//backoff, doubling the retry time for next attempt
+			waitDuration *= 2
 
 		}
-		//sleep for specified sleep time, then retry
-		time.Sleep(waitDuration)
-
-		//backoff, doubling the retry time for next attempt
-		waitDuration *= 2
 
 	} // end retry loop
 

diff --git a/core/ledger/util/couchdb/couchdb_test.go b/core/ledger/util/couchdb/couchdb_test.go
@@ -354,9 +354,24 @@ func TestBadDBCredentials(t *testing.T) {
 	}
 
 }
-
 func TestDBCreateDatabaseAndPersist(t *testing.T) {
 
+	//Test create and persist with default configured maxRetries
+	testDBCreateDatabaseAndPersist(t, couchDBDef.MaxRetries)
+
+	//Test create and persist with 0 retries
+	testDBCreateDatabaseAndPersist(t, 0)
+
+	//Test batch operations with default configured maxRetries
+	testBatchBatchOperations(t, couchDBDef.MaxRetries)
+
+	//Test batch operations with 0 retries
+	testBatchBatchOperations(t, 0)
+
+}
+
+func testDBCreateDatabaseAndPersist(t *testing.T, maxRetries int) {
+
 	if ledgerconfig.IsCouchDBEnabled() {
 
 		database := "testdbcreatedatabaseandpersist"
@@ -367,7 +382,7 @@ func TestDBCreateDatabaseAndPersist(t *testing.T) {
 		if err == nil {
 			//create a new instance and database object
 			couchInstance, err := CreateCouchInstance(couchDBDef.URL, couchDBDef.Username, couchDBDef.Password,
-				couchDBDef.MaxRetries, couchDBDef.MaxRetriesOnStartup, couchDBDef.RequestTimeout)
+				maxRetries, couchDBDef.MaxRetriesOnStartup, couchDBDef.RequestTimeout)
 			testutil.AssertNoError(t, err, fmt.Sprintf("Error when trying to create couch instance"))
 			db := CouchDatabase{CouchInstance: *couchInstance, DBName: database}
 
@@ -667,7 +682,7 @@ func TestDBBadNumberOfRetries(t *testing.T) {
 
 		//create a new instance and database object
 		_, err = CreateCouchInstance(couchDBDef.URL, couchDBDef.Username, couchDBDef.Password,
-			0, 3, couchDBDef.RequestTimeout)
+			-1, 3, couchDBDef.RequestTimeout)
 		testutil.AssertError(t, err, fmt.Sprintf("Error should have been thrown while attempting to create a database"))
 
 	}
@@ -1339,7 +1354,7 @@ func TestRichQuery(t *testing.T) {
 	}
 }
 
-func TestBatchBatchOperations(t *testing.T) {
+func testBatchBatchOperations(t *testing.T, maxRetries int) {
 
 	if ledgerconfig.IsCouchDBEnabled() {
 
@@ -1399,7 +1414,7 @@ func TestBatchBatchOperations(t *testing.T) {
 
 		//create a new instance and database object   --------------------------------------------------------
 		couchInstance, err := CreateCouchInstance(couchDBDef.URL, couchDBDef.Username, couchDBDef.Password,
-			couchDBDef.MaxRetries, couchDBDef.MaxRetriesOnStartup, couchDBDef.RequestTimeout)
+			maxRetries, couchDBDef.MaxRetriesOnStartup, couchDBDef.RequestTimeout)
 		testutil.AssertNoError(t, err, fmt.Sprintf("Error when trying to create couch instance"))
 		db := CouchDatabase{CouchInstance: *couchInstance, DBName: database}