Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Utilize shard clock for workflow / activity task #2744

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

wxing1292
Copy link
Contributor

What changed?

  • Add shard clock util
  • Add shard clock to shard context
  • Use shard clock for activity / workflow task

Why?
see #2743

How did you test it?
Unit tests

Potential risks
N/A

Is hotfix candidate?
N/A

@@ -66,6 +67,8 @@ type (
GetEngine() (Engine, error)
GetEngineWithContext(ctx context.Context) (Engine, error)

GetVClock() *clockpb.ShardClock
Copy link
Member

Choose a reason for hiding this comment

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

Replace VClock with ShardClock or VectorClock.

@@ -193,6 +195,14 @@ func (s *ContextImpl) GetMaxTaskIDForCurrentRangeID() int64 {
return s.maxTaskSequenceNumber - 1
}

func (s *ContextImpl) GetVClock() *clockpb.ShardClock {
s.rLock()
clock := s.taskSequenceNumber
Copy link
Member

Choose a reason for hiding this comment

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

Is it guaranteed that any update to any workflow (in the shard) will cause this to increment? For example, if there is already a workflow task scheduled (but not started), and there comes a new signal, will the new signal cause the taskSequenceNumber to increase?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe that doesn't matter since this is mainly to detect stale shard (which happen only if someone steal it) in that case, the clock must have moved forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it guaranteed that any update to any workflow (in the shard) will cause this to increment? For example, if there is already a workflow task scheduled (but not started), and there comes a new signal, will the new signal cause the taskSequenceNumber to increase?

yes, newly generated history event will use this sequence number

this GetVClock is a read method, if business logic need to stamp an event, then there should be a generate method

* Add shard clock util
* Add shard clock to shard context
* Use shard clock for activity / workflow task
@wxing1292 wxing1292 marked this pull request as ready for review April 20, 2022 05:50
@wxing1292 wxing1292 requested a review from a team as a code owner April 20, 2022 05:50
@@ -150,6 +152,7 @@ func (t *transferQueueTaskExecutorBase) pushWorkflowTask(
TaskQueue: taskqueue,
ScheduleId: task.ScheduleID,
ScheduleToStartTimeout: workflowTaskScheduleToStartTimeout,
Clock: vclock.NewShardClock(t.shard.GetShardID(), task.TaskID),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yiminc e.g. logic is using the task ID (generated from the sequence ID) as shard clock stamp

@wxing1292 wxing1292 enabled auto-merge (squash) April 21, 2022 18:41
@wxing1292 wxing1292 merged commit 58456a4 into temporalio:master Apr 21, 2022
@wxing1292 wxing1292 deleted the citadel-1-matching-task branch April 21, 2022 19:11
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.

None yet

2 participants