Skip to content

Unblock pending poll when passive cluster becomes active #8506

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Unblock pending poll when passive cluster becomes active #8506

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion service/matching/physical_task_queue_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/nexus-rpc/sdk-go/nexus"
"github.com/pborman/uuid"
"go.temporal.io/api/serviceerror"
taskqueuepb "go.temporal.io/api/taskqueue/v1"
"go.temporal.io/api/workflowservice/v1"
Expand Down Expand Up @@ -356,7 +357,20 @@ func (c *physicalTaskQueueManagerImpl) PollTask(
}

if !namespaceEntry.ActiveInCluster(c.clusterMeta.GetCurrentClusterName()) {
return c.matcher.PollForQuery(ctx, pollMetadata)
ctx, cancel := context.WithCancel(ctx)
key := uuid.New()
// Listening to registry changes in case the cluster becomes active while the poll is waiting
c.namespaceRegistry.RegisterStateChangeCallback(key, func(ns *namespace.Namespace, deletedFromDb bool) {
Copy link
Member

Choose a reason for hiding this comment

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

instead of doing it here, I think we should register the namespace state change listener at task queue level, when it sees the state change, it would unload the task queue and reload it.
this is hot path that the overhead here could be too heavy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm... yeah if we normally expect polls on the passive side what you're suggesting makes more sense. I was not sure if polls on passive cluster is typical.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not register/unregister callbacks here, but we shouldn't do it at task queue level either, we we should register a single callback at the engine level:

  1. There's no reason to unload/reload at the task queue partition level, nothing below the partition cares about namespace state.
  2. The namespace registry calls the callback initially for every loaded namespace when registered, so that's a lot of unnecessary work.
  3. We don't need a new level of cancellation, we already keep a map of cancelfuncs at the engine level, we just need to register/unregister them in another map by namespace. See https://github.com/temporalio/temporal/blob/main/service/matching/matching_engine.go#L2570-L2574 and related.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correction: two more things do look at active namespace state:

  1. AddTask skips sync match if passive
  2. taskValidator skips first validation if active
    I don't think there's any reason to unload/reload because of those two.

Copy link
Contributor

@dnr dnr Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

On second thought, it might simplify a few other things if we did reload the partition on ns state change (metrics, query-only state in new matcher). So I'm leaning towards reloading the partition. But I still think the listener itself should be at the engine level and it can just iterate over loaded partitions and unload the ones that belong to ns that changed (which will automatically interrupt polls, of course). Or keep a map to be more efficient.

if ns.ID() == namespaceEntry.ID() && namespaceEntry.ActiveInCluster(c.clusterMeta.GetCurrentClusterName()) {
cancel()
}
})
defer c.namespaceRegistry.UnregisterStateChangeCallback(key)

t, err := c.matcher.PollForQuery(ctx, pollMetadata)
if !errors.Is(err, context.Canceled) {
return t, err
} // else the cluster has become active so continue regular poll path
Comment on lines +372 to +373
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessarily true, the caller could have canceled.

}

for {
Expand Down
Loading