diff --git a/dbshard.go b/dbshard.go index 606fe75f2..a86cc4ab7 100644 --- a/dbshard.go +++ b/dbshard.go @@ -1228,6 +1228,10 @@ func (vs *FieldView2Shards) String() (r string) { return } +func (vs *FieldView2Shards) removeField(name string) { + delete(vs.m, name) +} + // Note: cannot call this during migration, because // it only ever returns the green shards if we are in blue-green. func (per *DBPerShard) GetFieldView2ShardsMapForIndex(idx *Index) (vs *FieldView2Shards, err error) { diff --git a/index.go b/index.go index 83a4e8e4a..4cfd301b2 100644 --- a/index.go +++ b/index.go @@ -805,6 +805,9 @@ func (i *Index) DeleteField(name string) error { // Remove reference. delete(i.fields, name) + // remove shard metadata for field + i.fieldView2shard.removeField(name) + // Delete the field from etcd as the system of record. if err := i.Schemator.DeleteField(context.TODO(), i.name, name); err != nil { return errors.Wrapf(err, "deleting field from etcd: %s/%s", i.name, name) diff --git a/index_test.go b/index_test.go index 890d481d1..f1a12b55c 100644 --- a/index_test.go +++ b/index_test.go @@ -15,10 +15,16 @@ package pilosa_test import ( + "context" + "fmt" + "math/rand" + "os" "reflect" "testing" + "time" - "github.com/molecula/featurebase/v2" + pilosa "github.com/molecula/featurebase/v2" + "github.com/molecula/featurebase/v2/disco" "github.com/molecula/featurebase/v2/pql" "github.com/molecula/featurebase/v2/test" "github.com/molecula/featurebase/v2/testhook" @@ -275,3 +281,80 @@ func isNotFoundError(err error) bool { _, ok := root.(pilosa.NotFoundError) return ok } + +// Ensure that after node/cluster restart, deleting and recreating a field +// does not cause a deadlock +// This is a regression test after a customer experienced the same deadlock. +// For details, check out https://molecula.atlassian.net/browse/CORE-919 +func TestIndex_RecreateFieldOnRestart(t *testing.T) { + c := test.MustRunCluster(t, 1) + defer c.Close() + + // create index + indexName := fmt.Sprintf("idx_%d", rand.Uint64()) + holder := c.GetHolder(0) + index, err := holder.CreateIndex(indexName, pilosa.IndexOptions{ + Keys: false, + }) + if err != nil { + t.Fatal(err) + } + defer index.Close() + + // create field + fieldName := fmt.Sprintf("field_%d", rand.Uint64()) + _, err = c.GetNode(0).API.CreateField(context.Background(), indexName, fieldName, + pilosa.OptFieldTypeDefault()) + if err != nil { + t.Fatal(err) + } + + // set value + _, err = c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{ + Index: indexName, + Query: fmt.Sprintf(`Set(1, %s=1)`, fieldName), + }) + if err != nil { + t.Fatal(err) + } + + // restart node + node := c.GetNode(0) + if err := node.Reopen(); err != nil { + t.Fatal(err) + } + if err := c.AwaitState(disco.ClusterStateNormal, 10*time.Second); err != nil { + t.Fatalf("restarting cluster: %v", err) + } + + // delete field + err = c.GetNode(0).API.DeleteField(context.Background(), indexName, fieldName) + if err != nil { + t.Fatal(err) + } + + // recreate field + errCh := make(chan error) + go func() { + _, err := c.GetNode(0).API.CreateField(context.Background(), indexName, + fieldName, pilosa.OptFieldTypeDefault()) + errCh <- err + }() + select { + case <-time.After(10 * time.Second): + // We have to use os.Exit here instead of t.Fatal or panic since + // on panic, deferred statements are still ran. Given that + // we have deferred cluster.Close(), it deadlocks on the same + // issue this test is, well, is testing on. + // With os.Exit, the process exits at that point without running the + // deferred actions. This is more of a work-around fix to make the + // test meaningful on timeout. + t.Logf("recreating field took too long") + os.Exit(1) + case err := <-errCh: + if err != nil { + t.Fatal(err) + } + } + +}