-
Notifications
You must be signed in to change notification settings - Fork 840
[domain-deletion]Add handler to process delete domain replication task #6918
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
[domain-deletion]Add handler to process delete domain replication task #6918
Conversation
This is also an IDL dependency update, can you link related the IDL commit as well? |
sure, here are the links: cadence-workflow/cadence-idl#200 I will update the summary as well |
common/domain/handler.go
Outdated
getResponse.PreviousFailoverVersion, | ||
isGlobalDomain, | ||
); err != nil { | ||
return err |
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.
wrap error with additional information warning that domain in replicas was not deleted correctly.
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.
Thank you, good point! Updated the PR
@@ -99,6 +100,8 @@ func (h *domainReplicationTaskExecutorImpl) Execute(task *types.DomainTaskAttrib | |||
return h.handleDomainCreationReplicationTask(ctx, task) | |||
case types.DomainOperationUpdate: | |||
return h.handleDomainUpdateReplicationTask(ctx, task) | |||
case types.DomainOperationDelete: |
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.
Is there a forward compatibility issue we should worry about?
e.g. Cluster A has this change but cluster B is not deployed yet. Cluster A starts generating DELETE tasks which get replicated to cluster B. Cluster B doesn't recognize it and falls into default case below. In that case does the task get discarded, tried forever or put into dlq?
If such cases would cause hard-to-recover situations, let's split idl changes and its usage into separate commits/prereleases.
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.
Great point! Thank you!
To address these concerns, I've introduced a feature flag that will be enabled when the changes have been rolled out in active and replica clusters as well. Here is the link - #6920
Please let me know what you think
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.
I think this is a great way to safely rollout this change. Thanks
What changed?
This PR introduces the
handleDomainDeleteReplicationTask
handler. It is responsible for processing replication tasks specifically related to domain deletions.Related IDL changes: cadence-workflow/cadence-idl#200
cadence-workflow/cadence-idl#201
Why?
Currently, domain deletions are not consistently propagated across all replicas, leading to potential inconsistencies. This task handler ensures that domain delete operations are reliably replicated.
How did you test it?
Unit tests & local testing
Potential risks
Release notes
Documentation Changes