Create a hook to capture copy batch errors and retries #1507
Conversation
Co-authored-by: Bastian Bartmann <[email protected]>
Co-authored-by: Bastian Bartmann <[email protected]>
CalculateNextIterationRangeEndValues needs to be recomputed on every retry in case of configuration (e.g. chunk-size) changes were made by onBatchCopyRetry hooks.
grodowski
requested review from
rashiq,
meiji163 and
timvaillancourt
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new hook, gh-ost-on-batch-copy-retry, to allow dynamic reconfiguration on errors during the batch copy phase, enabling retries with adjustments (e.g. changing the chunk size).
- Adds a new hook constant and a corresponding function in hooks.go
- Integrates hook execution within the migrator's retry workflow via retryBatchCopyWithHooks
- Updates documentation and CI configuration to support the new functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| dev.yml | Added environment variables and commands for CI configuration |
| doc/hooks.md | Updated hook list and documentation with the new batch copy retry hook |
| go/logic/hooks.go | Added hook constant and implementation for gh-ost-on-batch-copy-retry |
| go/logic/migrator.go | Added retryBatchCopyWithHooks and modified usage in iterateChunks |
| @@ -77,6 +78,7 @@ func (this *HooksExecutor) applyEnvironmentVariables(extraVariables ...string) [ | |||
| // executeHook executes a command, and sets relevant environment variables | |||
| // combined output & error are printed to the configured writer. | |||
| func (this *HooksExecutor) executeHook(hook string, extraVariables ...string) error { | |||
| this.migrationContext.Log.Infof("executing hook: %+v", hook) | |||
There was a problem hiding this comment.
[nitpick] The added logging statement in executeHook duplicates the logging in executeHooks and may lead to redundant log entries. Consider removing one of these log statements to avoid unnecessary duplication.
| this.migrationContext.Log.Infof("executing hook: %+v", hook) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| desc: Run all the tests. | ||
| run: | | ||
| export DOCKER_HOST=unix://$(podman machine inspect --format '{{.ConnectionInfo.PodmanSocket.Path}}') | ||
| script/test |
There was a problem hiding this comment.
Specific to Shopify tooling and will be removed.
Related issue: #1499
Description
Adds
gh-ost-on-batch-copy-retrytohooks.go, which enables dynamic reconfiguration on errors inapplyCopyRowsFunc. The use case my team needs is to allow changing thechunk-sizein case of binlog cache size limit errors (see issue example), however this hook can be useful in other scenarios too.script/cibuildreturns with no formatting errors, build errors or unit test errors.