Add a retry mechanism for `migrate_with_timeouts` #20

marcelofern · 2024-09-03T02:27:23Z

This branch adds a retry mechanism for migrate_with_timeouts.

Before

  ./manage.py migrate_with_timeouts \
    --lock-timeout-in-ms=10000 \
    --statement-timeout-in-ms=10000

After

  ./manage.py migrate_with_timeouts \
    --lock-timeout-in-ms=10000 \
    --lock-timeout-max-retries=3 \
    --lock-timeout-retry-exp=2 \
    --lock-timeout-retry-min-wait-in-ms=3000 \
    --lock-timeout-retry-max-wait-in-ms=10000 \
    --statement-timeout-in-ms=10000 \
    --retry-callback--path="dotted.path.to.callback.function"

github-actions · 2024-09-09T05:01:06Z

Coverage Report Results

Name	Stmts	Miss	Branch	BrPart	Cover
src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py	114	0	38	0	100%
src/django_pg_migration_tools/operations.py	67	0	6	0	100%
src/django_pg_migration_tools/timeouts.py	73	5	36	2	92%
TOTAL	254	5	80	2	97%

3 empty files skipped.

delfick

nice :)

I've added a suggestion that moves the code around a little.

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

delfick · 2024-09-12T11:32:01Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

-            super().handle(*args, **options)
+    def _migrate(self, *django_migrate_args: Any, **django_migrate_kwargs: Any) -> None:
+        migration_applied: bool = False
+        while self._can_migrate() and (migration_applied is False):


Suggested change

while self._can_migrate() and (migration_applied is False):

while self._can_migrate():

and then do a return instead of migration_applied = True

And then the raising of the exception does need the if not migration_applied guard cause the only time that would be reached would be if the migration didn't happen.

delfick · 2024-09-12T11:33:41Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+            # This raises ModuleNotFoundError, which gives a good explanation
+            # of the error already (see tests). We don't have to wrap this into
+            # our own exception.
+            *module, callback = callback_path.split(".")


in my suggestion I do a module, attr_name = callaback_path.rsplit(".") here to avoid needing the ".".join(module). I also make sure the path has at least one dot in it and I assert callable(callback)

timb07

I'm unsure of the need for retries on statement timeout — please convince me that the extra complexity they add is worthwhile.

timb07 · 2024-09-13T00:19:55Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+def _parse_non_required_ms_option_to_timedelta(
+    options: Any, key: str
+) -> datetime.timedelta | None:
+    value = options.pop(key, None)


🐼 The word "parse" in the function name suggests only parsing, not mutating the options dict. Naming things is hard, but perhaps "pop" or "extract" would be better.

[addressed] I've picked up the pop prefix instead.

timb07 · 2024-09-13T00:23:28Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+    statement_timeouts: int
+    statement_timeouts_max_retries: int
+    statement_timeout_retry_exp: int
+    statement_timeout_retry_max_wait: datetime.timedelta
+    statement_timeout_retry_min_wait: datetime.timedelta


🤔 There's a strong use case for having retries for lock timeouts, but I'm struggling to see why retrying in case of statement timeouts would be useful. Do we expect a statement that times out the first time to somehow become faster and not to timeout when retried? What's an example of such a statement where that's likely?

@timb07

Do we expect a statement that times out the first time to somehow become faster and not to timeout when retried?

Good question. I think that this is a possible situation because of temporary load spikes on the database, caused by either background tasks or maintenance tasks starving server resources.

E.g., a vacuum finishing between retries could make the schema change query execute faster and thus pass on a second attempt. Adding a unique constraint to a table with many dead rows is an example of such query. The constraint generation would need to step through many dead-rows and thus have to load more pages from disk to finish its job before the vacuum which could substantially change the results.

I do agree though, that it doesn't feel as useful as lock_timeout retries and its usage might be limited to fewer situations.

It almost makes sense to add a exponential back-off for setting statement_timeout to ever increasing values up to a limit, but that sounds like a different feature.

I agree that the examples you've given do fit the scenario I asked about. But they're not terribly convincing as a justification for the need for this feature. Whereas lock timeouts + automatic retries is well-established best practice (e.g. this blogpost) for applying migrations.

And let's go back to why we might want a statement timeout for migrations in the first place. The main reason (as I understand it) is that we don't want migrations to take too long, because they delay the overall deployment process. So if we hit the statement timeout, isn't retrying the wrong thing to do?

No problems, I will remove this part of the feature and if we find better use cases in the future we can add it back.

Addressed in:

bd5a5ed

5c35e23

timb07 · 2024-09-13T00:26:30Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

@@ -31,6 +54,111 @@ def add_arguments(self, parser: base.CommandParser) -> None:
            required=False,
            help="Value to set as statement_timeout in milliseconds.",
        )
+        parser.add_argument(
+            "--retry-callback--path",


Extra hyphen: ...callback--path

Fat finger + auto complete gone wrong:

5f2326f

79e7b8f

timb07 · 2024-09-13T00:27:07Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+            help=(
+                "How many times to retry after a lock timeout happened. "
+                "Defaults to zero, which means no retries - the migration "
+                "fails immediatelly upon lock timeout."


typo: "immediately"

timb07 · 2024-09-13T00:31:48Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+            required=False,
+            default=2,
+            help=(
+                "The value for the exponent for retry backoff propagation. "


🐼 "propagation" doesn't seem like the right word, but it's not clear to me what the right word is. Would "delay" work?

timb07 · 2024-09-13T00:35:03Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+    if value is None:
+        return None
+    assert isinstance(value, int)
+    return datetime.timedelta(seconds=int(value / 1_000))


Why have the argument in milliseconds if it's going to be rounded down to a whole number of seconds anyway?

[Addressed] That's a problem. I wasn't aware that timedelta had a miliseconds argument as I was biased by the author's preference for microseconds:

timb07 · 2024-09-13T00:38:55Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+                )
+                self._attempt_callback(exc)
+
+        if migration_applied is False:


Pet peeve: comparison of booleans.

Prefer: if not migration_applied:

(and also above)

timb07 · 2024-09-13T00:44:20Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+            # current_attempt is an integer, but it is turned into a float here
+            # because a huge exponentiation in Python between integers
+            # **never** overflows. Instead, the CPU is left trying to calculate
+            # the result forever and it will eventually return a memory error
+            # instead. Which we absolutely do not want. Please see:
+            # https://docs.python.org/3.12/library/exceptions.html#OverflowError
+            result = exp ** (float(current_attempt))


How many retries do we expect users to attempt? More than thousands???

Counterpoint: this code will cope with any number of retries, and with no particular downsides apart from the need to explain why the cast to float is done.

Not in the thousands. Hopefully not even in the hundreds.
You can, of course, run into overflow errors in less than a thousand attempts for a high exponential value - but I also hope this not to be a common scenario. Giving that I am "hoping for many things" I agree that it's best to make the code work for any number of retries lest an arbitrary limiting number is incorrectly chosen and we have to go through a soft deprecation process.

timb07 · 2024-09-13T00:47:51Z

docs/usage/management_commands.rst

+retry callback:
+++++++++++++++
+
+* ``--retry-callback--path``: Sets a callback to be called after a timeout


copy-paste: remove double hyphen here: ...callback--path

Refer to #20 (comment)

timb07 · 2024-09-13T00:48:15Z

docs/usage/management_commands.rst

+    --statement-timeout-retry-exp=2 \
+    --statement-timeout-retry-min-wait-in-ms=3000 \
+    --statement-timeout-retry-max-wait-in-ms=10000 \
+    --retry-callback--path="dotted.path.to.callback.function"


Refer to #20 (comment)

delfick · 2024-09-17T04:00:30Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+    retry_callback: Callable[[RetryState], None] | None
+
+    statement_timeout: datetime.timedelta | None
+
+    lock_timeout: datetime.timedelta | None
+    lock_timeouts: int
+    lock_timeouts_max_retries: int
+    lock_timeout_retry_exp: int
+    lock_timeout_retry_max_wait: datetime.timedelta
+    lock_timeout_retry_min_wait: datetime.timedelta
+


these aren't needed anymore right?

yikes yes, too many stashes. Fixup: d33f59d

delfick · 2024-09-17T04:00:53Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+                    statement_timeout=timeout_options.statement_timeout,
+                ):
+                    super().handle(*args, **options)
+                    migration_applied = True


Suggested change

migration_applied = True

return

And then remove the migration_applied flag

Good shout db207cd

This new dataclass is responsible for capturing the options necessary to run a migration with timeouts. It extracts the logic out of the Command class, so that once we add further configuration (a retry mechanism), the Command class can remain the same, at the expense of this dataclass growing instead.

This new class is responsible for parsing some of the input fields to the migrate_with_timeouts command that cannot be parsed with the `parser` class provided by Django. This also removes the parsing logic out of the MigrationTimeoutOptions class into a dedicated _Parser class.

delfick · 2024-09-19T04:33:57Z

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py

+class MigrateRetryStrategy:
+    timeout_options: MigrationTimeoutOptions
+    retries: int
+
+    def __init__(self, timeout_options: MigrationTimeoutOptions):
+        self.timeout_options = timeout_options
+        self.retries = 0


why not an dataclass.dataclass(frozen=True)?

I think there's something strange about putting that much logic behind a dataclass

delfick · 2024-09-19T04:36:47Z

tests/django_pg_migration_tools/management/commands/test_migrate_with_timeouts.py

+                "migrate_with_timeouts",
+                lock_timeout_in_ms=50_000,
+                lock_timeout_max_retries=2,
+                retry_callback_path="tests.django_pg_migration_tools.management.commands.test_migrate_with_timeouts.example_callback",


Suggested change

retry_callback_path="tests.django_pg_migration_tools.management.commands.test_migrate_with_timeouts.example_callback",

retry_callback_path=f"{__name__}.example_callback",

Nice

fixup: 9c0784b

delfick

very nice :)

The `migrate_with_timeouts` MC currently supports two flags: 1. --lock-timeout-in-ms 2. --statement-timeout-in-ms This commit will add changes to this command, so that we can embed retry mechanisms into it. The command will be able to be called as following: ```python from django.core.management import call_command call_command( "migrate_with_timeouts", # Original flags lock_timeout_in_ms=1_000, statement_timeout_in_ms=2_000, # A callback function to be called back between retries. retry_callback_path="dotted.path.to.callback.function", # Lock retry configuration lock_timeout_max_retries=2, # Retry twice lock_timeout_retry_exp=2, # back-off expo base lock_timeout_retry_min_wait_in_ms=3_000, # wait at least 3s lock_timeout_retry_max_wait_in_ms=10_000, # wait no more than 10s ``` During design, adding a retry mechanism for statement timeouts was discarded as no apparent use for it existed at the time. Therefore, there's only support for lock retries.

To include input fields for the retry mechanism.

marcelofern marked this pull request as draft September 3, 2024 02:27

marcelofern force-pushed the add-retry-for-migrate branch 2 times, most recently from e23f421 to 7eefab0 Compare September 9, 2024 05:00

marcelofern force-pushed the add-retry-for-migrate branch 5 times, most recently from 89a50a5 to 87b7234 Compare September 12, 2024 02:52

marcelofern marked this pull request as ready for review September 12, 2024 02:55

delfick reviewed Sep 12, 2024

View reviewed changes

src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py Show resolved Hide resolved

delfick reviewed Sep 12, 2024

View reviewed changes

timb07 reviewed Sep 13, 2024

View reviewed changes

marcelofern force-pushed the add-retry-for-migrate branch 8 times, most recently from e56c8fc to a32c8a3 Compare September 17, 2024 03:40

delfick reviewed Sep 17, 2024

View reviewed changes

marcelofern force-pushed the add-retry-for-migrate branch 5 times, most recently from b39a90d to c482024 Compare September 19, 2024 02:20

marcelofern force-pushed the add-retry-for-migrate branch from c482024 to 2365f1e Compare September 19, 2024 02:24

marcelofern requested a review from delfick September 19, 2024 03:07

delfick reviewed Sep 19, 2024

View reviewed changes

delfick approved these changes Sep 19, 2024

View reviewed changes

marcelofern added 2 commits September 19, 2024 20:03

Update migrate_with_timeouts MC documentation

92fd7b3

To include input fields for the retry mechanism.

marcelofern force-pushed the add-retry-for-migrate branch from 9c0784b to 92fd7b3 Compare September 19, 2024 08:03

marcelofern merged commit 2cd7df2 into main Sep 19, 2024
10 checks passed

marcelofern deleted the add-retry-for-migrate branch September 19, 2024 21:18

	while self._can_migrate() and (migration_applied is False):
	while self._can_migrate():

	retry_callback_path="tests.django_pg_migration_tools.management.commands.test_migrate_with_timeouts.example_callback",
	retry_callback_path=f"{__name__}.example_callback",

Add a retry mechanism for migrate_with_timeouts #20

Add a retry mechanism for migrate_with_timeouts #20

Conversation

marcelofern commented Sep 3, 2024 • edited Loading

Before

After

github-actions bot commented Sep 9, 2024 • edited Loading

Coverage Report Results

delfick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timb07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofern Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofern Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delfick left a comment

Choose a reason for hiding this comment

Add a retry mechanism for `migrate_with_timeouts` #20

Add a retry mechanism for `migrate_with_timeouts` #20

marcelofern commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

marcelofern Sep 16, 2024 •

edited

Loading

marcelofern Sep 16, 2024 •

edited

Loading