Create a hook to capture copy batch errors and retries #1507
Conversation
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.
|
@meiji163 any chance you could review this PR as well? Thank you! 🙇♂️ |
|
@grodowski Can you pull in master branch? I had to update the CI tests |
|
Thanks, tests are green now on our fork. |
| hasFurtherRange := false | ||
| expectedRangeSize := int64(0) | ||
| if err := this.retryOperation(func() (e error) { | ||
| hasFurtherRange, expectedRangeSize, e = this.applier.CalculateNextIterationRangeEndValues() |
There was a problem hiding this comment.
We found an issue that is potentially caused by CalculateNextIterationRangeEndValues being moved to inside applyCopyRowsFunc (and the retry loop). I'll post more info here and add a unit test ⏳
There was a problem hiding this comment.
Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com>
Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com>
grodowski
force-pushed
the
grodowski/coding-chimp/on-retry-hook
branch
from
e98fb12 to
e018cc3
Compare
CalculateNextIterationRangeEndValues needs to be recomputed on every retry in case of configuration (e.g. chunk-size) changes were made by onBatchCopyRetry hooks.
…e insert completes - extract MigrationContext.SetNextIterationRangeValues outside of applyCopyRowsFunc, so that it doesn't run on retries - add an integration test for Migrator with retry hooks Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com>
grodowski
force-pushed
the
grodowski/coding-chimp/on-retry-hook
branch
from
e018cc3 to
a48cae1
Compare
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.