From 04e95e1ec74e9d6cf5494f652c02233666d8f71d Mon Sep 17 00:00:00 2001 From: Chris Elder Date: Thu, 1 Feb 2018 13:22:24 -0500 Subject: [PATCH] [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 --- core/ledger/util/couchdb/couchdb.go | 78 ++++++++++++++---------- core/ledger/util/couchdb/couchdb_test.go | 25 ++++++-- 2 files changed, 66 insertions(+), 37 deletions(-) diff --git a/core/ledger/util/couchdb/couchdb.go b/core/ledger/util/couchdb/couchdb.go index c8e798cd7c0..9efa9782480 100644 --- 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 index b05f117c9a1..95e97d76985 100644 --- 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}