[DO NOT MERGE] Testing #1500 #1518
Conversation
- log insert warnings always - terminate if row count doesn't match and the PanicOnWarnings flag is set
…-type-change/extra_args Co-authored-by: Bastian Bartmann <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces changes to improve iteration range handling and SQL warning detection during data migration while adding new configuration options. Key changes include:
- Modifying the return signature and logic of CalculateNextIterationRangeEndValues in applier.go.
- Integrating SQL warning detection in ApplyIterationInsertQuery and adjusting error handling in migrator.go.
- Updating SQL query builders and associated tests, and adding a new flag "panic-on-warnings" in main.go.
Reviewed Changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dev.yml | Adds a new development configuration file for test environments. |
| go/logic/applier.go | Updates iteration range calculation and SQL warnings processing logic. |
| go/logic/migrator.go | Adjusts iteration processing to incorporate expected row count and SQL warning handling. |
| go/sql/builder_test.go | Updates test expectations for SQL query generation. |
| go/sql/builder.go | Revises SQL builder functions and notes an unused variable. |
| go/cmd/gh-ost/main.go | Introduces a new CLI flag for triggering panic on SQL warnings. |
| go/base/context.go | Adds new context fields to support panic-on-warnings functionality. |
Files not reviewed (6)
- localtests/panic-on-warnings-duplicate-unique-values-on-column-type-change/create.sql: Language not supported
- localtests/panic-on-warnings-duplicate-unique-values-on-column-type-change/expect_failure: Language not supported
- localtests/panic-on-warnings-duplicate-unique-values-on-column-type-change/extra_args: Language not supported
- localtests/panic-on-warnings-duplicate-values-for-unique-index/create.sql: Language not supported
- localtests/panic-on-warnings-duplicate-values-for-unique-index/expect_failure: Language not supported
- localtests/panic-on-warnings-duplicate-values-for-unique-index/extra_args: Language not supported
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
| - podman | ||
| - custom: | ||
| name: Go Dependencies | ||
| met?: go mod download |
There was a problem hiding this comment.
The key 'met?' is inconsistent with the key 'meet' on the following line, which may lead to configuration errors. Both keys should be identical if they are intended to represent the same check.
Copilot uses AI. Check for mistakes.
| } | ||
| if expectedRangeSize != rowsAffected { | ||
| joinedWarnings := strings.Join(this.migrationContext.MigrationLastInsertSQLWarnings, "; ") | ||
| terminateRowIteration(fmt.Errorf("ApplyIterationInsertQuery failed because of SQL warnings: [%s]", joinedWarnings)) |
There was a problem hiding this comment.
The call to 'terminateRowIteration' is not returned, so its effect on the control flow is lost. Prepend 'return' to properly terminate the iteration when SQL warnings occur.
| terminateRowIteration(fmt.Errorf("ApplyIterationInsertQuery failed because of SQL warnings: [%s]", joinedWarnings)) | |
| return terminateRowIteration(fmt.Errorf("ApplyIterationInsertQuery failed because of SQL warnings: [%s]", joinedWarnings)) |
Copilot uses AI. Check for mistakes.
| @@ -275,7 +275,7 @@ func BuildUniqueKeyRangeEndPreparedQueryViaOffset(databaseName, tableName string | |||
|
|
|||
| uniqueKeyColumnNames := duplicateNames(uniqueKeyColumns.Names()) | |||
| uniqueKeyColumnAscending := make([]string, len(uniqueKeyColumnNames)) | |||
| uniqueKeyColumnDescending := make([]string, len(uniqueKeyColumnNames)) | |||
| uniqueKeyColumnDescending := make([]string, len(uniqueKeyColumnNames)) // TODO unused variable | |||
There was a problem hiding this comment.
The variable 'uniqueKeyColumnDescending' is declared but never used. Remove it if it is not needed to avoid confusion.
| uniqueKeyColumnDescending := make([]string, len(uniqueKeyColumnNames)) // TODO unused variable |
Copilot uses AI. Check for mistakes.
Testing #1500 internally. Do not merge.