From cd2130893b0d731cb0347130b967c24189188d3d Mon Sep 17 00:00:00 2001 From: Gurkanwal Singh Batra Date: Wed, 4 Dec 2019 15:16:15 -0800 Subject: [PATCH 1/4] return descriptive error when a nil state is passed into JSONFileStore --- consumer/store_json_file.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/consumer/store_json_file.go b/consumer/store_json_file.go index 003b12a7..4411aab9 100644 --- a/consumer/store_json_file.go +++ b/consumer/store_json_file.go @@ -33,6 +33,9 @@ var _ Store = &JSONFileStore{} // JSONFileStore is-a Store. // of the Store's state, which is decoded into, encoded from, and retained // as JSONFileState.State. func NewJSONFileStore(rec *recoverylog.Recorder, state interface{}) (*JSONFileStore, error) { + if state == nil { + return nil, errors.New("state must not be nil") + } var store = &JSONFileStore{ State: state, fs: recoverylog.RecordedAferoFS{Recorder: rec, Fs: afero.NewOsFs()}, From 942b89fe43ac40e10b13f359c37406917b3dd337 Mon Sep 17 00:00:00 2001 From: Gurkanwal Singh Batra Date: Wed, 18 Dec 2019 01:52:08 -0800 Subject: [PATCH 2/4] - add test - refactor godoc comment --- consumer/shard_test.go | 13 +++++++++++++ consumer/store_json_file.go | 6 ++++-- consumer/test_support_test.go | 5 ++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/consumer/shard_test.go b/consumer/shard_test.go index 8dc15678..06f5f85c 100644 --- a/consumer/shard_test.go +++ b/consumer/shard_test.go @@ -156,6 +156,19 @@ func TestShardAppNewStoreError(t *testing.T) { tf.allocateShard(makeShard(shardA)) // Cleanup. } +func TestShardAppNilJSONStore(t *testing.T) { + var tf, cleanup = newTestFixture(t) + defer cleanup() + + tf.app.nilStore = true + tf.allocateShard(makeShard(shardA), localID) + + assert.Equal(t, "completeRecovery: app.NewStore: state must not be nil", + expectStatusCode(t, tf.state, pc.ReplicaStatus_FAILED).Errors[0]) + + tf.allocateShard(makeShard(shardA)) // Cleanup. +} + func TestShardAppNewMessageFails(t *testing.T) { var tf, cleanup = newTestFixture(t) defer cleanup() diff --git a/consumer/store_json_file.go b/consumer/store_json_file.go index 4411aab9..01f1c10c 100644 --- a/consumer/store_json_file.go +++ b/consumer/store_json_file.go @@ -31,7 +31,7 @@ var _ Store = &JSONFileStore{} // JSONFileStore is-a Store. // NewJSONFileStore returns a new JSONFileStore. |state| is the runtime instance // of the Store's state, which is decoded into, encoded from, and retained -// as JSONFileState.State. +// as JSONFileState.State. |state| must not be nil. func NewJSONFileStore(rec *recoverylog.Recorder, state interface{}) (*JSONFileStore, error) { if state == nil { return nil, errors.New("state must not be nil") @@ -117,7 +117,9 @@ func (s *JSONFileStore) StartCommit(_ Shard, cp pc.Checkpoint, waitFor OpFutures // Destroy the JSONFileStore directory and state file. func (s *JSONFileStore) Destroy() { - if err := os.RemoveAll(s.recorder.Dir); err != nil { + if s == nil { + return + } else if err := os.RemoveAll(s.recorder.Dir); err != nil { log.WithFields(log.Fields{ "dir": s.recorder.Dir, "err": err, diff --git a/consumer/test_support_test.go b/consumer/test_support_test.go index 46a72c5e..46368dd9 100644 --- a/consumer/test_support_test.go +++ b/consumer/test_support_test.go @@ -83,6 +83,7 @@ type testApplication struct { finalizeErr error // Error returned by Application.FinalizeTxn(). startCommitErr error // Error returned by Store.StartCommit(). restoreCheckpointErr error // Error returned by Store.RestoreCheckpoint(). + nilStore bool // Whether the application initializes its store in Application.NewStore(). finishedCh chan OpFuture // Signaled on FinishedTxn(). db *sql.DB // "Remote" sqlite database. } @@ -114,9 +115,11 @@ func (a *testApplication) NewStore(shard Shard, rec *recoverylog.Recorder) (Stor return &errStore{app: a}, nil } else if rec == nil { return NewSQLStore(a.db), nil + } else if a.nilStore { + return NewJSONFileStore(rec, nil) } else { var state = make(map[string]string) - return NewJSONFileStore(rec, &state) + return NewJSONFileStore(rec, state) } } From d1b498f592c8cb9689700ee4f507acacdb9230a1 Mon Sep 17 00:00:00 2001 From: Gurkanwal Singh Batra Date: Wed, 18 Dec 2019 14:22:33 -0800 Subject: [PATCH 3/4] - extra nil check --- consumer/shard.go | 3 ++- consumer/store_json_file.go | 4 +--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/consumer/shard.go b/consumer/shard.go index ab79790d..710b9768 100644 --- a/consumer/shard.go +++ b/consumer/shard.go @@ -2,6 +2,7 @@ package consumer import ( "context" + "reflect" "sync" "time" @@ -244,7 +245,7 @@ func servePrimary(s *shard) (err error) { func waitAndTearDown(s *shard, done func()) { s.wg.Wait() - if s.store != nil { + if s.store != nil && !reflect.ValueOf(s.store).IsNil() { s.store.Destroy() } done() diff --git a/consumer/store_json_file.go b/consumer/store_json_file.go index 01f1c10c..76619a9e 100644 --- a/consumer/store_json_file.go +++ b/consumer/store_json_file.go @@ -117,9 +117,7 @@ func (s *JSONFileStore) StartCommit(_ Shard, cp pc.Checkpoint, waitFor OpFutures // Destroy the JSONFileStore directory and state file. func (s *JSONFileStore) Destroy() { - if s == nil { - return - } else if err := os.RemoveAll(s.recorder.Dir); err != nil { + if err := os.RemoveAll(s.recorder.Dir); err != nil { log.WithFields(log.Fields{ "dir": s.recorder.Dir, "err": err, From 8555b58aa0b2e74de1390fd94a5d27e0e08d11a5 Mon Sep 17 00:00:00 2001 From: Gurkanwal Singh Batra Date: Wed, 18 Dec 2019 14:47:53 -0800 Subject: [PATCH 4/4] - nit --- consumer/test_support_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consumer/test_support_test.go b/consumer/test_support_test.go index 46368dd9..88151476 100644 --- a/consumer/test_support_test.go +++ b/consumer/test_support_test.go @@ -119,7 +119,7 @@ func (a *testApplication) NewStore(shard Shard, rec *recoverylog.Recorder) (Stor return NewJSONFileStore(rec, nil) } else { var state = make(map[string]string) - return NewJSONFileStore(rec, state) + return NewJSONFileStore(rec, &state) } }