Skip to content

feat(shard-distributor): integrate dynamic config for spectator#7722

Merged
jakobht merged 3 commits intocadence-workflow:masterfrom
jakobht:integrate_enableSMspectator
Feb 20, 2026
Merged

feat(shard-distributor): integrate dynamic config for spectator#7722
jakobht merged 3 commits intocadence-workflow:masterfrom
jakobht:integrate_enableSMspectator

Conversation

@jakobht
Copy link
Member

@jakobht jakobht commented Feb 19, 2026

What changed?
Integrated dynamic config check for spectator enablement and exported ModeKey constants in membership package

Why?
The spectator client needs to respond to dynamic config changes for MatchingShardDistributionMode. Previously, ModeKey constants were private in the membership package, preventing external packages from checking the current mode. This change exports the ModeKey type and constants, and adds an Enabled function to spectator params that dynamically checks if the mode is not hash_ring. This allows the spectator to automatically enable/disable based on the dynamic config without requiring restarts.

How did you test it?

  • Unit tests: go test -v ./common/membership -run TestShardDistributorResolver
  • All tests pass with the exported constants

Potential risks

  • API change: Exported previously private ModeKey type and constants (ModeKeyHashRing, ModeKeyShardDistributor, ModeKeyHashRingShadowShardDistributor, ModeKeyShardDistributorShadowHashRing). This is backwards compatible as it only adds public visibility without breaking existing code.
  • Behavior change: Spectator now dynamically responds to config changes rather than using a static configuration

Release notes
N/A - internal improvement to spectator configuration handling

Documentation Changes
N/A

- Added Enabled function to spectator params that checks MatchingShardDistributionMode
- Exported ModeKey constants in membership package for reuse
- Spectator now responds to dynamic config changes between hash_ring and shard_distributor modes

Signed-off-by: Jakob Haahr Taankvist <jht@uber.com>
Updated test file to use the new exported ModeKey constants after
they were changed from private modeKey in the main implementation.

Signed-off-by: Jakob Haahr Taankvist <jht@uber.com>
Signed-off-by: Jakob Haahr Taankvist <jht@uber.com>
@gitar-bot
Copy link

gitar-bot bot commented Feb 19, 2026

🔍 CI failure analysis for ad70169: Two CI test failures detected - both appear to be flaky infrastructure issues unrelated to the PR's shard distributor changes.

Failure 1: Cassandra Integration Tests - Cassandra Session Closure

Issue

The host/persistence/cassandra integration tests are failing with Cassandra session errors:

error: "session has been closed"
UpdateTaskList operation failed. Error: session has been closed

These errors occur during test execution when Cassandra persistence operations are attempted, indicating the database session was prematurely closed or lost.

Root Cause

This failure is not related to the PR changes. The PR modifies shard distributor spectator initialization and membership resolver constants. These changes do not affect:

  • Cassandra session management
  • Database connection lifecycle
  • Persistence layer operations

The "session has been closed" error typically indicates:

  • Infrastructure/network instability during test execution
  • Cassandra container lifecycle issues in the test environment
  • Race conditions in test setup/teardown

Failure 2: Replication Simulation (activeactive_same_wfid)

Issue

The TestReplicationSimulation test for the activeactive_same_wfid scenario fails with:

Operation 1 failed: Workflow execution is already running. WorkflowId: wf1, RunId: d390d0de-37e8-441f-9ba0-98ccfa7c86da

Root Cause

This failure is also not related to the PR changes. The PR does not modify replication logic, workflow ID conflict detection, or active-active cluster coordination.

The test failed on both retry attempts, indicating a race condition or timing issue in the replication simulation test infrastructure.


Details

The PR's modified files are:

  • cmd/server/cadence/server.go - adds dynamic config check for spectator enablement
  • common/membership/sharddistributorresolver.go - exports ModeKey type and constants
  • common/membership/sharddistributorresolver_test.go - updates tests to use exported constants

None of these changes interact with Cassandra persistence, replication logic, or the code paths exercised by these failing tests.

Code Review ✅ Approved 1 resolved / 1 findings

Previous finding (spurious discarded call) has been fixed. The remaining changes cleanly export ModeKey type/constants and wire up dynamic spectator enablement — looks good.

✅ 1 resolved
Bug: Spurious discarded call to matchingShardDistributionMode()

📄 cmd/server/cadence/server.go:191
On line 191, matchingShardDistributionMode() is called and its return value is discarded. The actual comparison uses a second call on line 192. This is dead code that appears to be an accidental leftover.

While the second call on line 192 produces the correct result, the discarded call on line 191:

  1. Is confusing to future maintainers (looks intentional but serves no purpose)
  2. Makes an unnecessary call to the dynamic config getter
  3. Could theoretically return a different value than the second call if config changes between the two invocations (TOCTOU), though this is unlikely to cause issues in practice

The fix is simply to remove the standalone call on line 191.

Rules ❌ No requirements met

Repository Rules

PR Description Quality Standards: Add test command for new dynamic enablement function and integration/manual test demonstrating config change → spectator state transition

1 rule 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

@jakobht jakobht merged commit 05a2412 into cadence-workflow:master Feb 20, 2026
72 of 74 checks passed
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