Skip to content
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

go/libraries/doltcore/sqle: DatabaseProvider: Fix a bug where a database created with call dolt_clone() would not be dropped until the server was restarted. #7107

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

reltuk
Copy link
Contributor

@reltuk reltuk commented Dec 6, 2023

Fixes #7106.

…ase created with call dolt_clone() would not be dropped until the server was restarted.

Fixes #7106.
@macneale4 macneale4 self-requested a review December 6, 2023 18:07
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -499,7 +499,7 @@ func (p *DoltDatabaseProvider) CloneDatabaseFromRemote(
return err
}

return ConfigureReplicationDatabaseHook(ctx, p, dbName, dEnv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is happening where now? I only see it in the struct initialization, so I assume this is deferred until later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My read of this was that it was an error that we called ConfigureReplicationDatabaseHook directly here. We had already called InitDatabaseHook, which was responsible for this (and possible more hook configuration) on the newly created database. Since registerNewDatabase() also calls InitDatabaseHook, and there were direct calls to ConfigureReplicationDatabaseHook in the CreateDatabase path, removing this seemed correct to me.

Copy link
Contributor

@macneale4 macneale4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There is something about the initialization which is an unknown to me, but tests pass so I'm sure it's fine

@reltuk reltuk merged commit db3472a into main Dec 6, 2023
30 checks passed
@Hydrocharged Hydrocharged deleted the aaron/sqle-database-provider-clone-dbLocations-fix branch February 7, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attempting to drop a database which was created with dolt_clone fails.
3 participants