Skip to content

feat(active-active): Add MySQL support for cluster selection policy#8014

Open
joannalauu wants to merge 2 commits intocadence-workflow:masterfrom
joannalauu:feat/active-mysql
Open

feat(active-active): Add MySQL support for cluster selection policy#8014
joannalauu wants to merge 2 commits intocadence-workflow:masterfrom
joannalauu:feat/active-mysql

Conversation

@joannalauu
Copy link
Copy Markdown
Contributor

@joannalauu joannalauu commented Apr 26, 2026

What changed?
Added MySQL support for cluster selection policy

  • Added active_cluster_selection_policy table in MySQL database
  • Implemented CRUD methods for MySQL database updates (insert, select, delete)
  • Implemented GetActiveClusterSelectionPolicy and DeleteActiveClusterSelectionPolicy for sql stores
  • Runs insertActiveClusterSelectionPolicy when new workflow is created

Why?
Implement MySQL support for active-active domains

How did you test it?
go test ./common/persistence/sql/sqlplugin/mysql ./common/persistence/sql/
Integration tests for MySQL

Potential risks

Release notes

Documentation Changes


Detailed Description
[In-depth description of the changes made to the schema or interfaces, specifying new fields, removed fields, or modified data structures]
Added MySQL support for cluster selection policy

  • Added active_cluster_selection_policy table in MySQL database
  • Implemented CRUD methods for MySQL database updates (insert, select, delete)
  • Implemented GetActiveClusterSelectionPolicy and DeleteActiveClusterSelectionPolicy for sql stores
  • Runs insertActiveClusterSelectionPolicy when new workflow is created

Impact Analysis

  • Backward Compatibility: Doesn't affect existing Cassandra support for active-active, and doesnt affect features that MySQL already supports
  • Forward Compatibility: Interface layer for row structure and CRUD methods allow more SQL database to be supported in the future

Testing Plan

  • Unit Tests: go test ./common/persistence/sql/sqlplugin/mysql ./common/persistence/sql/
  • Persistence Tests: [If the change is related to a data type which is persisted, do we have persistence tests covering the change?]
  • Integration Tests: Integration test for MySQL
  • Compatibility Tests: [Have we done tests to test the backward and forward compatibility?]

Rollout Plan

  • What is the rollout plan?
  • Does the order of deployment matter?
  • Is it safe to rollback? Does the order of rollback matter?
  • Is there a kill switch to mitigate the impact immediately?

Comment on lines +59 to +73
func (mdb *DB) SelectFromActiveClusterSelectionPolicy(ctx context.Context, filter *sqlplugin.ActiveClusterSelectionPolicyFilter) (*sqlplugin.ActiveClusterSelectionPolicyRow, error) {
dbShardID := sqlplugin.GetDBShardIDFromHistoryShardID(filter.ShardID, mdb.GetTotalNumDBShards())
var row sqlplugin.ActiveClusterSelectionPolicyRow
err := mdb.driver.GetContext(
ctx,
dbShardID,
&row,
getActiveClusterSelectionPolicyQuery,
filter.ShardID,
filter.DomainID,
filter.WorkflowID,
filter.RunID,
)
if err != nil {
return nil, err
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: MySQL SELECT query doesn't use db struct tags for column mapping

In SelectFromActiveClusterSelectionPolicy (mysql/active_cluster_selection_policy.go:62-71), the query selects columns like shard_id, domain_id, workflow_id, run_id, data, data_encoding and scans into ActiveClusterSelectionPolicyRow via GetContext. The struct relies on sqlx.MapperFunc(strcase.ToSnake) for automatic mapping (e.g., DataEncodingdata_encoding). This is consistent with the existing codebase pattern, but worth noting that ShardID maps to shard_i_d under strcase.ToSnake in some implementations. Verify that the mapper correctly handles the ID suffix — if it produces shard_i_d instead of shard_id, the SELECT will fail silently with zero values. Other structs in the codebase have the same pattern so this is likely fine, but worth a quick integration test.

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

Signed-off-by: Joanna Lau <118241363+joannalauu@users.noreply.github.com>
…postgres and sqlite

Signed-off-by: Joanna Lau <118241363+joannalauu@users.noreply.github.com>
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 27, 2026

CI failed: The CI analysis identified no build or test failures in the provided logs, as they only captured the successful setup and initialization phases of the job.

Overview

The provided logs contain only successful repository checkout and Go environment setup; no actual test execution or build output was present to analyze.

Failures

Incomplete Log Output (confidence: high)

  • Type: infrastructure
  • Affected jobs: 73141181197
  • Related to change: unclear
  • Root cause: The provided log snippet is truncated and only includes the initialization phase of the CI job.
  • Suggested fix: Provide the complete log output for the specific test or build steps that failed.

Summary

  • Change-related failures: 0
  • Infrastructure/flaky failures: 1 (Incomplete logs provided for analysis)
  • Recommended action: The current CI logs do not show any failures. If the build is failing, please verify that the full build output is being captured and provided for analysis.
Code Review 👍 Approved with suggestions 1 resolved / 2 findings

Adds MySQL support for the cluster selection policy and resolves the silent error handling bug in Postgres/SQLite delete operations. Ensure the MySQL SELECT query is updated to use db struct tags for consistent column mapping.

💡 Quality: MySQL SELECT query doesn't use db struct tags for column mapping

📄 common/persistence/sql/sqlplugin/mysql/active_cluster_selection_policy.go:59-73

In SelectFromActiveClusterSelectionPolicy (mysql/active_cluster_selection_policy.go:62-71), the query selects columns like shard_id, domain_id, workflow_id, run_id, data, data_encoding and scans into ActiveClusterSelectionPolicyRow via GetContext. The struct relies on sqlx.MapperFunc(strcase.ToSnake) for automatic mapping (e.g., DataEncodingdata_encoding). This is consistent with the existing codebase pattern, but worth noting that ShardID maps to shard_i_d under strcase.ToSnake in some implementations. Verify that the mapper correctly handles the ID suffix — if it produces shard_i_d instead of shard_id, the SELECT will fail silently with zero values. Other structs in the codebase have the same pattern so this is likely fine, but worth a quick integration test.

✅ 1 resolved
Bug: Postgres/SQLite Delete silently succeeds instead of returning error

📄 common/persistence/sql/sqlplugin/postgres/active_cluster_selection_policy.go:41-42 📄 common/persistence/sql/sqlplugin/sqlite/active_cluster_selection_policy.go:41-42
In the Postgres and SQLite stubs, InsertIntoActiveClusterSelectionPolicy and SelectFromActiveClusterSelectionPolicy correctly return a 'not implemented' error, but DeleteFromActiveClusterSelectionPolicy returns nil, nil — silently succeeding. This inconsistency means that if DeleteActiveClusterSelectionPolicy is called against a Postgres/SQLite backend, the caller will believe the row was deleted when no operation occurred. While the feature targets MySQL, this could mask bugs during testing or if backends are misconfigured.

🤖 Prompt for agents
Code Review: Adds MySQL support for the cluster selection policy and resolves the silent error handling bug in Postgres/SQLite delete operations. Ensure the MySQL SELECT query is updated to use db struct tags for consistent column mapping.

1. 💡 Quality: MySQL SELECT query doesn't use db struct tags for column mapping
   Files: common/persistence/sql/sqlplugin/mysql/active_cluster_selection_policy.go:59-73

   In `SelectFromActiveClusterSelectionPolicy` (mysql/active_cluster_selection_policy.go:62-71), the query selects columns like `shard_id, domain_id, workflow_id, run_id, data, data_encoding` and scans into `ActiveClusterSelectionPolicyRow` via `GetContext`. The struct relies on `sqlx.MapperFunc(strcase.ToSnake)` for automatic mapping (e.g., `DataEncoding` → `data_encoding`). This is consistent with the existing codebase pattern, but worth noting that `ShardID` maps to `shard_i_d` under `strcase.ToSnake` in some implementations. Verify that the mapper correctly handles the `ID` suffix — if it produces `shard_i_d` instead of `shard_id`, the SELECT will fail silently with zero values. Other structs in the codebase have the same pattern so this is likely fine, but worth a quick integration test.

Rules ❌ No requirements met

Repository Rules

PR Description Quality Standards: The PR description contains empty sections ([Potential risks], [Release notes], [Documentation Changes]) and the [Why?] section lacks sufficient technical context explaining the implementation approach for MySQL.

2 rules not applicable. Show all rules by commenting gitar display:verbose.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@joannalauu
Copy link
Copy Markdown
Contributor Author

all unit tests are passing locally

@c-warren c-warren self-assigned this Apr 28, 2026
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.

2 participants