Skip to content

Commit eeeff72

Browse files
authored
fix(featureflags): use proto entity and proto json serializer (#7708)
<!-- 1-2 line summary of WHAT changed technically: - Always link the relevant projects GitHub issue, unless it is a minor bugfix - Good: "Modified FailoverDomain mapper to allow null ActiveClusterName #320" - Bad: "added nil check" --> **What changed?** #7707 Use proto entity of `FeatureFlags` instead so it's consistent across languages. It's backward/forward compatible because the proto json marshaler returns the same result with thrift IDL by cadence-workflow/cadence-idl#245 <!-- Your goal is to provide all the required context for a future maintainer to understand the reasons for making this change (see https://cbea.ms/git-commit/#why-not-how). How did this work previously (and what was wrong with it)? What has changed, and why did you solve it this way? - Good: "Active-active domains have independent cluster attributes per region. Previously, modifying cluster attributes required spedifying the default ActiveClusterName which updates the global domain default. This prevents operators from updating regional configurations without affecting the primary cluster designation. This change allows attribute updates to be independent of active cluster selection." - Bad: "Improves domain handling" --> **Why?** Proto has better language consistency than thrift. <!-- Include specific test commands and setup. Please include the exact commands such that another maintainer or contributor can reproduce the test steps taken. - e.g Unit test commands with exact invocation `go test -v ./common/types/mapper/proto -run TestFailoverDomainRequest` - For integration tests include setup steps and test commands Example: "Started local server with `./cadence start`, then ran `make test_e2e`" - For local simulation testing include setup steps for the server and how you ran the tests - Good: Full commands that reviewers can copy-paste to verify - Bad: "Tested locally" or "Added tests" --> **How did you test it?** Added Unit Test to make sure the thrift marshaled json string can be deserialized into Proto entity. <!-- If there are risks that the release engineer should know about document them here. For example: - Has an API/IDL been modified? Is it backwards/forwards compatible? If not, what are the repecussions? - Has a schema change been introduced? Is it possible to roll back? - Has a feature flag been re-used for a new purpose? - Is there a potential performance concern? Is the change modifying core task processing logic? - If truly N/A, you can mark it as such --> **Potential risks** <!-- If this PR completes a user facing feature or changes functionality add release notes here. Your release notes should allow a user and the release engineer to understand the changes with little context. Always ensure that the description contains a link to the relevant GitHub issue. --> **Release notes** <!-- Consider whether this change requires documentation updates in the Cadence-Docs repo - If yes: mention what needs updating (or link to docs PR in cadence-docs repo) - If in doubt, add a note about potential doc needs - Only mark N/A if you're certain no docs are affected --> **Documentation Changes** --- ## Reviewer Validation **PR Description Quality** (check these before reviewing code): - [ ] **"What changed"** provides a clear 1-2 line summary - [ ] Project Issue is linked - [ ] **"Why"** explains the full motivation with sufficient context - [ ] **Testing is documented:** - [ ] Unit test commands are included (with exact `go test` invocation) - [ ] Integration test setup/commands included (if integration tests were run) - [ ] Canary testing details included (if canary was mentioned) - [ ] **Potential risks** section is thoughtfully filled out (or legitimately N/A) - [ ] **Release notes** included if this completes a user-facing feature - [ ] **Documentation** needs are addressed (or noted if uncertain) --------- Signed-off-by: Shijie Sheng <liouvetren@gmail.com>
1 parent b140a12 commit eeeff72

File tree

13 files changed

+169
-108
lines changed

13 files changed

+169
-108
lines changed

.gen/go/shared/shared.go

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.gen/proto/history/v1/service.pb.yarpc.go

Lines changed: 28 additions & 28 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.gen/proto/matching/v1/service.pb.yarpc.go

Lines changed: 28 additions & 28 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

cmd/server/go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ require (
4040
github.com/startreedata/pinot-client-go v0.2.0 // latest release supports pinot v0.12.0 which is also internal version
4141
github.com/stretchr/testify v1.10.0
4242
github.com/uber-go/tally v3.3.15+incompatible
43-
github.com/uber/cadence-idl v0.0.0-20260115163036-f68403083e26
43+
github.com/uber/cadence-idl v0.0.0-20260213213208-6fb822717d01
4444
github.com/uber/ringpop-go v0.8.5 // indirect
4545
github.com/uber/tchannel-go v1.22.2 // indirect
4646
github.com/valyala/fastjson v1.4.1 // indirect

cmd/server/go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -414,8 +414,8 @@ github.com/uber-go/tally v3.3.12+incompatible/go.mod h1:YDTIBxdXyOU/sCWilKB4bgyu
414414
github.com/uber-go/tally v3.3.15+incompatible h1:9hLSgNBP28CjIaDmAuRTq9qV+UZY+9PcvAkXO4nNMwg=
415415
github.com/uber-go/tally v3.3.15+incompatible/go.mod h1:YDTIBxdXyOU/sCWilKB4bgyufu1cEi0jdVnRdxvjnmU=
416416
github.com/uber/cadence-idl v0.0.0-20211111101836-d6b70b60eb8c/go.mod h1:oyUK7GCNCRHCCyWyzifSzXpVrRYVBbAMHAzF5dXiKws=
417-
github.com/uber/cadence-idl v0.0.0-20260115163036-f68403083e26 h1:ayljsfgiQNLzoA1Bn29LyRPhFdU9gPKq/1zCUjC8cHE=
418-
github.com/uber/cadence-idl v0.0.0-20260115163036-f68403083e26/go.mod h1:oyUK7GCNCRHCCyWyzifSzXpVrRYVBbAMHAzF5dXiKws=
417+
github.com/uber/cadence-idl v0.0.0-20260213213208-6fb822717d01 h1:qHVlPoKvvtivJoSqQiQr6fBGK+SBjiApbM8V+Iw+VRU=
418+
github.com/uber/cadence-idl v0.0.0-20260213213208-6fb822717d01/go.mod h1:oyUK7GCNCRHCCyWyzifSzXpVrRYVBbAMHAzF5dXiKws=
419419
github.com/uber/jaeger-client-go v2.22.1+incompatible h1:NHcubEkVbahf9t3p75TOCR83gdUHXjRJvjoBh1yACsM=
420420
github.com/uber/jaeger-client-go v2.22.1+incompatible/go.mod h1:WVhlPFC8FDjOFMMWRy2pZqQJSXxYSwNYOkTr/Z6d3Kk=
421421
github.com/uber/jaeger-lib v2.2.0+incompatible h1:MxZXOiR2JuoANZ3J6DE/U0kSFv/eJ/GfSYVCjK7dyaw=

common/client/versionChecker.go

Lines changed: 21 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,11 @@ package client
2424

2525
import (
2626
"context"
27-
"encoding/json"
2827
"fmt"
2928

29+
"github.com/gogo/protobuf/jsonpb"
3030
"github.com/hashicorp/go-version"
31-
"go.uber.org/cadence/client"
31+
apiv1 "github.com/uber/cadence-idl/go/proto/api/v1"
3232
"go.uber.org/yarpc"
3333

3434
"github.com/uber/cadence/.gen/go/shared"
@@ -86,8 +86,8 @@ var (
8686
ErrUnknownFeature = &types.BadRequestError{Message: "Unknown feature"}
8787

8888
// DefaultCLIFeatureFlags is the default FeatureFlags used by Cadence CLI
89-
DefaultCLIFeatureFlags = shared.FeatureFlags{
90-
WorkflowExecutionAlreadyCompletedErrorEnabled: common.BoolPtr(true),
89+
DefaultCLIFeatureFlags = apiv1.FeatureFlags{
90+
WorkflowExecutionAlreadyCompletedErrorEnabled: true,
9191
}
9292
)
9393

@@ -99,7 +99,7 @@ type (
9999
SupportsStickyQuery(clientImpl string, clientFeatureVersion string) error
100100
SupportsConsistentQuery(clientImpl string, clientFeatureVersion string) error
101101
SupportsRawHistoryQuery(clientImpl string, clientFeatureVersion string) error
102-
SupportsWorkflowAlreadyCompletedError(clientImpl string, clientFeatureVersion string, featureFlags shared.FeatureFlags) error
102+
SupportsWorkflowAlreadyCompletedError(clientImpl string, clientFeatureVersion string, featureFlags apiv1.FeatureFlags) error
103103
}
104104

105105
versionChecker struct {
@@ -109,39 +109,29 @@ type (
109109
}
110110
)
111111

112-
// ToClientFeatureFlags returns Cadence client FeatureFlags version of idl.FeatureFlags
113-
func ToClientFeatureFlags(featureFlags *shared.FeatureFlags) client.FeatureFlags {
114-
flags := client.FeatureFlags{}
115-
if featureFlags != nil {
116-
if featureFlags.WorkflowExecutionAlreadyCompletedErrorEnabled != nil {
117-
flags.WorkflowExecutionAlreadyCompletedErrorEnabled = *featureFlags.WorkflowExecutionAlreadyCompletedErrorEnabled
118-
}
119-
}
120-
return flags
121-
}
122-
123112
// FeatureFlagsHeader returns the serialized version of the FeatureFlags
124-
func FeatureFlagsHeader(featureFlags shared.FeatureFlags) string {
125-
serialized := ""
126-
buf, err := json.Marshal(featureFlags)
127-
if err == nil {
128-
serialized = string(buf)
113+
func FeatureFlagsHeader(featureFlags apiv1.FeatureFlags) string {
114+
var marshaler jsonpb.Marshaler
115+
serialized, err := marshaler.MarshalToString(&featureFlags)
116+
if err != nil {
117+
// fail open and return empty feature flags
118+
return ""
129119
}
130120
return serialized
131121
}
132122

133123
// GetFeatureFlagsFromHeader returns FeatureFlags from yarpc headers
134-
func GetFeatureFlagsFromHeader(call *yarpc.Call) shared.FeatureFlags {
124+
func GetFeatureFlagsFromHeader(call *yarpc.Call) apiv1.FeatureFlags {
135125
featureFlagsSerialized := call.Header(common.ClientFeatureFlagsHeaderName)
136126

137-
if len(featureFlagsSerialized) > 0 {
138-
featureFlags := shared.FeatureFlags{}
139-
errSerialize := json.Unmarshal([]byte(featureFlagsSerialized), &featureFlags)
140-
if errSerialize == nil {
141-
return featureFlags
142-
}
127+
var featureFlags apiv1.FeatureFlags
128+
err := jsonpb.UnmarshalString(featureFlagsSerialized, &featureFlags)
129+
if err != nil {
130+
// fail open and return empty feature flags
131+
return apiv1.FeatureFlags{}
143132
}
144-
return shared.FeatureFlags{}
133+
134+
return featureFlags
145135
}
146136

147137
// NewVersionChecker constructs a new VersionChecker
@@ -224,8 +214,8 @@ func (vc *versionChecker) SupportsRawHistoryQuery(clientImpl string, clientFeatu
224214
// Returns error if workflowAlreadyCompletedError is not supported otherwise nil.
225215
// In case client version lookup fails assume the client does not support feature.
226216
// NOTE: Enabling this error will break customer code handling the workflow errors in their workflow
227-
func (vc *versionChecker) SupportsWorkflowAlreadyCompletedError(clientImpl string, clientFeatureVersion string, featureFlags shared.FeatureFlags) error {
228-
if featureFlags.WorkflowExecutionAlreadyCompletedErrorEnabled != nil && *featureFlags.WorkflowExecutionAlreadyCompletedErrorEnabled {
217+
func (vc *versionChecker) SupportsWorkflowAlreadyCompletedError(clientImpl string, clientFeatureVersion string, featureFlags apiv1.FeatureFlags) error {
218+
if featureFlags.WorkflowExecutionAlreadyCompletedErrorEnabled {
229219
return vc.featureSupported(clientImpl, clientFeatureVersion, workflowAlreadyCompletedError)
230220
}
231221
return &shared.FeatureNotEnabledError{FeatureFlag: "WorkflowExecutionAlreadyCompletedErrorEnabled"}

common/client/versionChecker_mock.go

Lines changed: 2 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)