From b7749af886fe5c801bc78e095e82192535b7df9d Mon Sep 17 00:00:00 2001 From: Evan Cordell Date: Mon, 6 Dec 2021 10:11:12 -0500 Subject: [PATCH] Revert "Add follower read delay option" f163c91087f86464f5a87ccfc89d8c37933e7749 --- cmd/spicedb/serve.go | 2 -- internal/datastore/crdb/crdb.go | 7 +--- internal/datastore/crdb/crdb_test.go | 50 +++------------------------- internal/datastore/crdb/options.go | 12 ------- 4 files changed, 5 insertions(+), 66 deletions(-) diff --git a/cmd/spicedb/serve.go b/cmd/spicedb/serve.go index 81ed971797..48f931b9e0 100644 --- a/cmd/spicedb/serve.go +++ b/cmd/spicedb/serve.go @@ -86,7 +86,6 @@ func registerServeCmd(rootCmd *cobra.Command) { serveCmd.Flags().Duration("datastore-gc-interval", 3*time.Minute, "amount of time between passes of garbage collection (postgres driver only)") serveCmd.Flags().Duration("datastore-gc-max-operation-time", 1*time.Minute, "maximum amount of time a garbage collection pass can operate before timing out (postgres driver only)") serveCmd.Flags().Duration("datastore-revision-fuzzing-duration", 5*time.Second, "amount of time to advertize stale revisions") - serveCmd.Flags().Duration("datastore-follower-read-delay-duration", 0*time.Second, "amount of time to use as a delay to enable follower reads (cockroach driver only)") serveCmd.Flags().String("datastore-query-split-size", common.DefaultSplitAtEstimatedQuerySize.String(), "estimated number of bytes at which a query is split when using a remote datastore") serveCmd.Flags().StringSlice("datastore-bootstrap-files", []string{}, "bootstrap data yaml files to load") serveCmd.Flags().Bool("datastore-bootstrap-overwrite", false, "overwrite any existing data with bootstrap data") @@ -165,7 +164,6 @@ func serveRun(cmd *cobra.Command, args []string) { crdb.MaxOpenConns(cobrautil.MustGetInt(cmd, "datastore-conn-max-open")), crdb.MinOpenConns(cobrautil.MustGetInt(cmd, "datastore-conn-min-open")), crdb.RevisionQuantization(revisionFuzzingTimedelta), - crdb.FollowerReadDelay(cobrautil.MustGetDuration(cmd, "datastore-follower-read-delay-duration")), crdb.GCWindow(gcWindow), crdb.MaxRetries(maxRetries), crdb.SplitAtEstimatedQuerySize(splitQuerySize), diff --git a/internal/datastore/crdb/crdb.go b/internal/datastore/crdb/crdb.go index 0c13f3df16..1a3dd37cf8 100644 --- a/internal/datastore/crdb/crdb.go +++ b/internal/datastore/crdb/crdb.go @@ -124,8 +124,6 @@ func NewCRDBDatastore(url string, options ...Option) (datastore.Datastore, error maxRevisionStaleness := time.Duration(float64(config.revisionQuantization.Nanoseconds())* config.maxRevisionStalenessPercent) * time.Nanosecond - followerReadDelayNanos := config.followerReadDelay.Nanoseconds() - return &crdbDatastore{ dburl: url, conn: conn, @@ -133,7 +131,6 @@ func NewCRDBDatastore(url string, options ...Option) (datastore.Datastore, error quantizationNanos: config.revisionQuantization.Nanoseconds(), maxRevisionStaleness: maxRevisionStaleness, gcWindowNanos: gcWindowNanos, - followerReadDelayNanos: followerReadDelayNanos, splitAtEstimatedQuerySize: config.splitAtEstimatedQuerySize, execute: executeWithMaxRetries(config.maxRetries), overlapKeyer: keyer, @@ -147,7 +144,6 @@ type crdbDatastore struct { quantizationNanos int64 maxRevisionStaleness time.Duration gcWindowNanos int64 - followerReadDelayNanos int64 splitAtEstimatedQuerySize units.Base2Bytes execute executeTxRetryFunc overlapKeyer overlapKeyer @@ -199,8 +195,7 @@ func (cds *crdbDatastore) Revision(ctx context.Context) (datastore.Revision, err } // Round the revision down to the nearest quantization - // Apply a delay to enable historial reads - crdbNow := nowHLC.IntPart() - cds.followerReadDelayNanos + crdbNow := nowHLC.IntPart() quantized := crdbNow if cds.quantizationNanos > 0 { quantized -= (crdbNow % cds.quantizationNanos) diff --git a/internal/datastore/crdb/crdb_test.go b/internal/datastore/crdb/crdb_test.go index da61be1089..c6d5d20f30 100644 --- a/internal/datastore/crdb/crdb_test.go +++ b/internal/datastore/crdb/crdb_test.go @@ -12,7 +12,6 @@ import ( "github.com/jackc/pgx/v4" "github.com/ory/dockertest/v3" - "github.com/stretchr/testify/require" "github.com/authzed/spicedb/internal/datastore" "github.com/authzed/spicedb/internal/datastore/crdb/migrations" @@ -22,11 +21,9 @@ import ( ) type sqlTest struct { - conn *pgx.Conn - port string - creds string - followerReadDelay time.Duration - + conn *pgx.Conn + port string + creds string cleanup func() } @@ -71,7 +68,6 @@ func (st sqlTest) New(revisionFuzzingTimedelta, gcWindow time.Duration, watchBuf GCWindow(gcWindow), RevisionQuantization(revisionFuzzingTimedelta), WatchBufferLength(watchBufferLength), - FollowerReadDelay(st.followerReadDelay), ) } @@ -82,44 +78,6 @@ func TestCRDBDatastore(t *testing.T) { test.All(t, tester) } -func TestCRDBDatastoreWithFollowerReads(t *testing.T) { - followerReadDelay := time.Duration(4.8 * float64(time.Second)) - gcWindow := 100 * time.Second - - quantizationDurations := []time.Duration{ - 0 * time.Second, - 100 * time.Millisecond, - } - for _, quantization := range quantizationDurations { - t.Run(fmt.Sprintf("followReadDelayWithQuantization%s", quantization), func(t *testing.T) { - require := require.New(t) - - tester := newTester(crdbContainer, "root:fake", 26257) - tester.followerReadDelay = followerReadDelay - - ds, err := tester.New(quantization, gcWindow, 1) - require.NoError(err) - - ctx := context.Background() - ok, err := ds.IsReady(ctx) - require.NoError(err) - require.True(ok) - - // Revisions should be at least the follower read delay amount in the past - for start := time.Now(); time.Since(start) < 50*time.Millisecond; { - testRevision, err := ds.Revision(ctx) - nowRevision, err := ds.SyncRevision(ctx) - require.NoError(err) - - diff := nowRevision.IntPart() - testRevision.IntPart() - require.True(diff > followerReadDelay.Nanoseconds()) - } - tester.cleanup() - ds.Close() - }) - } -} - func newTester(containerOpts *dockertest.RunOptions, creds string, portNum uint16) *sqlTest { pool, err := dockertest.NewPool("") if err != nil { @@ -151,5 +109,5 @@ func newTester(containerOpts *dockertest.RunOptions, creds string, portNum uint1 } } - return &sqlTest{conn: conn, port: port, cleanup: cleanup, creds: creds, followerReadDelay: 0 * time.Second} + return &sqlTest{conn: conn, port: port, cleanup: cleanup, creds: creds} } diff --git a/internal/datastore/crdb/options.go b/internal/datastore/crdb/options.go index b4c823c679..e50042b4ac 100644 --- a/internal/datastore/crdb/options.go +++ b/internal/datastore/crdb/options.go @@ -17,7 +17,6 @@ type crdbOptions struct { watchBufferLength uint16 revisionQuantization time.Duration - followerReadDelay time.Duration maxRevisionStalenessPercent float64 gcWindow time.Duration maxRetries int @@ -34,7 +33,6 @@ const ( overlapStrategyInsecure = "insecure" defaultRevisionQuantization = 5 * time.Second - defaultFollowerReadDelay = 0 * time.Second defaultMaxRevisionStalenessPercent = 0.1 defaultWatchBufferLength = 128 @@ -52,7 +50,6 @@ func generateConfig(options []Option) (crdbOptions, error) { gcWindow: 24 * time.Hour, watchBufferLength: defaultWatchBufferLength, revisionQuantization: defaultRevisionQuantization, - followerReadDelay: defaultFollowerReadDelay, maxRevisionStalenessPercent: defaultMaxRevisionStalenessPercent, splitAtEstimatedQuerySize: common.DefaultSplitAtEstimatedQuerySize, maxRetries: defaultMaxRetries, @@ -146,15 +143,6 @@ func RevisionQuantization(bucketSize time.Duration) Option { } } -// FollowerReadDelay is the time delay to apply to enable historial reads. -// -// This value defaults to 0 seconds. -func FollowerReadDelay(delay time.Duration) Option { - return func(po *crdbOptions) { - po.followerReadDelay = delay - } -} - // MaxRevisionStalenessPercent is the amount of time, expressed as a percentage of // the revision quantization window, that a previously computed rounded revision // can still be advertised after the next rounded revision would otherwise be ready.