From 67535bbda1ef63c1e4fe409e8c4d9f3cfaa48fa4 Mon Sep 17 00:00:00 2001 From: Aaron Son Date: Thu, 30 Nov 2023 12:00:55 -0800 Subject: [PATCH] go/cmd/dolt: sql-server.lock/sql-server.info changes. PR feedback. --- go/cmd/dolt/commands/filter-branch.go | 2 -- go/cmd/dolt/commands/gc.go | 7 +++++- go/cmd/dolt/commands/indexcmds/rebuild.go | 2 -- go/cmd/dolt/commands/schcmds/import.go | 2 -- go/cmd/dolt/commands/sqlserver/creds.go | 12 ++------- go/cmd/dolt/commands/stashcmds/clear.go | 2 -- go/cmd/dolt/commands/stashcmds/drop.go | 2 -- go/cmd/dolt/commands/stashcmds/list.go | 2 -- go/cmd/dolt/commands/stashcmds/pop.go | 2 -- go/cmd/dolt/commands/stashcmds/stash.go | 2 -- go/cmd/dolt/dolt.go | 25 +++++++++++-------- go/libraries/doltcore/env/environment.go | 1 + go/store/chunks/chunk_store.go | 7 +++++- ...er_lock_test.go => sqlserver_info_test.go} | 18 ++++++------- 14 files changed, 38 insertions(+), 48 deletions(-) rename integration-tests/go-sql-server-driver/{sqlserver_lock_test.go => sqlserver_info_test.go} (95%) diff --git a/go/cmd/dolt/commands/filter-branch.go b/go/cmd/dolt/commands/filter-branch.go index 6b722ebc46..7f05c2b5e5 100644 --- a/go/cmd/dolt/commands/filter-branch.go +++ b/go/cmd/dolt/commands/filter-branch.go @@ -104,8 +104,6 @@ func (cmd FilterBranchCmd) Exec(ctx context.Context, commandStr string, args []s return HandleVErrAndExitCode(verr, usage) } - // TODO: Assert that dEnv.DoltDB.AccessMode() != ReadOnly - queryString := apr.GetValueOrDefault(QueryFlag, "") verbose := apr.Contains(cli.VerboseFlag) continueOnErr := apr.Contains(continueFlag) diff --git a/go/cmd/dolt/commands/gc.go b/go/cmd/dolt/commands/gc.go index 980e5be4de..982def3114 100644 --- a/go/cmd/dolt/commands/gc.go +++ b/go/cmd/dolt/commands/gc.go @@ -88,7 +88,12 @@ func (cmd GarbageCollectionCmd) Exec(ctx context.Context, commandStr string, arg help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, gcDocs, ap)) apr := cli.ParseArgsOrDie(ap, args, help) - // TODO: Assert that dEnv.DoltDB.AccessMode() != ReadOnly? + // We assert this here simply because the implementation of GC can + // delay actually trying to write to the chunk store for a long time + // after doing a lot or work. It's better to fail early. + if dEnv.IsAccessModeReadOnly() { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(env.ErrDatabaseIsLocked), help) + } var err error if apr.Contains(cli.ShallowFlag) { diff --git a/go/cmd/dolt/commands/indexcmds/rebuild.go b/go/cmd/dolt/commands/indexcmds/rebuild.go index 6ff73b63c9..26b4403b58 100644 --- a/go/cmd/dolt/commands/indexcmds/rebuild.go +++ b/go/cmd/dolt/commands/indexcmds/rebuild.go @@ -71,8 +71,6 @@ func (cmd RebuildCmd) Exec(ctx context.Context, commandStr string, args []string return HandleErr(errhand.BuildDError("Both the table and index names must be provided.").Build(), usage) } - // TODO: Error if dEnv.DoltDB.AccessMode() == ReadOnly? - working, err := dEnv.WorkingRoot(context.Background()) if err != nil { return HandleErr(errhand.BuildDError("Unable to get working.").AddCause(err).Build(), nil) diff --git a/go/cmd/dolt/commands/schcmds/import.go b/go/cmd/dolt/commands/schcmds/import.go index a39f49deec..5c30a1cb75 100644 --- a/go/cmd/dolt/commands/schcmds/import.go +++ b/go/cmd/dolt/commands/schcmds/import.go @@ -170,8 +170,6 @@ func (cmd ImportCmd) Exec(ctx context.Context, commandStr string, args []string, return 1 } - // TODO: Error if dEnv.DoltDB.AccessMode() == ReadOnly? - return commands.HandleVErrAndExitCode(importSchema(ctx, dEnv, apr), usage) } diff --git a/go/cmd/dolt/commands/sqlserver/creds.go b/go/cmd/dolt/commands/sqlserver/creds.go index 53df7b41dd..17fe046ac0 100644 --- a/go/cmd/dolt/commands/sqlserver/creds.go +++ b/go/cmd/dolt/commands/sqlserver/creds.go @@ -29,7 +29,7 @@ import ( "github.com/dolthub/dolt/go/libraries/utils/filesys" ) -const ServerLocalCredsFile = "sql-server.lock" +const ServerLocalCredsFile = "sql-server.info" // LocalCreds is a struct that contains information about how to access the // locally running server for a CLI process which wants to operate against a @@ -87,7 +87,7 @@ func WriteLocalCreds(fs filesys.Filesys, creds *LocalCreds) error { // Starting at `fs`, look for the a ServerLocalCredsFile in the .dolt directory // of this directory and every parent directory, until we find one. When we -// find we, we return its contents if we can open and parse it successfully. +// find one, we return its contents if we can open and parse it successfully. // Otherwise, we return an error associated with attempting to read it. If we // do not find anything all the way up to the root of the filesystem, returns // `nil` *LocalCreds and a `nil` error. @@ -136,14 +136,6 @@ func LoadLocalCreds(fs filesys.Filesys) (creds *LocalCreds, err error) { data := strings.TrimSpace(string(b[:n])) parts := strings.Split(data, ":") - if len(parts) == 1 { - // Legacy Lock file. We can remove this code path in a couple of months (6/2023) - pid, err := strconv.Atoi(parts[0]) - if err != nil { - return nil, err - } - return &LocalCreds{Pid: pid, Port: -1, Secret: ""}, nil - } if len(parts) != 3 { return nil, fmt.Errorf("invalid lock file format") } diff --git a/go/cmd/dolt/commands/stashcmds/clear.go b/go/cmd/dolt/commands/stashcmds/clear.go index 456319f90f..4a2b7618ba 100644 --- a/go/cmd/dolt/commands/stashcmds/clear.go +++ b/go/cmd/dolt/commands/stashcmds/clear.go @@ -72,8 +72,6 @@ func (cmd StashClearCmd) Exec(ctx context.Context, commandStr string, args []str help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, stashClearDocs, ap)) apr := cli.ParseArgsOrDie(ap, args, help) - // TODO: Error if dEnv.DoltDB.AccessMode() == ReadOnly? - if apr.NArg() != 0 { usage() return 1 diff --git a/go/cmd/dolt/commands/stashcmds/drop.go b/go/cmd/dolt/commands/stashcmds/drop.go index 680aaa3a17..e6e760b036 100644 --- a/go/cmd/dolt/commands/stashcmds/drop.go +++ b/go/cmd/dolt/commands/stashcmds/drop.go @@ -75,8 +75,6 @@ func (cmd StashDropCmd) Exec(ctx context.Context, commandStr string, args []stri help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, stashDropDocs, ap)) apr := cli.ParseArgsOrDie(ap, args, help) - // TODO: Error if dEnv.DoltDB.AccessMode() == ReadOnly? - var idx = 0 var err error if apr.NArg() == 1 { diff --git a/go/cmd/dolt/commands/stashcmds/list.go b/go/cmd/dolt/commands/stashcmds/list.go index b00d7043ca..21fa04c9e0 100644 --- a/go/cmd/dolt/commands/stashcmds/list.go +++ b/go/cmd/dolt/commands/stashcmds/list.go @@ -72,8 +72,6 @@ func (cmd StashListCmd) Exec(ctx context.Context, commandStr string, args []stri help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, stashListDocs, ap)) cli.ParseArgsOrDie(ap, args, help) - // TODO: Error if dEnv.DoltDB.AccessMode() == ReadOnly? - err := listStashes(ctx, dEnv) if err != nil { return commands.HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) diff --git a/go/cmd/dolt/commands/stashcmds/pop.go b/go/cmd/dolt/commands/stashcmds/pop.go index 8ac10fd5ec..09ff5591d1 100644 --- a/go/cmd/dolt/commands/stashcmds/pop.go +++ b/go/cmd/dolt/commands/stashcmds/pop.go @@ -81,8 +81,6 @@ func (cmd StashPopCmd) Exec(ctx context.Context, commandStr string, args []strin help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, stashPopDocs, ap)) apr := cli.ParseArgsOrDie(ap, args, help) - // TODO: Error if dEnv.DoltDB.AccessMode() == ReadOnly? - _, sqlCtx, closer, err := cliCtx.QueryEngine(ctx) if err != nil { cli.PrintErrln(err.Error()) diff --git a/go/cmd/dolt/commands/stashcmds/stash.go b/go/cmd/dolt/commands/stashcmds/stash.go index ef1aca7087..f4fa54fa63 100644 --- a/go/cmd/dolt/commands/stashcmds/stash.go +++ b/go/cmd/dolt/commands/stashcmds/stash.go @@ -99,8 +99,6 @@ func (cmd StashCmd) Exec(ctx context.Context, commandStr string, args []string, return 1 } - // TODO: Error if dEnv.DoltDB.AccessMode() == ReadOnly? - err := stashChanges(ctx, dEnv, apr) if err != nil { return commands.HandleStageError(err) diff --git a/go/cmd/dolt/dolt.go b/go/cmd/dolt/dolt.go index 6465891cfb..19cd3405e0 100644 --- a/go/cmd/dolt/dolt.go +++ b/go/cmd/dolt/dolt.go @@ -676,17 +676,20 @@ If you're interested in running this command against a remote host, hit us up on } } - usedEnv := targetEnv - if usedEnv == nil && useDb != "" { - usedEnv = mrEnv.GetEnv(useDb) - } + // If our targetEnv is still |nil| and we don't have an environment + // which we will be using based on |useDb|, then our initialization + // here did not find a repository we will be operating against. + noValidRepository := targetEnv == nil && (useDb == "" || mrEnv.GetEnv(useDb) == nil) - // There is no target environment detected. This is allowed for a small number of commands. - // We don't expect that number to grow, so we list them here. - // It's also allowed when --help is passed. - // So we defer the error until the caller tries to use the cli.LateBindQueryist - isDoltEnvironmentRequired := subcommandName != "init" && subcommandName != "sql" && subcommandName != "sql-server" && subcommandName != "sql-client" - if usedEnv == nil && isDoltEnvironmentRequired { + // Not having a valid repository as we start to execute a CLI command + // implementation is allowed for a small number of commands. We don't + // expect this set of commands to grow, so we list them here. + // + // This is also allowed when --help is passed. So we defer the error + // until the caller tries to use the cli.LateBindQueryist. + isValidRepositoryRequired := subcommandName != "init" && subcommandName != "sql" && subcommandName != "sql-server" && subcommandName != "sql-client" + + if noValidRepository && isValidRepositoryRequired { return func(ctx context.Context) (cli.Queryist, *sql.Context, func(), error) { return nil, nil, nil, fmt.Errorf("The current directory is not a valid dolt repository.") }, nil @@ -713,7 +716,7 @@ If you're interested in running this command against a remote host, hit us up on if dEnv.DoltDB != nil { allReposAreReadOnly = allReposAreReadOnly && dEnv.IsAccessModeReadOnly() } - return false, nil + return !allReposAreReadOnly, nil }) lookForServer = allReposAreReadOnly } diff --git a/go/libraries/doltcore/env/environment.go b/go/libraries/doltcore/env/environment.go index 82ff40ccb2..77537b2b87 100644 --- a/go/libraries/doltcore/env/environment.go +++ b/go/libraries/doltcore/env/environment.go @@ -75,6 +75,7 @@ var ErrFailedToWriteRepoState = errors.New("failed to write repo state") var ErrRemoteAddressConflict = errors.New("address conflict with a remote") var ErrDoltRepositoryNotFound = errors.New("can no longer find .dolt dir on disk") var ErrFailedToAccessDB = goerrors.NewKind("failed to access '%s' database: can no longer find .dolt dir on disk") +var ErrDatabaseIsLocked = errors.New("the database is locked by another dolt process") // DoltEnv holds the state of the current environment used by the cli. type DoltEnv struct { diff --git a/go/store/chunks/chunk_store.go b/go/store/chunks/chunk_store.go index 751f2556d9..bed89c8e68 100644 --- a/go/store/chunks/chunk_store.go +++ b/go/store/chunks/chunk_store.go @@ -32,7 +32,7 @@ import ( // A ChunkStore's |ExclusiveAccessMode| can indicate to a client what type of // exclusive access and concurrency control are support by the ChunkStore, and // for a ChunkStore instance which the client is holding, what level of access -// it has. Typically, a ChunkStore will by Mode_Shared, which means multiple +// it has. Typically, a ChunkStore will be in Mode_Shared, which means multiple // processes can concurrently write to the chunk store and the chunk store can // change out from under the process. Other possible modes, currently only used // by local ChunkStores using the chunk journal in go/store/nbs are @@ -42,6 +42,11 @@ import ( // while this ChunkStore is open. type ExclusiveAccessMode uint8 +// Note: the order of these constants is relied on in nbs/GenerationalNBS. The +// order should go from least restricted access to most restricted access. If a +// client has many stores with different levels of accses, a common question +// they want to answer is: what is the most restricted access across all of +// these stores. const ( ExclusiveAccessMode_Shared = iota ExclusiveAccessMode_Exclusive diff --git a/integration-tests/go-sql-server-driver/sqlserver_lock_test.go b/integration-tests/go-sql-server-driver/sqlserver_info_test.go similarity index 95% rename from integration-tests/go-sql-server-driver/sqlserver_lock_test.go rename to integration-tests/go-sql-server-driver/sqlserver_info_test.go index 9b819bc195..5461ecc03b 100644 --- a/integration-tests/go-sql-server-driver/sqlserver_lock_test.go +++ b/integration-tests/go-sql-server-driver/sqlserver_info_test.go @@ -28,7 +28,7 @@ import ( driver "github.com/dolthub/dolt/go/libraries/doltcore/dtestutils/sql_server_driver" ) -func TestSQLServerLockFile(t *testing.T) { +func TestSQLServerInfoFile(t *testing.T) { t.Run("With Two Repos", func(t *testing.T) { u, err := driver.NewDoltUser() require.NoError(t, err) @@ -46,8 +46,8 @@ func TestSQLServerLockFile(t *testing.T) { t.Run("With server running in root", func(t *testing.T) { RunServerUntilEndOfTest(t, rs, &driver.Server{}) - t.Run("sql-server.lock file exists", func(t *testing.T) { - location := filepath.Join(rs.Dir, ".dolt/sql-server.lock") + t.Run("sql-server.info file exists", func(t *testing.T) { + location := filepath.Join(rs.Dir, ".dolt/sql-server.info") f, err := os.Open(location) require.NoError(t, err) require.NoError(t, f.Close()) @@ -98,8 +98,8 @@ func TestSQLServerLockFile(t *testing.T) { require.Regexp(t, "db_two", outstr) }) }) - t.Run("sql-server.lock in root no longer exists", func(t *testing.T) { - location := filepath.Join(rs.Dir, ".dolt/sql-server.lock") + t.Run("sql-server.info in root no longer exists", func(t *testing.T) { + location := filepath.Join(rs.Dir, ".dolt/sql-server.info") f, err := os.Open(location) if f != nil { defer f.Close() @@ -211,9 +211,9 @@ func TestSQLServerLockFile(t *testing.T) { require.Error(t, err) }) }) - t.Run("With stale sql-server.lock file in root", func(t *testing.T) { + t.Run("With stale sql-server.info file in root", func(t *testing.T) { Setup := func(t *testing.T) { - path := filepath.Join(rs.Dir, ".dolt/sql-server.lock") + path := filepath.Join(rs.Dir, ".dolt/sql-server.info") err := os.WriteFile(path, []byte("1:3306:this_is_not_a_real_secret"), 0600) require.NoError(t, err) t.Cleanup(func() { os.Remove(path) }) @@ -244,9 +244,9 @@ func TestSQLServerLockFile(t *testing.T) { require.Regexp(t, "db_one", outstr) }) }) - t.Run("With malformed sql-server.lock file in root", func(t *testing.T) { + t.Run("With malformed sql-server.info file in root", func(t *testing.T) { Setup := func(t *testing.T) { - path := filepath.Join(rs.Dir, ".dolt/sql-server.lock") + path := filepath.Join(rs.Dir, ".dolt/sql-server.info") err := os.WriteFile(path, []byte("1:3306:this_is_not_a_real_secret:extra:fields:make:it:fail"), 0600) require.NoError(t, err) t.Cleanup(func() { os.Remove(path) })