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
Add tdbg dlq merge for v2 #5025
Conversation
265942d
to
4d9e4b1
Compare
344d55d
to
c20f37f
Compare
response, err := adminClient.MergeDLQTasks(ctx, &adminservice.MergeDLQTasksRequest{ | ||
DlqKey: ac.getDLQKey(), | ||
InclusiveMaxTaskMetadata: &commonspb.HistoryDLQTaskMetadata{ | ||
MessageId: lastMessageID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm I can specify MaxInt64 here if I want all tasks merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. I added that as the default behavior behind a confirmation flag. I also extracted the prompter logic into an object and added unit tests for it. Lastly, I added documentation for the behavior of the last-message-id flag.
service/frontend/admin_handler.go
Outdated
return fmt.Sprintf("manage-dlq-tasks-%s", key.GetQueueName()) | ||
} | ||
|
||
func (adh *AdminHandler) parseKey(key *commonspb.HistoryDLQKey) (*persistence.QueueKey, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: parseDLQKey
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes more sense.
service/worker/dlq/workflow.go
Outdated
return err | ||
} | ||
ctx = workflow.WithActivityOptions(ctx, workflow.ActivityOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we add a retry policy for those activities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, please let me know if you think this retry policy is reasonable.
4d9e4b1
to
930c529
Compare
132d45b
to
9867700
Compare
9867700
to
dff5f14
Compare
930c529
to
2da103b
Compare
ba51661
to
d546ba9
Compare
d546ba9
to
222d411
Compare
What changed?
This PR adds support for
--dlq-version v2
to thetdbg dlq merge
command.Why?
Admins need this to be able to re-enqueue tasks.
How did you test it?
I added an end-to-end test with a real workflow which does this:
I also added various unit tests for parameter validation and control flow to get 100% test coverage.
Note that this won't work for cross-cluster tasks yet. I will add that and update the history replication DLQ integration test as well in a follow-up.
Potential risks
Is hotfix candidate?