gemini: Improved validation error handling

We no longer ignore possible CQL errors by silently ignoring iterator close errors. We do no longer over book write ops count in case there is a mutation error.
scylladb · May 2, 2019 · 9f49fc5 · 9f49fc5
1 parent 68ddb47
commit 9f49fc5
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Improved error handling in validation code.
+- Avoiding small double booking of write ops in case of mutation errors.
 - Printing executable CQL statements when logging errors or in verbose mode.
 - JSON schema definition file has simpler index definition.
 

diff --git a/cmd/gemini/root.go b/cmd/gemini/root.go
@@ -265,14 +265,15 @@ func mutationJob(schema *gemini.Schema, table gemini.Table, s *gemini.Session, p
 	if verbose {
 		fmt.Println(mutateStmt.PrettyCQL())
 	}
-	testStatus.WriteOps++
 	if err := s.Mutate(mutateQuery, mutateValues...); err != nil {
 		e := gemini.JobError{
 			Message: "Mutation failed: " + err.Error(),
 			Query:   mutateStmt.PrettyCQL(),
 		}
 		testStatus.Errors = append(testStatus.Errors, e)
 		testStatus.WriteErrors++
+	} else {
+		testStatus.WriteOps++
 	}
 }
 
@@ -283,19 +284,16 @@ func validationJob(schema *gemini.Schema, table gemini.Table, s *gemini.Session,
 	if verbose {
 		fmt.Println(checkStmt.PrettyCQL())
 	}
-	err := s.Check(table, checkQuery, checkValues...)
-	if err == nil {
-		testStatus.ReadOps++
-	} else {
-		if err != gemini.ErrReadNoDataReturned {
-			// De-duplication needed?
-			e := gemini.JobError{
-				Message: "Validation failed: " + err.Error(),
-				Query:   checkStmt.PrettyCQL(),
-			}
-			testStatus.Errors = append(testStatus.Errors, e)
-			testStatus.ReadErrors++
+	if err := s.Check(table, checkQuery, checkValues...); err != nil {
+		// De-duplication needed?
+		e := gemini.JobError{
+			Message: "Validation failed: " + err.Error(),
+			Query:   checkStmt.PrettyCQL(),
 		}
+		testStatus.Errors = append(testStatus.Errors, e)
+		testStatus.ReadErrors++
+	} else {
+		testStatus.ReadOps++
 	}
 }
 

diff --git a/go.mod b/go.mod
@@ -19,6 +19,8 @@ require (
 	github.com/spf13/cobra v0.0.3
 	github.com/spf13/pflag v1.0.3 // indirect
 	github.com/stretchr/testify v1.3.0 // indirect
+	go.uber.org/atomic v1.4.0 // indirect
+	go.uber.org/multierr v1.1.0
 	golang.org/x/net v0.0.0-20190313082753-5c2c250b6a70
 	gopkg.in/inf.v0 v0.9.1
 )

diff --git a/go.sum b/go.sum
@@ -44,6 +44,10 @@ github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnIn
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
+go.uber.org/atomic v1.4.0 h1:cxzIVoETapQEqDhQu3QfnvXAV4AlzcvUCxkVUFw3+EU=
+go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
+go.uber.org/multierr v1.1.0 h1:HoEmRHQPVSqub6w2z2d2EOVs2fjyFRGyofhKuyDq0QI=
+go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/net v0.0.0-20190313082753-5c2c250b6a70 h1:0OwHPyvXNyZS9VW4XXoGkWOwhrMN52Y4n/gSxvJOgj0=
 golang.org/x/net v0.0.0-20190313082753-5c2c250b6a70/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=

diff --git a/session.go b/session.go
@@ -1,7 +1,6 @@
 package gemini
 
 import (
-	"errors"
 	"fmt"
 	"math/big"
 	"sort"
@@ -12,6 +11,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/scylladb/go-set/strset"
+	"go.uber.org/multierr"
 	"gopkg.in/inf.v0"
 )
 
@@ -21,10 +21,6 @@ type Session struct {
 	schema        *Schema
 }
 
-var (
-	ErrReadNoDataReturned = errors.New("read: no data returned")
-)
-
 type JobError struct {
 	Message string `json:"message"`
 	Query   string `json:"query"`
@@ -66,18 +62,19 @@ func (s *Session) Mutate(query string, values ...interface{}) error {
 	return nil
 }
 
-func (s *Session) Check(table Table, query string, values ...interface{}) error {
+func (s *Session) Check(table Table, query string, values ...interface{}) (err error) {
 	testIter := s.testSession.Query(query, values...).Iter()
 	oracleIter := s.oracleSession.Query(query, values...).Iter()
 	defer func() {
-		testIter.Close()
-		oracleIter.Close()
+		err = multierr.Append(err, testIter.Close())
+		err = multierr.Append(err, oracleIter.Close())
 	}()
 
 	testRows := loadSet(testIter)
 	oracleRows := loadSet(oracleIter)
 	if len(testRows) == 0 && len(oracleRows) == 0 {
-		return ErrReadNoDataReturned
+		// Both empty is fine
+		return nil
 	}
 	if len(testRows) != len(oracleRows) {
 		testSet := strset.New(pks(table, testRows)...)