-
-
Notifications
You must be signed in to change notification settings - Fork 535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[no-release-notes] go: sqle: DatabaseProvider: Fix double-creation of push-on-write commit hooks in registerNewDatabase. #8830
Conversation
… push-on-write commit hooks in registerNewDatabase.
|
||
func (ddb *DoltDB) PrependCommitHook(ctx context.Context, hook CommitHook) *DoltDB { | ||
ddb.db = ddb.db.SetCommitHooks(ctx, append([]CommitHook{hook}, ddb.db.PostCommitHooks()...)) | ||
func (ddb *DoltDB) PrependCommitHooks(ctx context.Context, hooks ...CommitHook) *DoltDB { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetCommitHooks was destructive and potentially dangerous. For now, it seems easier to make hook writers coordinate on these things upstream.
return err | ||
} | ||
dbs, err := ApplyReplicationConfig(ctx, sql.NewBackgroundThreads(), mrEnv, cli.CliErr, db) | ||
sdb, err := applyReadReplicationConfigToDatabase(ctx, newEnv, db) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ApplyReplicationConfig
creates the commit hooks and is responsible for the transform of the dsess.SqlDatabase into a ReadOnlyDatabase if it needs to be configured as such. Here, we just need the later functionality: the other functionality is already handled (and more) by our InitDatabaseHook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
if replicationRemoteName == "" { | ||
return nil | ||
} | ||
func NewConfigureReplicationDatabaseHook(bThreads *sql.BackgroundThreads) func(ctx *sql.Context, p *DoltDatabaseProvider, name string, newEnv *env.DoltEnv, _ dsess.SqlDatabase) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda hate that the hook is a func instead of an interface but I guess that's not new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crazy this is a new file, what would uncle bob martin say
@@ -115,6 +115,22 @@ func newReplicaDatabase(ctx context.Context, name string, remoteName string, dEn | |||
return rrd, nil | |||
} | |||
|
|||
func applyReadReplicationConfigToDatabase(ctx context.Context, dEnv *env.DoltEnv, db dsess.SqlDatabase) (dsess.SqlDatabase, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should comment similar to the PR note here
No description provided.