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

[8.16][Task Manager] Honor config-provided poll_interval #205064

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

afharo
Copy link
Member

@afharo afharo commented Dec 21, 2024

Summary

We noticed that 8.16 overrides the any configured poll_interval if claim_strategy is not set (the default).

Checklist

@afharo afharo added bug Fixes for quality problems that affect the customer experience release_note:fix Feature:Task Manager backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Dec 21, 2024
@afharo afharo requested a review from a team December 21, 2024 08:38
@afharo afharo self-assigned this Dec 21, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

cc @afharo

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Changes look good to me, and takes the claim strategy into account.

* @param config Current config
* @param defaultPollInterval Default to set
*/
function maybeSetDefaultInterval(config: TaskManagerConfig, defaultPollInterval: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! I was looking at how this function differs to letting the poll_interval overwrite completely (opts.config.poll_interval ?? DEFAULT_POLL_INTERVAL).

From looking at the function, it seems if ever the user sets xpack.task_manager.poll_interval: 3000 in their yml and their deployment is auto enrolled to the mget task claiming strategy, we're going to discard the 3000 and set 500 as the value. I guess this is the current behaviour on 8.16, was your intent to preserve this as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIUC, this method returns the default only if the user didn't provide it (which, in case they didn't set "mget" in their kibana.yml is DEFAULT_POLL_INTERVAL).

I don't think that the a/b decision logic in this file affects it (at least, that was not my intention).

In case they set "mget" in their kibana.yml, this method is not called because there's an early return at the top of this file.

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for submitting a PR!

@afharo
Copy link
Member Author

afharo commented Dec 23, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

There are no new commits on the base branch.

@afharo afharo merged commit 022f029 into elastic:8.16 Dec 23, 2024
7 of 8 checks passed
@afharo afharo deleted the 8.16-task-manager-default-fix branch December 23, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience Feature:Task Manager release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants