-
Notifications
You must be signed in to change notification settings - Fork 1.1k
TaskQueue per-priority stats in API #8030
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
Conversation
c41265b
to
4e63150
Compare
6b011d0
to
b2cf130
Compare
WorkflowTaskTimeout: durationpb.New(10 * time.Minute), | ||
RequestId: uuid.New(), | ||
WorkflowIdReusePolicy: enumspb.WORKFLOW_ID_REUSE_POLICY_ALLOW_DUPLICATE, | ||
Priority: &commonpb.Priority{PriorityKey: int32(priority)}, |
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.
This is a notable test update.
@@ -102,7 +118,7 @@ func (s *TaskQueueStatsSuite) TestMultipleTasks_MultiplePartitions_WithMatchingB | |||
s.OverrideDynamicConfig(dynamicconfig.TaskQueueInfoByBuildIdTTL, 1*time.Millisecond) // zero means no TTL | |||
|
|||
s.RunTestWithMatchingBehavior(func() { | |||
s.publishConsumeWorkflowTasksValidateStats(50, false) // 25 unversioned, 25 versioned |
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.
Changed these from totals to "sets".
}, 5*time.Second, 100*time.Millisecond) | ||
} | ||
|
||
func validateTaskQueueStatsByPriority( |
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.
This is a net new set of assertions.
tests/task_queue_stats_test.go
Outdated
// Unless a test calls out a specific methods, all three methods are tested in each test case. | ||
func TestTaskQueueStatsSuite(t *testing.T) { | ||
// TODO(pri): remove once the classic matcher is removed | ||
func TestTaskQueueStatsSuite_ClassicMatcher(t *testing.T) { |
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.
the naming scheme for the others is:
func TestTaskQueueStatsSuite_ClassicMatcher(t *testing.T) { | |
func TestTaskQueueStats_Classic_Suite(t *testing.T) { |
I don't care that much but just keep them easy to type and consistent
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> **What changed?** Adding a breakdown of stats per-priority levels to the two APIs that return task queue stats already. <!-- Tell your future self why have you made these changes --> **Why?** To give users insights on a per priority level. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** N/A <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** temporalio/temporal#8030
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> **What changed?** Adding a breakdown of stats per-priority levels to the two APIs that return task queue stats already. <!-- Tell your future self why have you made these changes --> **Why?** To give users insights on a per priority level. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** N/A <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> **Server PR** temporalio/temporal#8030
What changed?
Adding a breakdown of stats per-priority levels to the two APIs that return task queue stats already.
Why?
New API is defined here: temporalio/api#611.
How did you test it?