From edc7a9822f44b77f6bcd47cc2fc593d107b4e3f6 Mon Sep 17 00:00:00 2001
From: reesporte <reesedporter@gmail.com>
Date: Thu, 23 Jun 2022 10:33:16 -0500
Subject: [PATCH] delete things from holder after removal from etcd

This way we can't get into an inconsistent state.

Consider this example:
You have a 3-node cluster, nodes A, B and C.
You create an index "blah" while all three nodes are up.
Nodes B and C go down.
You attempt to delete the index. It is removed from node A's holder, but is not removed from nodes B and C.
When nodes B and C are restarted, the schema still shows this "blah" index.
If you attempt to delete the index from node A, you receive an index not found error, but the schema indicates the index exists.

With this change however, when you first attempt to delete the index, it is not removed from the holder until there is enough nodes up to achieve consensus.

The same situation applies to fields and views.
---
 field.go  | 10 +++++-----
 holder.go | 10 +++++-----
 index.go  | 11 +++++------
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/field.go b/field.go
index 90e03640e..d2bd17223 100644
--- a/field.go
+++ b/field.go
@@ -1147,6 +1147,11 @@ func (f *Field) deleteView(name string) error {
 		return ErrInvalidView
 	}
 
+	// Delete the view from etcd as the system of record.
+	if err := f.schemator.DeleteView(context.TODO(), f.index, f.name, name); err != nil {
+		return errors.Wrapf(err, "deleting view from etcd: %s/%s/%s", f.index, f.name, name)
+	}
+
 	// Close data files before deletion.
 	if err := view.close(); err != nil {
 		return errors.Wrap(err, "closing view")
@@ -1159,11 +1164,6 @@ func (f *Field) deleteView(name string) error {
 
 	delete(f.viewMap, name)
 
-	// Delete the view from etcd as the system of record.
-	if err := f.schemator.DeleteView(context.TODO(), f.index, f.name, name); err != nil {
-		return errors.Wrapf(err, "deleting view from etcd: %s/%s/%s", f.index, f.name, name)
-	}
-
 	return nil
 }
 
diff --git a/holder.go b/holder.go
index c8100bc17..bfa2fcb5d 100644
--- a/holder.go
+++ b/holder.go
@@ -1056,6 +1056,11 @@ func (h *Holder) DeleteIndex(name string) error {
 		return newNotFoundError(ErrIndexNotFound, name)
 	}
 
+	// Delete the index from etcd as the system of record.
+	if err := h.schemator.DeleteIndex(context.TODO(), name); err != nil {
+		return errors.Wrapf(err, "deleting index from etcd: %s", name)
+	}
+
 	// Close index.
 	if err := index.Close(); err != nil {
 		return errors.Wrap(err, "closing")
@@ -1083,11 +1088,6 @@ func (h *Holder) DeleteIndex(name string) error {
 	// Remove reference.
 	h.deleteIndex(name)
 
-	// Delete the index from etcd as the system of record.
-	if err := h.schemator.DeleteIndex(context.TODO(), name); err != nil {
-		return errors.Wrapf(err, "deleting index from etcd: %s", name)
-	}
-
 	// I'm not sure if calling Reset() here is necessary
 	// since closing the index stops its translation
 	// sync processes.
diff --git a/index.go b/index.go
index 21c85b7fd..7c05e8c66 100644
--- a/index.go
+++ b/index.go
@@ -897,6 +897,11 @@ func (i *Index) DeleteField(name string) error {
 		return newNotFoundError(ErrFieldNotFound, 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)
+	}
+
 	// Close field.
 	if err := f.Close(); err != nil {
 		return errors.Wrap(err, "closing")
@@ -911,12 +916,6 @@ func (i *Index) DeleteField(name string) error {
 
 	// 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)
-	}
-
 	return i.translationSyncer.Reset()
 }