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

improve error message when current branch on session is force deleted #4395

Merged
merged 16 commits into from
Sep 28, 2022

Conversation

jennifersp
Copy link
Contributor

@jennifersp jennifersp commented Sep 22, 2022

An active branch in session can be force deleted. The session on deleted branch cannot run any queries that requires dolt transaction. It gives error message with possible actions that user can take, either to run USE <database>/<branch> query or reconnect to the server.
Depends on dolthub/go-mysql-server#1288

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

This is looking pretty close. I think we just need to think through the experience we want for the deleted branch and make sure it's working like we want.

I tested this code out locally and noticed a couple strange behaviors:

  • My first mysql client connected to the database, by default to the main branch. The second mysql client connected to deletetest/branch1 using a revision db spec to connect directly to that branch1 branch. After I deleted branch1 from the first client, the second client got errors like database not found: deletetest/branch1 when I tried to show tables or select from tables. This seemed pretty reasonable to me – if a branch was deleted out from under you, I don't expect everything to work. However, call dolt_checkout didn't work in this case. I think the summary of this one is just to test with revision database specs and see if we can get that case to work so users can still call dolt_checkout.
  • In my next case, both mysql clients initially connected to main and then I checked out branch2 on the second client. After force deleting branch2 from the first client, the second client still worked pretty well. I got a good error when I called active_branch() and I could still access the data to select from tables. However... I was also able to call dolt_commit and create a new commit, which I don't think we want to allow since you're creating commits off a head that should be removed. I also noticed that after calling dolt_commit the branch2 branch was showing up again for both clients in the dolt_branches table.

So, overall, this seems pretty close, but there are some tricky edge cases worth digging into a little deeper. I think the most important behaviors I'd expect for a session connected to a branch that was deleted out from under it, are:

  • the user has some clue that the branch they were working on has been forcefully removed and they know what to do next to get out of that state. Probably just a good error message to solve this.
  • the user is unable to write commits or do anything to change the branch HEAD, since it has been deleted.
  • the user can checkout a different branch.

Still being able to access data in a read-only mode might be a nice to have, but not nearly as important as the other requirements. I think I prefer bringing attention as soon as possible to the fact that the branch for the session was deleted so the user can deal with it appropriately.

@jennifersp jennifersp changed the title allow nil working set in dolt session transaction improve error message when current branch on session is force deleted Sep 23, 2022
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

The new error message is very nice. I love that it gives the customer an actionable next step.

I played with this locally, too, and it seems to work pretty well, particularly when @autocommit=1. I think it just needs some tests covering when @autocommit=0 to make sure we understand and test the behavior there, too.

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Just a quick minor suggestion for the error message text. I like the direction you're going with the validation callback from GMS. That could also allow us to move the call out of beginTransaction so we could do it at the start of every query, which would remove the complexity around autocommit behavior.

go/libraries/doltcore/sqle/dsess/session.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for iterating on this one. I think you came to a very nice, clean result with this code!

@jennifersp jennifersp merged commit f99a171 into main Sep 28, 2022
@jennifersp jennifersp deleted the jennifer/delete-branch branch September 28, 2022 21:04
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.

Deleting branch causes error in another session if that branch is active
2 participants