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

Timeout Flag #627

Merged
merged 7 commits into from
Dec 15, 2023
Merged

Timeout Flag #627

merged 7 commits into from
Dec 15, 2023

Conversation

smantic
Copy link
Contributor

@smantic smantic commented Oct 30, 2023

Adds a CLI flag -timeout so that users can specify a duration for how long the migrations should be able to run.

Closes #576

Some edge cases

  • Finished migration steps will not have been rolled back if a migration times out later.
  • if you specify NO TRANSACTION the timeout wont be respected if it's been exceeded for that part of the migration.

Let me know if I should change anything else.

@smantic smantic changed the title add option for migrations to timeout Timeout Flag Oct 30, 2023
@@ -99,6 +100,56 @@ func TestMigrateUpTo(t *testing.T) {
check.Number(t, gotVersion, upToVersion) // incorrect database version
}

func TestMigrateUpTimeout(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these tests are simulating the library timeout, but we should probably have something that exercises the --timeout flag and uses pg_sleep (sqlite would be ideal, but I don't think there's a way to do that in sqlite)

Copy link
Collaborator

@mfridman mfridman Dec 15, 2023

Choose a reason for hiding this comment

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

Made some changes in 303331c, I think this captures what you described in the description, and exercises a "long-running" migration instead of starting with an already cancelled context. With a slight modification in 363e629 to start the context timeout after the container started.

README.md Outdated
@@ -105,6 +105,8 @@ Options:
file path to SSL key in pem format (only supported on mysql)
-table string
migrations table name (default "goose_db_version")
-timeout duration
duration that the queries should run for; e.g. 1h13m
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
duration that the queries should run for; e.g. 1h13m
maximum duration queries allowed to run; e.g. 1h13m

Not sure if that's better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Went with:

-timeout duration
      maximum allowed duration for queries to run; e.g., 1h13m

cmd/goose/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mfridman mfridman left a comment

Choose a reason for hiding this comment

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

Thanks for putting that together, and apologies for delay in reviewing.

@mfridman
Copy link
Collaborator

Finished migration steps will not have been rolled back if a migration times out later.

You mean something like this, right? Where timeout was set to 5s, but migration 2 had a pg_sleep(10) and it failed to apply but migration 1 succeeded before the timeout.

2023/12/13 23:15:57 OK 00001_users_table.sql (8.17ms)
2023/12/13 23:16:02 goose run: ERROR 00002_posts_table.sql: failed to run SQL migration: failed to execute SQL query "SELECT pg_sleep(10);": timeout: context deadline exceeded

if you specify NO TRANSACTION the timeout wont be respected if it's been exceeded for that part of the migration.

Same as the previous one, but in this case goose partially migrated before the timeout was reached?

Note, this isn't really needed since the context outlives the main function. But go is being difficult despite adding nolint comment

//nolint:govet  // The context outlives the main function
@mfridman mfridman merged commit 90a4f4f into pressly:master Dec 15, 2023
9 checks passed
@smantic
Copy link
Contributor Author

smantic commented Dec 15, 2023

Thanks for putting that together, and apologies for delay in reviewing.

All good! Glad I got a chance to contribute!
WRT the edge cases, yes thats the jist of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timeout Flag
2 participants