Merge pull request #6033 from dolthub/zachmu/replication-errors

Changed replication configuration errors to log warnings, rather than stop the server from starting

go/cmd/dolt/commands/engine/utils.go

@@ -16,18 +16,12 @@ package engine
 
 import (
 	"context"
-	"fmt"
 
-	"github.com/dolthub/go-mysql-server/sql"
-
-	"github.com/dolthub/dolt/go/cmd/dolt/cli"
-	"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
 	"github.com/dolthub/dolt/go/libraries/doltcore/env"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
 	"github.com/dolthub/dolt/go/libraries/doltcore/table/editor"
 	"github.com/dolthub/dolt/go/libraries/utils/filesys"
-	"github.com/dolthub/dolt/go/store/types"
 )
 
 // CollectDBs takes a MultiRepoEnv and creates Database objects from each environment and returns a slice of these
@@ -38,28 +32,11 @@ func CollectDBs(ctx context.Context, mrEnv *env.MultiRepoEnv, useBulkEditor bool
 	var db dsess.SqlDatabase
 
 	err := mrEnv.Iter(func(name string, dEnv *env.DoltEnv) (stop bool, err error) {
-		postCommitHooks, err := GetCommitHooks(ctx, dEnv)
-		if err != nil {
-			return true, err
-		}
-		dEnv.DoltDB.SetCommitHooks(ctx, postCommitHooks)
-
 		db, err = newDatabase(ctx, name, dEnv, useBulkEditor)
 		if err != nil {
 			return false, err
 		}
 
-		if _, remote, ok := sql.SystemVariables.GetGlobal(dsess.ReadReplicaRemote); ok && remote != "" {
-			remoteName, ok := remote.(string)
-			if !ok {
-				return true, sql.ErrInvalidSystemVariableValue.New(remote)
-			}
-			db, err = newReplicaDatabase(ctx, name, remoteName, dEnv)
-			if err != nil {
-				return true, err
-			}
-		}
-
 		dbs = append(dbs, db)
 		locations = append(locations, dEnv.FS)
 
@@ -73,27 +50,6 @@ func CollectDBs(ctx context.Context, mrEnv *env.MultiRepoEnv, useBulkEditor bool
 	return dbs, locations, nil
 }
 
-// GetCommitHooks creates a list of hooks to execute on database commit. If doltdb.SkipReplicationErrorsKey is set,
-// replace misconfigured hooks with doltdb.LogHook instances that prints a warning when trying to execute.
-// TODO: this duplicates code in the sqle package
-func GetCommitHooks(ctx context.Context, dEnv *env.DoltEnv) ([]doltdb.CommitHook, error) {
-	postCommitHooks := make([]doltdb.CommitHook, 0)
-
-	if hook, err := getPushOnWriteHook(ctx, dEnv); err != nil {
-		path, _ := dEnv.FS.Abs(".")
-		err = fmt.Errorf("failure loading hook for database at %s; %w", path, err)
-		if dsess.IgnoreReplicationErrors() {
-			postCommitHooks = append(postCommitHooks, doltdb.NewLogHook([]byte(err.Error()+"\n")))
-		} else {
-			return nil, err
-		}
-	} else if hook != nil {
-		postCommitHooks = append(postCommitHooks, hook)
-	}
-
-	return postCommitHooks, nil
-}
-
 func newDatabase(ctx context.Context, name string, dEnv *env.DoltEnv, useBulkEditor bool) (sqle.Database, error) {
 	deaf := dEnv.DbEaFactory()
 	if useBulkEditor {
@@ -109,68 +65,3 @@ func newDatabase(ctx context.Context, name string, dEnv *env.DoltEnv, useBulkEdi
 	}
 	return sqle.NewDatabase(ctx, name, dEnv.DbData(), opts)
 }
-
-// newReplicaDatabase creates a new dsqle.ReadReplicaDatabase. If the doltdb.SkipReplicationErrorsKey global variable is set,
-// skip errors related to database construction only and return a partially functional dsqle.ReadReplicaDatabase
-// that will log warnings when attempting to perform replica commands.
-func newReplicaDatabase(ctx context.Context, name string, remoteName string, dEnv *env.DoltEnv) (sqle.ReadReplicaDatabase, error) {
-	tmpDir, err := dEnv.TempTableFilesDir()
-	if err != nil {
-		return sqle.ReadReplicaDatabase{}, err
-	}
-	opts := editor.Options{
-		Deaf:    dEnv.DbEaFactory(),
-		Tempdir: tmpDir,
-	}
-
-	db, err := sqle.NewDatabase(ctx, name, dEnv.DbData(), opts)
-	if err != nil {
-		return sqle.ReadReplicaDatabase{}, err
-	}
-
-	rrd, err := sqle.NewReadReplicaDatabase(ctx, db, remoteName, dEnv)
-	if err != nil {
-		err = fmt.Errorf("%w from remote '%s'; %s", sqle.ErrFailedToLoadReplicaDB, remoteName, err.Error())
-		if !dsess.IgnoreReplicationErrors() {
-			return sqle.ReadReplicaDatabase{}, err
-		}
-		cli.Println(err)
-		return sqle.ReadReplicaDatabase{Database: db}, nil
-	}
-	return rrd, nil
-}
-
-func getPushOnWriteHook(ctx context.Context, dEnv *env.DoltEnv) (*doltdb.PushOnWriteHook, error) {
-	_, val, ok := sql.SystemVariables.GetGlobal(dsess.ReplicateToRemote)
-	if !ok {
-		return nil, sql.ErrUnknownSystemVariable.New(dsess.ReplicateToRemote)
-	} else if val == "" {
-		return nil, nil
-	}
-
-	remoteName, ok := val.(string)
-	if !ok {
-		return nil, sql.ErrInvalidSystemVariableValue.New(val)
-	}
-
-	remotes, err := dEnv.GetRemotes()
-	if err != nil {
-		return nil, err
-	}
-
-	rem, ok := remotes[remoteName]
-	if !ok {
-		return nil, fmt.Errorf("%w: '%s'", env.ErrRemoteNotFound, remoteName)
-	}
-
-	ddb, err := rem.GetRemoteDB(ctx, types.Format_Default, dEnv)
-	if err != nil {
-		return nil, err
-	}
-	tmpDir, err := dEnv.TempTableFilesDir()
-	if err != nil {
-		return nil, err
-	}
-	pushHook := doltdb.NewPushOnWriteHook(ddb, tmpDir)
-	return pushHook, nil
-}

go/cmd/dolt/commands/sql_test.go

@@ -17,6 +17,7 @@ package commands
 import (
 	"context"
 	"fmt"
+	"io"
 	"testing"
 
 	"github.com/dolthub/go-mysql-server/sql"
@@ -25,22 +26,12 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/dolthub/dolt/go/cmd/dolt/commands/engine"
 	"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
 	"github.com/dolthub/dolt/go/libraries/doltcore/dtestutils"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle"
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
 )
 
-//var UUIDS = []uuid.UUID{
-//	uuid.Must(uuid.Parse("00000000-0000-0000-0000-000000000000")),
-//	uuid.Must(uuid.Parse("00000000-0000-0000-0000-000000000001")),
-//	uuid.Must(uuid.Parse("00000000-0000-0000-0000-000000000002"))}
-//var Names = []string{"Bill Billerson", "John Johnson", "Rob Robertson"}
-//var Ages = []uint64{32, 25, 21}
-//var Titles = []string{"Senior Dufus", "Dufus", ""}
-//var MaritalStatus = []bool{true, false, false}
-
 var tableName = "people"
 
 // Smoke test: Console opens and exits
@@ -622,9 +613,8 @@ func TestCommitHooksNoErrors(t *testing.T) {
 	defer dEnv.DoltDB.Close()
 
 	sqle.AddDoltSystemVariables()
-	sql.SystemVariables.SetGlobal(dsess.SkipReplicationErrors, true)
 	sql.SystemVariables.SetGlobal(dsess.ReplicateToRemote, "unknown")
-	hooks, err := engine.GetCommitHooks(context.Background(), dEnv)
+	hooks, err := sqle.GetCommitHooks(context.Background(), nil, dEnv, io.Discard)
 	assert.NoError(t, err)
 	if len(hooks) < 1 {
 		t.Error("failed to produce noop hook")

go/libraries/doltcore/sqle/read_replica_database.go

@@ -143,7 +143,8 @@ func (rrd ReadReplicaDatabase) PullFromRemote(ctx *sql.Context) error {
 
 	switch {
 	case headsArg != "" && allHeads == dsess.SysVarTrue:
-		return fmt.Errorf("%w; cannot set both 'dolt_replicate_heads' and 'dolt_replicate_all_heads'", ErrInvalidReplicateHeadsSetting)
+		ctx.GetLogger().Warnf("cannot set both @@dolt_replicate_heads and @@dolt_replicate_all_heads, replication disabled")
+		return nil
 	case headsArg != "":
 		heads, ok := headsArg.(string)
 		if !ok {
@@ -210,7 +211,8 @@ func (rrd ReadReplicaDatabase) PullFromRemote(ctx *sql.Context) error {
 			return nil
 		}
 	default:
-		return fmt.Errorf("%w: dolt_replicate_heads not set", ErrInvalidReplicateHeadsSetting)
+		ctx.GetLogger().Warnf("must set either @@dolt_replicate_heads or @@dolt_replicate_all_heads, replication disabled")
+		return nil
 	}
 
 	return nil

go/libraries/doltcore/sqle/replication.go

@@ -20,6 +20,7 @@ import (
 	"io"
 
 	"github.com/dolthub/go-mysql-server/sql"
+	"github.com/sirupsen/logrus"
 
 	"github.com/dolthub/dolt/go/cmd/dolt/cli"
 	"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
@@ -68,25 +69,22 @@ func getPushOnWriteHook(ctx context.Context, bThreads *sql.BackgroundThreads, dE
 	return doltdb.NewPushOnWriteHook(ddb, tmpDir), nil
 }
 
-// GetCommitHooks creates a list of hooks to execute on database commit. If doltdb.SkipReplicationErrorsKey is set,
-// replace misconfigured hooks with doltdb.LogHook instances that prints a warning when trying to execute.
+// GetCommitHooks creates a list of hooks to execute on database commit. Hooks that cannot be created because of an
+// error in configuration will not prevent the server from starting, and will instead log errors.
 func GetCommitHooks(ctx context.Context, bThreads *sql.BackgroundThreads, dEnv *env.DoltEnv, logger io.Writer) ([]doltdb.CommitHook, error) {
 	postCommitHooks := make([]doltdb.CommitHook, 0)
 
-	if hook, err := getPushOnWriteHook(ctx, bThreads, dEnv, logger); err != nil {
+	hook, err := getPushOnWriteHook(ctx, bThreads, dEnv, logger)
+	if err != nil {
 		path, _ := dEnv.FS.Abs(".")
-		err = fmt.Errorf("failure loading hook for database at %s; %w", path, err)
-		if dsess.IgnoreReplicationErrors() {
-			postCommitHooks = append(postCommitHooks, doltdb.NewLogHook([]byte(err.Error()+"\n")))
-		} else {
-			return nil, err
-		}
+		logrus.Errorf("error loading replication for database at %s, replication disabled: %v", path, err)
+		postCommitHooks = append(postCommitHooks, doltdb.NewLogHook([]byte(err.Error()+"\n")))
 	} else if hook != nil {
 		postCommitHooks = append(postCommitHooks, hook)
 	}
 
 	for _, h := range postCommitHooks {
-		h.SetLogger(ctx, logger)
+		_ = h.SetLogger(ctx, logger)
 	}
 	return postCommitHooks, nil
 }
@@ -135,9 +133,11 @@ func ApplyReplicationConfig(ctx context.Context, bThreads *sql.BackgroundThreads
 			if !ok {
 				return nil, sql.ErrInvalidSystemVariableValue.New(remote)
 			}
-			db, err = newReplicaDatabase(ctx, db.Name(), remoteName, dEnv)
-			if err != nil {
-				return nil, err
+			rdb, err := newReplicaDatabase(ctx, db.Name(), remoteName, dEnv)
+			if err == nil {
+				db = rdb
+			} else {
+				logrus.Errorf("invalid replication configuration, replication disabled: %v", err)
 			}
 		}
 

integration-tests/bats/helper/query-server-common.bash

@@ -31,8 +31,14 @@ wait_for_connection(port=int(port_str), timeout_ms=int(timeout_ms), database=dat
 
 start_sql_server() {
     DEFAULT_DB="$1"
+    logFile="$2"
     PORT=$( definePORT )
-    dolt sql-server --host 0.0.0.0 --port=$PORT --user dolt --socket "dolt.$PORT.sock" &
+    if [[ $logFile ]]
+    then
+        dolt sql-server --host 0.0.0.0 --port=$PORT --user dolt --socket "dolt.$PORT.sock" > $logFile 2>&1 &
+    else
+        dolt sql-server --host 0.0.0.0 --port=$PORT --user dolt --socket "dolt.$PORT.sock" &
+    fi
     SERVER_PID=$!
     wait_for_connection $PORT 5000
 }

integration-tests/bats/remotes-sql-server.bats

@@ -120,17 +120,21 @@ teardown() {
     [[ "$output" =~ "test" ]] || false
 }
 
-@test "remotes-sql-server: pull remote not found error" {
+@test "remotes-sql-server: pull remote not found" {
     skiponwindows "Missing dependencies"
 
     cd repo1
     dolt config --local --add sqlserver.global.dolt_read_replica_remote unknown
     dolt config --local --add sqlserver.global.dolt_replicate_heads main
 
-    run dolt sql-server -P 3333
-    [ "$status" -eq 1 ]
-    [[ ! "$output" =~ "panic" ]]
-    [[ "$output" =~ "remote not found: 'unknown'" ]] || false
+    start_sql_server repo1 ./server-log.txt
+
+    run dolt sql-client --use-db repo1 -P $PORT -u dolt -q "show tables"
+    [ $status -eq 0 ]
+    [[ "$output" =~ "Table" ]] || false
+
+    run grep 'replication disabled' ./server-log.txt
+    [[ "$output" =~ "replication disabled" ]] || false
 }
 
 @test "remotes-sql-server: quiet pull warnings" {
@@ -140,23 +144,30 @@ teardown() {
     dolt config --local --add sqlserver.global.dolt_skip_replication_errors 1
     dolt config --local --add sqlserver.global.dolt_read_replica_remote unknown
     dolt config --local --add sqlserver.global.dolt_replicate_heads main
-    start_sql_server repo1
+
+    start_sql_server repo1 ./server-log.txt
 
     run dolt sql-client --use-db repo1 -P $PORT -u dolt -q "show tables"
     [ $status -eq 0 ]
     [[ "$output" =~ "Table" ]] || false
+
+    run grep 'failed to load replica database from remote' ./server-log.txt
+    [[ "$output" =~ "failed to load replica database from remote" ]] || false
 }
 
-@test "remotes-sql-server: push remote not found error" {
+@test "remotes-sql-server: push remote not found" {
     skiponwindows "Missing dependencies"
 
     cd repo1
     dolt config --local --add sqlserver.global.dolt_replicate_to_remote unknown
 
-    run dolt sql-server -P 3333
-    [ "$status" -eq 1 ]
-    [[ ! "$output" =~ "panic" ]]
-    [[ "$output" =~ "remote not found: 'unknown'" ]] || false
+    start_sql_server repo1 ./server-log.txt
+
+    run dolt sql-client --use-db repo1 -P $PORT -u dolt -q "show tables"
+    [[ "$output" =~ "Table" ]] || false
+
+    run grep 'replication disabled' ./server-log.txt
+    [[ "$output" =~ "replication disabled" ]] || false
 }
 
 @test "remotes-sql-server: quiet push warnings" {

integration-tests/bats/replication-multidb.bats

@@ -274,15 +274,18 @@ SQL
 @test "replication-multidb: missing database config" {
     dolt config --global --add sqlserver.global.dolt_replicate_to_remote unknown
     run dolt --data-dir=dbs1 sql -b -q "use repo1; create table t1 (a int primary key)"
-    [ "$status" -eq 1 ]
-    [[ ! "$output" =~ "panic" ]] || false
+    [ "$status" -eq 0 ]
     [[ "$output" =~ "remote not found: 'unknown'" ]] || false
+    [[ "$output" =~ "replication disabled" ]] || false
 }
 
 @test "replication-multidb: missing database config quiet warning" {
     dolt config --global --add sqlserver.global.dolt_replicate_to_remote unknown
     dolt config --global --add sqlserver.global.dolt_skip_replication_errors 1
-    dolt --data-dir=dbs1 sql -b -q "use repo1; create table t1 (a int primary key)"
+    run dolt --data-dir=dbs1 sql -b -q "use repo1; create table t1 (a int primary key)"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "remote not found: 'unknown'" ]] || false
+    [[ "$output" =~ "replication disabled" ]] || false
 }
 
 @test "replication-multidb: sql-server push on commit" {

integration-tests/bats/replication.bats

@@ -23,6 +23,31 @@ teardown() {
     cd $BATS_TMPDIR
 }
 
+@test "replication: configuration errors" {
+    cd repo1
+    dolt sql -q "SET @@persist.dolt_read_replica_remote = 'doesNotExist';"
+    
+    run dolt sql -q "show tables"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "replication disabled" ]] || false
+
+    dolt sql -q "SET @@persist.dolt_read_replica_remote = 'remote1';"
+    run dolt sql -q "show tables"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "replication disabled" ]] || false
+
+    dolt sql -q "SET @@persist.dolt_replicate_all_heads = 1";
+    dolt sql -q "SET @@persist.dolt_replicate_heads = 'main';";
+    run dolt sql -q "show tables"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "replication disabled" ]] || false
+
+    dolt sql -q "SET @@persist.dolt_replicate_heads = '';";
+    run dolt sql -q "show tables"
+    [ "$status" -eq 0 ]
+    [[ ! "$output" =~ "replication disabled" ]] || false
+}
+
 @test "replication: default no replication" {
     cd repo1
     dolt sql -q "create table t1 (a int primary key)"
@@ -370,36 +395,6 @@ SQL
     [[ "$output" =~ "branch not found" ]] || false
 }
 
-@test "replication: pull with no head configuration fails" {
-    dolt clone file://./rem1 repo2
-    cd repo2
-    dolt branch new_feature
-    dolt push origin new_feature
-
-    cd ../repo1
-    dolt config --local --add sqlserver.global.dolt_read_replica_remote remote1
-    run dolt sql -q "show tables"
-    [ "$status" -eq 1 ]
-    [[ ! "$output" =~ "panic" ]] || false
-    [[ "$output" =~ "invalid replicate heads setting: dolt_replicate_heads not set" ]] || false
-}
-
-@test "replication: replica pull conflicting head configurations" {
-    dolt clone file://./rem1 repo2
-    cd repo2
-    dolt branch new_feature
-    dolt push origin new_feature
-
-    cd ../repo1
-    dolt config --local --add sqlserver.global.dolt_replicate_heads main,unknown
-    dolt config --local --add sqlserver.global.dolt_replicate_all_heads 1
-    dolt config --local --add sqlserver.global.dolt_read_replica_remote remote1
-    run dolt sql -q "show tables"
-    [ "$status" -eq 1 ]
-    [[ ! "$output" =~ "panic" ]] || false
-    [[ "$output" =~ "invalid replicate heads setting; cannot set both" ]] || false
-}
-
 @test "replication: replica pull multiple heads quiet warnings" {
     dolt clone file://./rem1 repo2
     cd repo2
@@ -560,9 +555,10 @@ SQL
     dolt config --local --add sqlserver.global.dolt_replicate_to_remote unknown
 
     run dolt sql -q "create table t1 (a int primary key)"
-    [ "$status" -eq 1 ]
+    [ "$status" -eq 0 ]
     [[ ! "$output" =~ "panic" ]] || false
     [[ "$output" =~ "remote not found: 'unknown'" ]] || false
+    [[ "$output" =~ "replication disabled" ]] || false
 }
 
 @test "replication: quiet push to unknown remote warnings" {
@@ -571,7 +567,6 @@ SQL
     dolt config --local --add sqlserver.global.dolt_replicate_to_remote unknown
     run dolt sql -q "create table t1 (a int primary key)"
     [ "$status" -eq 0 ]
-    [[ ! "$output" =~ "remote not found" ]] || false
 
     dolt add .
 
@@ -590,17 +585,6 @@ SQL
     [[ ! "$output" =~ "remote not found: 'unknown'" ]] || false
 }
 
-@test "replication: pull bad remote errors" {
-    cd repo1
-    dolt config --local --add sqlserver.global.dolt_read_replica_remote unknown
-    dolt config --local --add sqlserver.global.dolt_replicate_heads main
-
-    run dolt sql -q "show tables"
-    [ "$status" -eq 1 ]
-    [[ ! "$output" =~ "panic" ]]
-    [[ "$output" =~ "remote not found: 'unknown'" ]] || false
-}
-
 @test "replication: non-fast-forward pull fails replication" {
     dolt clone file://./rem1 clone1
     cd clone1