Skip to content

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Feb 6, 2025

This PR moves the flags backfill-batch-delay and backfill-batch-size to
the commands start and migrate. So these options no longer show up in the list
of global flags.

I created a new struct backfill.Config to store and pass the options down
to the migration runner. I also moved the default values for batch delay and batch size
into the package backfill. I renamed options.go to config.go because it is a better name given the new contents of the file.

Closes #657

@kvch kvch marked this pull request as draft February 6, 2025 15:25
@kvch kvch force-pushed the feature-move-backfill-options branch 2 times, most recently from 08a7668 to ae056d6 Compare February 6, 2025 15:46
@kvch kvch force-pushed the feature-move-backfill-options branch from ae056d6 to 4dcd097 Compare February 6, 2025 16:14
@kvch kvch marked this pull request as ready for review February 6, 2025 16:18
@kvch kvch requested a review from andrew-farries February 6, 2025 16:18
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

I think this has broken the flags - they aren't taking effect for me:

Apply the migration:

{
  "name": "01_create_table",
  "operations": [
    {
      "create_table": {
        "name": "items",
        "columns": [
          {
            "name": "id",
            "type": "serial",
            "pk": true
          },
          {
            "name": "name",
            "type": "text"
          }
        ]
      }
    }
  ]
}

Fill the table with data:

insert into items(name) select 'name_' || i from generate_series(1, 1_000_000) as i

Start a migration with a specified delay:

go run . start --backfill-batch-delay=10s 02_set_unique.json

where the migration is defined:

{
  "name": "02_set_unique",
  "operations": [
    {
      "alter_column": {
        "table": "items",
        "column": "name",
        "unique": {
          "name": "items_name_unique"
        },
        "up": "name",
        "down": "name"
      }
    }
  ]
}

There is no delay between each batch during the backfill.

I think the problem is how the flags are defined as PersistentFlags in the start and migrate commands.

@kvch kvch requested a review from andrew-farries February 11, 2025 14:50
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

The flags still don't work for me, using the same reproduction as in this comment:

#660 (review)

@kvch
Copy link
Contributor Author

kvch commented Feb 12, 2025

Fixed the issue

@kvch kvch requested a review from andrew-farries February 12, 2025 10:22
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

Looks good now; the flags work as expected 👍

Left some comments, feel free to address them or not.

@kvch kvch enabled auto-merge (squash) February 13, 2025 10:45
@kvch kvch merged commit cd19f8a into xataio:main Feb 13, 2025
27 checks passed
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.

Move backfill-batch-delay and backfill-batch-size flags to the start command
2 participants