From ede57181740395772514e174ec760307130c7045 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Sat, 19 Mar 2016 17:32:26 -0700 Subject: [PATCH] Don't treat arbitrary strings as format strings. --- .../fakecacheservice/fakecacheservice.go | 25 ++++++++++--------- .../fakecacheservice/fakecacheservice_test.go | 2 +- .../tabletconntest/tabletconntest.go | 3 ++- go/vt/worker/split_diff.go | 18 ++++++------- go/vt/worker/vertical_split_diff.go | 14 +++++------ go/zk/zkctl/zkctl.go | 2 +- 6 files changed, 33 insertions(+), 31 deletions(-) diff --git a/go/vt/tabletserver/fakecacheservice/fakecacheservice.go b/go/vt/tabletserver/fakecacheservice/fakecacheservice.go index efd57da75a..843e105f5b 100644 --- a/go/vt/tabletserver/fakecacheservice/fakecacheservice.go +++ b/go/vt/tabletserver/fakecacheservice/fakecacheservice.go @@ -6,6 +6,7 @@ package fakecacheservice import ( + "errors" "fmt" "math/rand" "sync" @@ -15,7 +16,7 @@ import ( "github.com/youtube/vitess/go/sync2" ) -var errCacheService = "cacheservice error" +var errCacheService = errors.New("cacheservice error") // FakeCacheService is a fake implementation of CacheService type FakeCacheService struct { @@ -85,7 +86,7 @@ func NewFakeCacheService(cache *Cache) *FakeCacheService { // Get returns cached data for given keys. func (service *FakeCacheService) Get(keys ...string) ([]cs.Result, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return nil, fmt.Errorf(errCacheService) + return nil, errCacheService } results := make([]cs.Result, 0, len(keys)) for _, key := range keys { @@ -101,7 +102,7 @@ func (service *FakeCacheService) Get(keys ...string) ([]cs.Result, error) { // the item's CAS value has changed since you Gets'ed it, it will not be stored. func (service *FakeCacheService) Gets(keys ...string) ([]cs.Result, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return nil, fmt.Errorf(errCacheService) + return nil, errCacheService } results := make([]cs.Result, 0, len(keys)) for _, key := range keys { @@ -117,7 +118,7 @@ func (service *FakeCacheService) Gets(keys ...string) ([]cs.Result, error) { // Set set the value with specified cache key. func (service *FakeCacheService) Set(key string, flags uint16, timeout uint64, value []byte) (bool, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return false, fmt.Errorf(errCacheService) + return false, errCacheService } service.cache.Set(key, &cs.Result{ Key: key, @@ -131,7 +132,7 @@ func (service *FakeCacheService) Set(key string, flags uint16, timeout uint64, v // Add store the value only if it does not already exist. func (service *FakeCacheService) Add(key string, flags uint16, timeout uint64, value []byte) (bool, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return false, fmt.Errorf(errCacheService) + return false, errCacheService } if _, ok := service.cache.Get(key); ok { return false, nil @@ -149,7 +150,7 @@ func (service *FakeCacheService) Add(key string, flags uint16, timeout uint64, v // for the specified cache key. func (service *FakeCacheService) Replace(key string, flags uint16, timeout uint64, value []byte) (bool, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return false, fmt.Errorf(errCacheService) + return false, errCacheService } result, ok := service.cache.Get(key) if !ok { @@ -164,7 +165,7 @@ func (service *FakeCacheService) Replace(key string, flags uint16, timeout uint6 // Append appends the value after the last bytes in an existing item. func (service *FakeCacheService) Append(key string, flags uint16, timeout uint64, value []byte) (bool, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return false, fmt.Errorf(errCacheService) + return false, errCacheService } result, ok := service.cache.Get(key) if !ok { @@ -179,7 +180,7 @@ func (service *FakeCacheService) Append(key string, flags uint16, timeout uint64 // Prepend prepends the value before existing value. func (service *FakeCacheService) Prepend(key string, flags uint16, timeout uint64, value []byte) (bool, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return false, fmt.Errorf(errCacheService) + return false, errCacheService } result, ok := service.cache.Get(key) if !ok { @@ -194,7 +195,7 @@ func (service *FakeCacheService) Prepend(key string, flags uint16, timeout uint6 // Cas stores the value only if no one else has updated the data since you read it last. func (service *FakeCacheService) Cas(key string, flags uint16, timeout uint64, value []byte, cas uint64) (bool, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return false, fmt.Errorf(errCacheService) + return false, errCacheService } result, ok := service.cache.Get(key) if !ok || result.Cas != cas { @@ -210,7 +211,7 @@ func (service *FakeCacheService) Cas(key string, flags uint16, timeout uint64, v // Delete delete the value for the specified cache key. func (service *FakeCacheService) Delete(key string) (bool, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return false, fmt.Errorf(errCacheService) + return false, errCacheService } service.cache.Delete(key) return true, nil @@ -219,7 +220,7 @@ func (service *FakeCacheService) Delete(key string) (bool, error) { // FlushAll purges the entire cache. func (service *FakeCacheService) FlushAll() error { if service.cache.enableCacheServiceError.Get() == 1 { - return fmt.Errorf(errCacheService) + return errCacheService } service.cache.Clear() return nil @@ -228,7 +229,7 @@ func (service *FakeCacheService) FlushAll() error { // Stats returns a list of basic stats. func (service *FakeCacheService) Stats(key string) ([]byte, error) { if service.cache.enableCacheServiceError.Get() == 1 { - return nil, fmt.Errorf(errCacheService) + return nil, errCacheService } return []byte{}, nil } diff --git a/go/vt/tabletserver/fakecacheservice/fakecacheservice_test.go b/go/vt/tabletserver/fakecacheservice/fakecacheservice_test.go index 9e813bc5d9..bf09f37a4b 100644 --- a/go/vt/tabletserver/fakecacheservice/fakecacheservice_test.go +++ b/go/vt/tabletserver/fakecacheservice/fakecacheservice_test.go @@ -165,7 +165,7 @@ func TestFakeCacheServiceError(t *testing.T) { } func checkCacheServiceError(t *testing.T, err error) { - if err.Error() != errCacheService { + if err != errCacheService { t.Fatalf("should get cacheservice error") } } diff --git a/go/vt/tabletserver/tabletconntest/tabletconntest.go b/go/vt/tabletserver/tabletconntest/tabletconntest.go index b72a0baa02..9f165eb7da 100644 --- a/go/vt/tabletserver/tabletconntest/tabletconntest.go +++ b/go/vt/tabletserver/tabletconntest/tabletconntest.go @@ -7,6 +7,7 @@ package tabletconntest import ( + "errors" "fmt" "reflect" "strings" @@ -717,7 +718,7 @@ var testStreamHealthErrorMsg = "to trigger a server error" // StreamHealthRegister is part of the queryservice.QueryService interface func (f *FakeQueryService) StreamHealthRegister(c chan<- *querypb.StreamHealthResponse) (int, error) { if f.hasError { - return 0, fmt.Errorf(testStreamHealthErrorMsg) + return 0, errors.New(testStreamHealthErrorMsg) } if f.panics { panic(fmt.Errorf("test-triggered panic")) diff --git a/go/vt/worker/split_diff.go b/go/vt/worker/split_diff.go index a2f1183877..618f1672de 100644 --- a/go/vt/worker/split_diff.go +++ b/go/vt/worker/split_diff.go @@ -341,7 +341,7 @@ func (sdw *SplitDiffWorker) diff(ctx context.Context) error { sdw.wr.Logger().Infof("Gathering schema information...") sdw.sourceSchemaDefinitions = make([]*tabletmanagerdatapb.SchemaDefinition, len(sdw.sourceAliases)) wg := sync.WaitGroup{} - rec := concurrency.AllErrorRecorder{} + rec := &concurrency.AllErrorRecorder{} wg.Add(1) go func() { var err error @@ -374,10 +374,10 @@ func (sdw *SplitDiffWorker) diff(ctx context.Context) error { // TODO(alainjobart) Checking against each source may be // overkill, if all sources have the same schema? sdw.wr.Logger().Infof("Diffing the schema...") - rec = concurrency.AllErrorRecorder{} + rec = &concurrency.AllErrorRecorder{} for i, sourceSchemaDefinition := range sdw.sourceSchemaDefinitions { sourceName := fmt.Sprintf("source[%v]", i) - tmutils.DiffSchema("destination", sdw.destinationSchemaDefinition, sourceName, sourceSchemaDefinition, &rec) + tmutils.DiffSchema("destination", sdw.destinationSchemaDefinition, sourceName, sourceSchemaDefinition, rec) } if rec.HasErrors() { sdw.wr.Logger().Warningf("Different schemas: %v", rec.Error().Error()) @@ -399,7 +399,7 @@ func (sdw *SplitDiffWorker) diff(ctx context.Context) error { if len(sdw.sourceAliases) != 1 { err := fmt.Errorf("Don't support more than one source for table yet: %v", tableDefinition.Name) rec.RecordError(err) - sdw.wr.Logger().Errorf(err.Error()) + sdw.wr.Logger().Errorf("%v", err) return } @@ -407,14 +407,14 @@ func (sdw *SplitDiffWorker) diff(ctx context.Context) error { if err != nil { newErr := fmt.Errorf("Source shard doesn't overlap with destination????: %v", err) rec.RecordError(newErr) - sdw.wr.Logger().Errorf(newErr.Error()) + sdw.wr.Logger().Errorf("%v", newErr) return } sourceQueryResultReader, err := TableScanByKeyRange(ctx, sdw.wr.Logger(), sdw.wr.TopoServer(), sdw.sourceAliases[0], tableDefinition, overlap, sdw.keyspaceInfo.ShardingColumnType) if err != nil { newErr := fmt.Errorf("TableScanByKeyRange(source) failed: %v", err) rec.RecordError(newErr) - sdw.wr.Logger().Errorf(newErr.Error()) + sdw.wr.Logger().Errorf("%v", newErr) return } defer sourceQueryResultReader.Close() @@ -423,7 +423,7 @@ func (sdw *SplitDiffWorker) diff(ctx context.Context) error { if err != nil { newErr := fmt.Errorf("TableScanByKeyRange(destination) failed: %v", err) rec.RecordError(newErr) - sdw.wr.Logger().Errorf(newErr.Error()) + sdw.wr.Logger().Errorf("%v", newErr) return } defer destinationQueryResultReader.Close() @@ -432,7 +432,7 @@ func (sdw *SplitDiffWorker) diff(ctx context.Context) error { if err != nil { newErr := fmt.Errorf("NewRowDiffer() failed: %v", err) rec.RecordError(newErr) - sdw.wr.Logger().Errorf(newErr.Error()) + sdw.wr.Logger().Errorf("%v", newErr) return } @@ -440,7 +440,7 @@ func (sdw *SplitDiffWorker) diff(ctx context.Context) error { if err != nil { newErr := fmt.Errorf("Differ.Go failed: %v", err.Error()) rec.RecordError(newErr) - sdw.wr.Logger().Errorf(newErr.Error()) + sdw.wr.Logger().Errorf("%v", newErr) } else { if report.HasDifferences() { err := fmt.Errorf("Table %v has differences: %v", tableDefinition.Name, report.String()) diff --git a/go/vt/worker/vertical_split_diff.go b/go/vt/worker/vertical_split_diff.go index 6fad962f94..964162176d 100644 --- a/go/vt/worker/vertical_split_diff.go +++ b/go/vt/worker/vertical_split_diff.go @@ -336,7 +336,7 @@ func (vsdw *VerticalSplitDiffWorker) diff(ctx context.Context) error { vsdw.wr.Logger().Infof("Gathering schema information...") wg := sync.WaitGroup{} - rec := concurrency.AllErrorRecorder{} + rec := &concurrency.AllErrorRecorder{} wg.Add(1) go func() { var err error @@ -366,8 +366,8 @@ func (vsdw *VerticalSplitDiffWorker) diff(ctx context.Context) error { // Check the schema vsdw.wr.Logger().Infof("Diffing the schema...") - rec = concurrency.AllErrorRecorder{} - tmutils.DiffSchema("destination", vsdw.destinationSchemaDefinition, "source", vsdw.sourceSchemaDefinition, &rec) + rec = &concurrency.AllErrorRecorder{} + tmutils.DiffSchema("destination", vsdw.destinationSchemaDefinition, "source", vsdw.sourceSchemaDefinition, rec) if rec.HasErrors() { vsdw.wr.Logger().Warningf("Different schemas: %v", rec.Error()) } else { @@ -389,7 +389,7 @@ func (vsdw *VerticalSplitDiffWorker) diff(ctx context.Context) error { if err != nil { newErr := fmt.Errorf("TableScan(source) failed: %v", err) rec.RecordError(newErr) - vsdw.wr.Logger().Errorf(newErr.Error()) + vsdw.wr.Logger().Errorf("%v", newErr) return } defer sourceQueryResultReader.Close() @@ -398,7 +398,7 @@ func (vsdw *VerticalSplitDiffWorker) diff(ctx context.Context) error { if err != nil { newErr := fmt.Errorf("TableScan(destination) failed: %v", err) rec.RecordError(newErr) - vsdw.wr.Logger().Errorf(newErr.Error()) + vsdw.wr.Logger().Errorf("%v", newErr) return } defer destinationQueryResultReader.Close() @@ -407,7 +407,7 @@ func (vsdw *VerticalSplitDiffWorker) diff(ctx context.Context) error { if err != nil { newErr := fmt.Errorf("NewRowDiffer() failed: %v", err) rec.RecordError(newErr) - vsdw.wr.Logger().Errorf(newErr.Error()) + vsdw.wr.Logger().Errorf("%v", newErr) return } @@ -418,7 +418,7 @@ func (vsdw *VerticalSplitDiffWorker) diff(ctx context.Context) error { if report.HasDifferences() { err := fmt.Errorf("Table %v has differences: %v", tableDefinition.Name, report.String()) rec.RecordError(err) - vsdw.wr.Logger().Errorf(err.Error()) + vsdw.wr.Logger().Errorf("%v", err) } else { vsdw.wr.Logger().Infof("Table %v checks out (%v rows processed, %v qps)", tableDefinition.Name, report.processedRows, report.processingQPS) } diff --git a/go/zk/zkctl/zkctl.go b/go/zk/zkctl/zkctl.go index d85a321cb0..1f7cfd48f4 100644 --- a/go/zk/zkctl/zkctl.go +++ b/go/zk/zkctl/zkctl.go @@ -185,7 +185,7 @@ func (zkd *Zkd) init(preserveData bool) error { log.Infof("zkd.Init") for _, path := range zkd.config.DirectoryList() { if err := os.MkdirAll(path, 0775); err != nil { - log.Errorf(err.Error()) + log.Errorf("%v", err) return err } // FIXME(msolomon) validate permissions?