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

Update docs for K8s TaskRunner Dynamic Config #16600

Merged
merged 6 commits into from
Jun 21, 2024
Merged

Conversation

suneet-s
Copy link
Contributor

@suneet-s suneet-s commented Jun 13, 2024

Description

Mostly a refactor of the docs added in #16510

While writing these docs, I realized that the use of a defaultKey in the dynamic config (that I suggested in the original patch) is not actually that helpful, so I removed it from the docs and the code.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.

@suneet-s
Copy link
Contributor Author

@YongGang fyi

docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
docs/development/extensions-contrib/k8s-jobs.md Outdated Show resolved Hide resolved
@suneet-s
Copy link
Contributor Author

thanks for the reviews @vtlim @asdf2014 and @YongGang I believe this patch should have addressed all the comments


return templates.getOrDefault(templateKey, templates.get("base"));
PodTemplate podTemplate = templates.getOrDefault(templateKey, templates.get("base"));
Copy link
Contributor

Choose a reason for hiding this comment

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

A non-null podTemplate will always be returned in this line of code as when loading templates it will make sure base template get configured here

match any selector in the list, it will use the `base` pod template.

For a task to match a selector, all the conditions within the selector must match. A selector can match on
- `type`: Type of the task
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `type`: Type of the task
- `type`: Type of the task.

Copy link
Member

@vtlim vtlim left a comment

Choose a reason for hiding this comment

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

Thanks for adding these docs! 🦖

|`SelectorBasedPodTemplateSelectStrategy`| This strategy evaluates a series of selectors, known as `selectors`, which are aligned with potential task properties. | false |
#### Pod template selection

The pod template adapapter can select which pod template should be used for a task using the [task runner execution config](#dynamic-config)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The pod template adapapter can select which pod template should be used for a task using the [task runner execution config](#dynamic-config)
The pod template adapter can select which pod template should be used for a task using the [task runner execution config](#dynamic-config)

Raised by spell checker

@suneet-s
Copy link
Contributor Author

Overruling CI because the k8s extension is not tested in these integration tests

@suneet-s suneet-s merged commit 4e0ea78 into apache:master Jun 21, 2024
85 of 87 checks passed
@suneet-s suneet-s deleted the k8s-docs branch June 21, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants