Skip to content

Commit 5db0ac2

Browse files
committed
remove fastForwardNotifier from context provider and simiplify the hash
function
1 parent acd5af3 commit 5db0ac2

8 files changed

Lines changed: 149 additions & 32 deletions

File tree

common/testing/mocksdk/client_mock.go

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

service/history/api/polltimeskipping/api_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ func TestInvoke(t *testing.T) {
221221
ctrl := gomock.NewController(t)
222222
// not running, but NewExecutionRunId set (retry/cron/CaN) => not "closed"; polls and times out.
223223
checker := mockConsistencyChecker{lease: mockLease{ms: mutableState(ctrl, fastForwardTSI(testFastForwardID, false), false, "next-run")}}
224-
ffNotifier := notification.NewTimeSkippingFastForwardNotifier(func(namespace.ID, string) int32 { return 1 })
224+
ffNotifier := notification.NewTimeSkippingFastForwardNotifier()
225225
resp, err := Invoke(context.Background(), pollReq(uuid.NewString(), testWorkflowID, testFastForwardID),
226226
shardContext(ctrl, 20*time.Millisecond), checker, ffNotifier)
227227
require.NoError(t, err)
@@ -232,7 +232,7 @@ func TestInvoke(t *testing.T) {
232232
ctrl := gomock.NewController(t)
233233
checker := mockConsistencyChecker{lease: mockLease{ms: mutableState(ctrl, fastForwardTSI(testFastForwardID, false), true, "")}}
234234
// Real notifier: never notified, so the wait blocks until the soft timeout.
235-
ffNotifier := notification.NewTimeSkippingFastForwardNotifier(func(namespace.ID, string) int32 { return 1 })
235+
ffNotifier := notification.NewTimeSkippingFastForwardNotifier()
236236
resp, err := Invoke(context.Background(), pollReq(uuid.NewString(), testWorkflowID, testFastForwardID),
237237
shardContext(ctrl, 20*time.Millisecond), checker, ffNotifier)
238238
require.NoError(t, err)

service/history/fx.go

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ import (
4747
"go.temporal.io/server/service/history/consts"
4848
"go.temporal.io/server/service/history/events"
4949
"go.temporal.io/server/service/history/hsm"
50-
"go.temporal.io/server/service/history/notification"
5150
"go.temporal.io/server/service/history/replication"
5251
"go.temporal.io/server/service/history/shard"
5352
"go.temporal.io/server/service/history/workflow"
@@ -93,7 +92,6 @@ var Module = fx.Options(
9392
service.PersistenceLazyLoadedServiceResolverModule,
9493
fx.Provide(ServiceResolverProvider),
9594
fx.Provide(EventNotifierProvider),
96-
fx.Provide(TimeSkippingFastForwardNotifierProvider),
9795
fx.Provide(HistoryEngineFactoryProvider),
9896
fx.Provide(HandlerProvider),
9997
fx.Provide(HistoryServiceServerProvider),
@@ -484,12 +482,6 @@ func EventNotifierProvider(
484482
)
485483
}
486484

487-
func TimeSkippingFastForwardNotifierProvider(
488-
serviceConfig *configs.Config,
489-
) notification.TimeSkippingFastForwardNotifier {
490-
return notification.NewTimeSkippingFastForwardNotifier(serviceConfig.GetShardID)
491-
}
492-
493485
func ServiceLifetimeHooks(lc fx.Lifecycle, svc *Service) {
494486
lc.Append(fx.StartStopHook(svc.Start, svc.Stop))
495487
}

service/history/history_engine.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,6 @@ func NewEngineWithShardContext(
161161
matchingClient matchingservice.MatchingServiceClient,
162162
sdkClientFactory sdk.ClientFactory,
163163
eventNotifier events.Notifier,
164-
fastForwardNotifier notification.TimeSkippingFastForwardNotifier,
165164
config *configs.Config,
166165
versionCache worker_versioning.VersionMembershipAndReactivationStatusCache,
167166
workerDeploymentClient workerdeployment.Client,
@@ -218,7 +217,7 @@ func NewEngineWithShardContext(
218217
throttledLogger: log.With(shard.GetThrottledLogger(), tag.ComponentHistoryEngine),
219218
metricsHandler: shard.GetMetricsHandler(),
220219
eventNotifier: eventNotifier,
221-
fastForwardNotifier: fastForwardNotifier,
220+
fastForwardNotifier: notification.NewTimeSkippingFastForwardNotifier(),
222221
config: config,
223222
sdkClientFactory: sdkClientFactory,
224223
matchingClient: matchingClient,
@@ -895,7 +894,7 @@ func (e *historyEngineImpl) NotifyFastForwardUpdate(
895894
fastforwardNotification *notification.TimeSkippingFastForwardNotification,
896895
) {
897896
if e.fastForwardNotifier == nil {
898-
// Always injected in production via fx; a nil here means a misconfigured engine.
897+
// Always set by NewEngineWithShardContext; a nil here means a hand-built engine.
899898
// Fast-forward notification is best-effort (waiters re-poll on timeout), so log and
900899
// skip rather than panic.
901900
e.logger.Warn("fastForwardNotifier is not configured; skipping fast-forward notification",

service/history/history_engine_factory.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"go.temporal.io/server/service/history/configs"
1818
"go.temporal.io/server/service/history/events"
1919
historyi "go.temporal.io/server/service/history/interfaces"
20-
"go.temporal.io/server/service/history/notification"
2120
"go.temporal.io/server/service/history/replication"
2221
"go.temporal.io/server/service/history/tasks"
2322
"go.temporal.io/server/service/history/workflow"
@@ -34,7 +33,6 @@ type (
3433
MatchingClient resource.MatchingClient
3534
SdkClientFactory sdk.ClientFactory
3635
EventNotifier events.Notifier
37-
TimeSkippingFastForwardNotifier notification.TimeSkippingFastForwardNotifier
3836
Config *configs.Config
3937
RawMatchingClient resource.MatchingRawClient
4038
WorkflowCache wcache.Cache
@@ -73,7 +71,6 @@ func (f *historyEngineFactory) CreateEngine(
7371
f.MatchingClient,
7472
f.SdkClientFactory,
7573
f.EventNotifier,
76-
f.TimeSkippingFastForwardNotifier,
7774
f.Config,
7875
f.VersionMembershipCache,
7976
f.WorkerDeploymentClient,

service/history/notification/notifier.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,8 @@ func (noopNotifier[K, T]) Unwatch(K, string) error { return nil }
5656
// concurrent waiters per key; Watch beyond that returns a ResourceExhausted error.
5757
//
5858
// hashKey only stripes the internal map's locks for concurrency — any uniform hash of the
59-
// key works and it has no history-shard meaning here (callers just pass the workflow->shard
60-
// hash as a convenient, well-distributed one). It need not cover every field of K: keys are
61-
// matched by equality, so a hash over a subset only affects lock distribution.
59+
// key works and it carries no history-shard meaning here. It need not cover every field of K:
60+
// keys are matched by equality, so a hash over a subset only affects lock distribution.
6261
func NewPubSubNotifier[K comparable, T any](hashKey func(K) uint32, maxSubscribersPerKey int) PubSubNotifier[K, T] {
6362
if hashKey == nil {
6463
// A caller bug, but not a fatal one, and better caught here than as a nil-func panic on

service/history/notification/timeskipping_notifier.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package notification
22

33
import (
4+
"strconv"
5+
6+
"github.com/dgryski/go-farm"
47
persistencespb "go.temporal.io/server/api/persistence/v1"
58
"go.temporal.io/server/chasm"
6-
"go.temporal.io/server/common/namespace"
79
)
810

911
// TimeSkippingFastForwardNotification is the payload delivered to subscribers waiting for
@@ -78,13 +80,17 @@ func NewTimeSkippingNotificationKey(
7880

7981
const maxFastForwardWaitersPerExecution = 5
8082

81-
func NewTimeSkippingFastForwardNotifier(hashKey func(namespace.ID, string) int32) TimeSkippingFastForwardNotifier {
83+
func NewTimeSkippingFastForwardNotifier() TimeSkippingFastForwardNotifier {
8284
return NewPubSubNotifier[TimeSkippingNotificationKey, *TimeSkippingFastForwardNotification](
83-
func(key TimeSkippingNotificationKey) uint32 {
84-
return uint32(hashKey(namespace.ID(key.NamespaceID), key.WorkflowID))
85-
},
85+
hashTimeSkippingNotificationKey,
8686
maxFastForwardWaitersPerExecution,
8787
)
8888
}
8989

90+
func hashTimeSkippingNotificationKey(key TimeSkippingNotificationKey) uint32 {
91+
return farm.Fingerprint32([]byte(
92+
key.NamespaceID + "_" + key.WorkflowID + "_" + strconv.FormatUint(uint64(key.ArchetypeID), 10),
93+
))
94+
}
95+
9096
var NoopTimeSkippingFastForwardNotifier = NewNoopNotifier[TimeSkippingNotificationKey, *TimeSkippingFastForwardNotification]()

service/history/notification/timeskipping_notifier_test.go

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,11 @@ import (
55

66
"github.com/stretchr/testify/require"
77
"go.temporal.io/server/chasm"
8-
"go.temporal.io/server/common/namespace"
98
)
109

1110
func TestTimeSkippingFastForwardNotifier(t *testing.T) {
12-
// The notifier's hash covers only (namespace, workflowID), so keys differing solely by
13-
// archetype land on the same lock shard and stay distinct by struct equality alone.
14-
oneShardByWorkflow := func(namespace.ID, string) int32 { return 1 }
15-
1611
t.Run("archetype isolates executions sharing a business id", func(t *testing.T) {
17-
n := NewTimeSkippingFastForwardNotifier(oneShardByWorkflow)
12+
n := NewTimeSkippingFastForwardNotifier()
1813
workflowKey := NewTimeSkippingNotificationKey("ns", "wf", chasm.WorkflowArchetypeID)
1914
schedulerKey := NewTimeSkippingNotificationKey("ns", "wf", chasm.SchedulerArchetypeID)
2015

@@ -38,7 +33,7 @@ func TestTimeSkippingFastForwardNotifier(t *testing.T) {
3833
// The waiter always names the workflow archetype while the publisher forwards whatever the
3934
// execution carries, so a pre-archetype record (unspecified) must reach the workflow waiter.
4035
t.Run("unspecified archetype is normalized to workflow", func(t *testing.T) {
41-
n := NewTimeSkippingFastForwardNotifier(oneShardByWorkflow)
36+
n := NewTimeSkippingFastForwardNotifier()
4237
require.Equal(t,
4338
NewTimeSkippingNotificationKey("ns", "wf", chasm.WorkflowArchetypeID),
4439
NewTimeSkippingNotificationKey("ns", "wf", chasm.UnspecifiedArchetypeID))
@@ -71,7 +66,7 @@ func TestTimeSkippingFastForwardNotifier(t *testing.T) {
7166
})
7267

7368
t.Run("keys match by value, not identity", func(t *testing.T) {
74-
n := NewTimeSkippingFastForwardNotifier(oneShardByWorkflow)
69+
n := NewTimeSkippingFastForwardNotifier()
7570
_, ch, err := n.Watch(NewTimeSkippingNotificationKey("ns", "wf", chasm.WorkflowArchetypeID))
7671
require.NoError(t, err)
7772

0 commit comments

Comments
 (0)