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

[history] add domain status check in taskfilter #5140

Merged

Conversation

shijiesheng
Copy link
Member

@shijiesheng shijiesheng commented Mar 8, 2023

What changed?

  • add domain status check in taskFilter

Why?

  • tasks in queues should drop for deprecated or deleted domains

How did you test it?

Potential risks

Release notes

Documentation Changes

@shijiesheng shijiesheng requested a review from Shaddoll March 8, 2023 19:48
@shijiesheng shijiesheng force-pushed the taskallocator-refactor branch from a74cb9a to 4a3bde3 Compare March 8, 2023 19:51
@shijiesheng shijiesheng changed the title [history] refactor taskfilter logic in transfer and timer task queue processor [history] add domain status check in taskfilter Mar 8, 2023
@coveralls
Copy link

coveralls commented Mar 8, 2023

Pull Request Test Coverage Report for Build 0186c818-1bf4-43a1-b48b-980f5dbeabd9

  • 10 of 36 (27.78%) changed or added relevant lines in 3 files are covered.
  • 93 unchanged lines in 17 files lost coverage.
  • Overall coverage increased (+0.002%) to 57.055%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/history/queue/task_allocator.go 6 12 50.0%
service/history/queue/timer_queue_processor.go 2 12 16.67%
service/history/queue/transfer_queue_processor.go 2 12 16.67%
Files with Coverage Reduction New Missed Lines %
service/history/queue/timer_queue_processor.go 1 57.57%
service/history/queue/transfer_queue_processor.go 1 56.06%
common/membership/hashring.go 2 83.54%
common/task/weightedRoundRobinTaskScheduler.go 2 88.6%
common/util.go 2 52.31%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go 3 59.55%
common/persistence/statsComputer.go 3 94.64%
common/task/fifoTaskScheduler.go 3 84.54%
service/history/task/fetcher.go 3 91.75%
service/matching/taskListManager.go 3 81.15%
Totals Coverage Status
Change from base Build 0186c7a5-3214-49a7-8dd7-893ecc8326ee: 0.002%
Covered Lines: 85203
Relevant Lines: 149335

💛 - Coveralls

@@ -514,24 +514,6 @@ func newTransferQueueStandbyProcessor(
if !ok {
return false, errUnexpectedQueueTask
}
if task.TaskType == persistence.TransferTaskTypeCloseExecution ||
Copy link
Member

Choose a reason for hiding this comment

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

These logic was put here on purpose, because we don't want to hide it

@shijiesheng shijiesheng force-pushed the taskallocator-refactor branch from 900d563 to c9866ae Compare March 9, 2023 19:19
@shijiesheng shijiesheng enabled auto-merge (squash) March 9, 2023 19:31
@shijiesheng shijiesheng merged commit 4b5e888 into cadence-workflow:master Mar 9, 2023
domainEntry, err := shard.GetDomainCache().GetDomainByID(domainID)
if err != nil {
if _, ok := err.(*types.EntityNotExistsError); ok {
return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be more conservative here? Normally we never delete domains

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have a timer and we cannot find the domain, something must be wrong.

@@ -189,3 +191,20 @@ func (t *taskAllocatorImpl) Lock() {
func (t *taskAllocatorImpl) Unlock() {
t.locker.Unlock()
}

// isDomainNotRegistered checks either if domain does not exist or is in deprecated or deleted status
func isDomainNotRegistered(shard shard.Context, domainID string) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this to isDomainDeprecated

Groxx pushed a commit that referenced this pull request Mar 17, 2023
What changed?

add domain status check in taskAllocator verification

Why?

tasks in queues should drop for deprecated or deleted domains
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.

4 participants