namespace replication: extract shared replication helpers (PR0) by qyc5937 · Pull Request #12112 · temporalio/temporal

qyc5937 · 2026-09-16T20:41:16Z

Stack: PR0 of 7

Summary

extract the namespace replication gate into common/namespace/nsreplication
extract namespace detail-to-task-attributes conversion for reuse by legacy and CHASM paths
preserve existing legacy replication and lifecycle-event behavior

Tests

go test -tags test_dep ./common/namespace/nsreplication/... ./chasm/lib/namespacereplication/...
make fmt-imports
make lint-code-fast

Operational impact

No behavior change; this is shared preparation for the CHASM namespace-replication stack.

… converter (no behavior change) Extract two pure functions out of HandleTransmissionTask: - ShouldReplicateNamespace: the replicate/skip gate (global + has-peer + not-deleted) - NamespaceDetailToTaskAttributes: the NamespaceDetail -> NamespaceTaskAttributes wire conversion consumed by the receiver-side apply-if-higher logic and rewrite HandleTransmissionTask to call them. Behavior is unchanged; only the DELETED short-circuit moved ahead of the gate (outcome-equivalent: DELETED always skips, and non-global/single-cluster still skip only when !forceReplicate). This is groundwork for an eventual CHASM-based namespace replication transport: that path will share the same gate and the same detail->wire converter so the two transports can never diverge on which mutations replicate or on the payload shape they emit. Today only the legacy queue path calls the extracted functions. Proof of no behavior change: the pre-existing transmission_task_handler_test.go (which pins the exact Publish(&ReplicationTask{...}) payload via gomock) is unchanged and still green. Adds conversion_test.go (direct unit tests for both extracted functions, including the NORMAL-only replication-State special case) and 3 gate cases (ForceReplicate_BypassesGate, ForceReplicate_DeletedNotReplicated, DeletedNotReplicated) covering the reordered force/deleted/gate branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…n's field-access style Two review-follow-up tweaks on the shared-converter extraction, both no behavior change: - Fold forceReplicate into ShouldReplicateNamespace with the DELETED check ahead of it, so force can never bypass the never-replicate-deleted rule. Previously the gate was called as `!forceReplicate && !ShouldReplicate...`, which let a caller bypass the gate's DELETED semantics under force; the committed code masked that with a separate top-level DELETED guard. Making the single shared gate own the whole force/deleted/global/peer decision is the point of the extraction: the eventual CHASM transport shares this exact function and cannot accidentally drop part of the decision. - Revert the getters the extraction introduced (config.GetRetention() etc.) back to direct field access (config.Retention) so NamespaceDetailToTaskAttributes reads line-for-line like the original inline block on main and the diff is a pure move, not a style change. All four force/deleted/gate outcomes are identical to main and to the prep commit; the gomock payload test is unchanged and green. golangci-lint (pinned v2.9.0, --new-from-rev=main) reports 0 issues. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The reflective, schema-coupled tests conflict with repository simplicity guidance and should be streamlined.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Extracts reusable namespace-replication gating and payload conversion while preserving legacy behavior.

Changes:

Centralizes replication eligibility decisions.
Adds shared persistence-to-wire conversion.
Adds gate and conversion coverage.

File summaries

File	Description
`transmission_task_handler.go`	Uses shared replication helpers.
`transmission_task_handler_test.go`	Covers force and deletion behavior.
`conversion_test.go`	Tests gating and payload conversion.

Review details

Files reviewed: 3/3 changed files
Comments generated: 2
Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🔵 Needs a closer look

The new gate changes legacy forced-replication behavior for deleted namespaces.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

common/namespace/nsreplication/transmission_task_handler.go:166

Details

med — Forced replication no longer preserves legacy behavior for deleted namespaces.

Before this extraction, the deleted-state check was nested inside if !forceReplicate, so forceReplicate=true always reached Publish. Checking deletion first now skips the force-plus-deleted case, contradicting the PR's no-behavior-change contract and changing the exported replicator's semantics even though today's frontend callers pass false.

Suggestion: Check forceReplicate before the deleted state, then update the new tests and comments to assert the legacy ordering.
common/namespace/nsreplication/conversion_test.go:109

Details

small — Independent table cases should run in parallel.

These cases have no shared mutable state, so running them sequentially conflicts with the project's test convention for plain t.Run cases.

Suggestion: Start each subtest with t.Parallel().

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Balanced

Copilot

Copilot review overview

🟢 Approval recommended

Legacy behavior is preserved; comments identify only non-blocking test-style improvements.

Review effort: Balanced
Findings: 1

Open (1)

Compare the converter's complete proto result · New

Previously missed (1)

In code that hasn't changed since last review

Run independent table-driven cases in parallel

common/namespace/nsreplication/conversion_test.go:109

small — Run these independent table cases in parallel.

These pure cases share no mutable state, so serializing every subtest violates the project's testing convention and needlessly lengthens the suite.

Suggestion: Mark each subtest parallel.

+	require.Same(t, info, detail.GetInfo())
+	require.Same(t, config, detail.GetConfig())
+	require.Equal(t, "active", detail.GetReplicationConfig().GetActiveClusterName())
+	require.Equal(t, enumspb.REPLICATION_STATE_HANDOVER, detail.GetReplicationConfig().GetState())
+	require.Equal(t, []string{"active", "standby"}, detail.GetReplicationConfig().GetClusters())
+	require.Equal(t, failoverHistory, detail.GetReplicationConfig().GetFailoverHistory())
+	require.Equal(t, int64(3), detail.GetConfigVersion())
+	require.Equal(t, int64(4), detail.GetFailoverVersion())


qyc5937 and others added 2 commits September 16, 2026 13:34

qyc5937 mentioned this pull request Sep 16, 2026

namespace replication: implement inert CHASM library (PR1b) #12113

Draft

qyc5937 closed this Sep 16, 2026

qyc5937 reopened this Sep 17, 2026

namespace replication: strengthen converter and gate tests

09a9e74

qyc5937 requested a balanced review from Copilot September 17, 2026 23:04

Copilot started reviewing on behalf of qyc5937 September 17, 2026 23:05 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread common/namespace/nsreplication/conversion_test.go Outdated

Comment thread common/namespace/nsreplication/conversion_test.go Outdated

namespace replication: simplify converter tests

840739d

qyc5937 requested a balanced review from Copilot September 18, 2026 15:51

Copilot started reviewing on behalf of qyc5937 September 18, 2026 15:52 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

namespace replication: extract legacy detail builder

b8033d5

qyc5937 requested a balanced review from Copilot September 18, 2026 22:21

Copilot started reviewing on behalf of qyc5937 September 18, 2026 22:21 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Navigation Menu

namespace replication: extract shared replication helpers (PR0) - #12112

namespace replication: extract shared replication helpers (PR0)#12112
qyc5937 wants to merge 5 commits into
mainfrom
prep/nsrepl-shared-converter

qyc5937 commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

qyc5937 commented Sep 16, 2026

Summary

Tests

Operational impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants