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

feat(db/create): ask to wake sleeping groups before create #900

Closed
wants to merge 7 commits into from

Conversation

notrab
Copy link
Member

@notrab notrab commented Jul 30, 2024

Fixes #894

This does however use a mix of the sleeping and archived terminology. We should clean that up when we know which word we want to go with.

@notrab notrab requested review from penberg and haaawk July 30, 2024 11:24
@notrab
Copy link
Member Author

notrab commented Jul 30, 2024

The original issue #894 also stated that db shell would return the same error. We can use the method there, but I'm not sure where is best considering db shell can be used with local URL, and the groups concept doesn't yet apply there...

spinner.Stop()
			awake, err := ensureGroupAwake(client, db.Group)
			if err != nil {
				return err
			}
			if !awake {
				return fmt.Errorf("cannot connect to a database in an archived group. Please wake up the group first")
			}
			spinner.Start()

@notrab
Copy link
Member Author

notrab commented Jul 30, 2024

I pushed a commit to db_shell.go where I believe is the right place to check, but we can remove that if it's not correct.

internal/cmd/group_utils.go Outdated Show resolved Hide resolved
"github.com/tursodatabase/turso-cli/internal/turso"
)

func ensureGroupAwake(client *turso.Client, groupName string) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of this approach because:

  • We add an extra call on all invocations to handle an edge case that happens once in a while
  • We have to keep track of all ops that are affected by archiving and remember to add this logic to them (e.g. rotating creds, and deleting dbs also doesn't work when the group is archived)

The "better" way would be to be able to detect archived errors from API responses and handle those properly.

Not a blocker though, feel free to move forward if you don't have the time to address this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree this would be better.

internal/cmd/group_utils.go Outdated Show resolved Hide resolved
}

errMsg := actionErr.Error()
if strings.Contains(errMsg, "group_sleeping") ||
Copy link
Member Author

Choose a reason for hiding this comment

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

I fear group_sleeping will never be reached because we don't actually return or formalise the API error from the turso client.


errMsg := actionErr.Error()
if strings.Contains(errMsg, "group_sleeping") ||
(strings.Contains(errMsg, "cannot create database on group") && strings.Contains(errMsg, "because it is archived")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be reached, but I hate having to pass in the string and detect errors like this. I would much prefer to use group_sleeping from the error response code field.

@notrab notrab closed this Dec 9, 2024
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.

Improve error message for sleeping databases
2 participants