From fb8f612afebc25984d02999fac5dfa3aa37a0743 Mon Sep 17 00:00:00 2001 From: Travis Date: Thu, 27 Feb 2020 12:57:50 -0600 Subject: [PATCH 1/2] very crude fix for the translate key read-only bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR forces the non-coordinator nodes to reset their translation sync (and therefore their own cosideration of read-only partitions) any time they receive a `ClusterStatus` message. So basically, as the cluster grows during the startup process, each node will reset their translation sync. This is NOT a good solution log term, but it should address the immediate problem. Things to note: - the coordinator sync isn't getting reset, but that's ok, because the immediate problem is a partition marked as read-only when it shouldn't be; i.e. it's ok to have the inverse (a partition not marked as read-only when it should be) because that partition won't receive requests anyway. - the last node to start is already correct and doesn't really need to reset its sync. - there are many other scenarios not covered by this fix. Based on this theory: ``` i have another theory that i’m going to try to test. this one would only apply in the case where a multi-node cluster is restarted with an existing, keyed index. - start node0: it thinks it’s responsible for all partitions (nothing is read-only) - start node1: it thinks it’s responsible for ~1/2 of the partitions and marks the other 1/2 as read-only - start node2: it thinks it’s responsible for ~1/3 of the partitions and marks the other 2/3 as read-only now if node0 is the coordinator receiving all translation requests, that still might not explain what’s happening, because in that case it would just do all the translating. i think. but either way, i should make sure that scenario is not happening, but i think it may be. actually, that might explain it, because what would happen when the coordinator received a translation request, is that it would handle the 1/3 that it owned (now that the cluster is 3 nodes), and it would send the other 2/3 out to the other 2 nodes. but where it sent the requests wouldn’t line up with what the nodes thought they were responsible for based on the restart order in this example, node 1 would receive requests for the wrong partitions ``` --- server.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server.go b/server.go index 455e4dd59..5c97dd82c 100644 --- a/server.go +++ b/server.go @@ -725,6 +725,11 @@ func (s *Server) receiveMessage(m Message) error { if err != nil { return err } + if s.syncer.Cluster != nil { + if err := s.syncer.ResetTranslationSync(); err != nil { + return err + } + } case *ResizeInstruction: err := s.cluster.followResizeInstruction(obj) if err != nil { From 717bd09e9715f8f2e5decb0538987fff7ce62c52 Mon Sep 17 00:00:00 2001 From: Travis Date: Sat, 29 Feb 2020 08:41:40 -0600 Subject: [PATCH 2/2] include the test which covers this scenario --- test/pilosa.go | 14 ++++++ translator_test.go | 116 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/test/pilosa.go b/test/pilosa.go index 02cfd3722..3896b7ce0 100644 --- a/test/pilosa.go +++ b/test/pilosa.go @@ -138,6 +138,20 @@ func (m *Command) Reopen() error { return m.Start() } +// SoftOpen is like Reopen, but doesn't close the program first. +// This is useful in the case where a test needs to decouple the +// close from the re-open (for example, there may need to be +// actions which take place between those two steps). +func (m *Command) SoftOpen() error { + // Create new main with the same config. + config := m.Command.Config + m.Command = server.NewCommand(bytes.NewReader(nil), ioutil.Discard, ioutil.Discard, m.commandOptions...) + m.Command.Config = config + + // Run new program. + return m.Start() +} + // MustCreateIndex uses this command's API to create an index and fails the test // if there is an error. func (m *Command) MustCreateIndex(tb testing.TB, name string, opts pilosa.IndexOptions) *pilosa.Index { diff --git a/translator_test.go b/translator_test.go index 34eaca0b1..d8957e8bc 100644 --- a/translator_test.go +++ b/translator_test.go @@ -15,6 +15,7 @@ package pilosa_test import ( + "bytes" "context" "errors" "io" @@ -22,7 +23,11 @@ import ( "github.com/google/go-cmp/cmp" "github.com/pilosa/pilosa/v2" + "github.com/pilosa/pilosa/v2/boltdb" + "github.com/pilosa/pilosa/v2/http" "github.com/pilosa/pilosa/v2/mock" + "github.com/pilosa/pilosa/v2/server" + "github.com/pilosa/pilosa/v2/test" ) func TestInMemTranslateStore_TranslateKey(t *testing.T) { @@ -177,3 +182,114 @@ func TestMultiTranslateEntryReader(t *testing.T) { } }) } + +// Test key translation with multiple nodes. +func TestTranslation_Reset(t *testing.T) { + // We need to ensure that the translate key partitions for each + // node are getting set as read-only based on the full cluster, + // not just the state of the cluster at the time of the individual + // node restart. + t.Run("RollingRestart", func(t *testing.T) { + // Start a 4-node cluster. + // Note that the prefix on the nodeID is intentional; it puts the + // nodes in a specific order which exercises the condition for + // which we are testing. In a normal use case, these would be + // randomly generated uuids, so this is mimicking that. + c := test.MustRunCluster(t, 4, + []server.CommandOption{ + server.OptCommandServerOptions( + pilosa.OptServerIsCoordinator(true), + pilosa.OptServerNodeID("2node0"), + pilosa.OptServerOpenTranslateStore(boltdb.OpenTranslateStore), + pilosa.OptServerOpenTranslateReader(http.GetOpenTranslateReaderFunc(nil)), + )}, + []server.CommandOption{ + server.OptCommandServerOptions( + pilosa.OptServerIsCoordinator(false), + pilosa.OptServerNodeID("4node1"), + pilosa.OptServerOpenTranslateStore(boltdb.OpenTranslateStore), + pilosa.OptServerOpenTranslateReader(http.GetOpenTranslateReaderFunc(nil)), + )}, + []server.CommandOption{ + server.OptCommandServerOptions( + pilosa.OptServerIsCoordinator(false), + pilosa.OptServerNodeID("3node2"), + pilosa.OptServerOpenTranslateStore(boltdb.OpenTranslateStore), + pilosa.OptServerOpenTranslateReader(http.GetOpenTranslateReaderFunc(nil)), + )}, + []server.CommandOption{ + server.OptCommandServerOptions( + pilosa.OptServerIsCoordinator(false), + pilosa.OptServerNodeID("1node3"), + pilosa.OptServerOpenTranslateStore(boltdb.OpenTranslateStore), + pilosa.OptServerOpenTranslateReader(http.GetOpenTranslateReaderFunc(nil)), + )}, + ) + + node0 := c[0] + node1 := c[1] + node2 := c[2] + node3 := c[3] + + ctx := context.Background() + idx := "i" + + // Create an index with keys. + if _, err := node0.API.CreateIndex(ctx, idx, + pilosa.IndexOptions{ + Keys: true, + }); err != nil { + t.Fatal(err) + } + + // Stop the cluster. + if err := node0.Command.Close(); err != nil { + t.Fatal(err) + } + if err := node1.Command.Close(); err != nil { + t.Fatal(err) + } + if err := node2.Command.Close(); err != nil { + t.Fatal(err) + } + if err := node3.Command.Close(); err != nil { + t.Fatal(err) + } + + // Restart the nodes serially. + if err := node0.SoftOpen(); err != nil { + t.Fatal(err) + } + gossipSeeds := []string{node0.GossipAddress()} + + node1.Config.Gossip.Seeds = gossipSeeds + if err := node1.SoftOpen(); err != nil { + t.Fatal(err) + } + node2.Config.Gossip.Seeds = gossipSeeds + if err := node2.SoftOpen(); err != nil { + t.Fatal(err) + } + node3.Config.Gossip.Seeds = gossipSeeds + if err := node3.SoftOpen(); err != nil { + t.Fatal(err) + } + + // Send a key translation request that triggers + // a read-only translate store error if the + // translate store sync is not reset correctly. + + // Generate request body for translate row keys request + reqBody, err := node0.API.Serializer.Marshal(&pilosa.TranslateKeysRequest{ + Index: idx, + Keys: []string{"a1"}, + }) + if err != nil { + t.Fatal(err) + } + + if _, err := node0.API.TranslateKeys(ctx, bytes.NewReader(reqBody)); err != nil { + t.Fatal(err) + } + }) +}