Handle empty values for task and datasource conditions in pod template selector #17400
Conversation
There was a problem hiding this comment.
Thanks for your first patch @kirangadhave-imply ! In addition to a few more tests, can you please update the docs https://github.com/apache/druid/blob/master/docs/development/extensions-contrib/k8s-jobs.md#select-based-on-one-or-more-conditions to indicate what null is expected to mean aka match anything.
I agree with your comment in the description of this issue that if a user passes in null as a selector to either the task type or datasource - they most likely meant to match any task type or any datasource, since it is not possible to have a task without a type or a datasource. Let's make this explicit in the docs.
| "TestSelector", | ||
| cxtTagsConditions, | ||
| Sets.newHashSet(NoopTask.TYPE), | ||
| new HashSet<>() |
There was a problem hiding this comment.
Please add another test for passing in null for the datasource selector
There was a problem hiding this comment.
existing tests already cover null value
| Selector selector = new Selector( | ||
| "TestSelector", | ||
| cxtTagsConditions, | ||
| new HashSet<>(), |
There was a problem hiding this comment.
Please add another test for passing in null for the task type selector
|
|
||
| Selector selector = new Selector( | ||
| "TestSelector", | ||
| cxtTagsConditions, |
There was a problem hiding this comment.
Please add another test for passing in null for the tags condition
|
@suneet-s The |
There was a problem hiding this comment.
Thanks for this fix @kirangadhave-imply ! And congrats on your first contribution to the repo!
I will take care of merging this once CI passes. Can you please do a follow up patch to update the docs to explicitly call out the differences between an empty set being used for datasources or task type and an empty set being used in the tag selector.
The only reason I'm suggesting doing the docs change in a follow up patch is to reduce the CI time, since a docs only change goes through much fewer tests.
kirangadhave-imply
deleted the
fix-pod-template-selector-empty-conditions
branch
…e selector (apache#17400) * handling empty sets for dataSourceCondition and taskTypeCondition * using new HashSet<>() to fix forbidden api error in testCheck * fixing style issues
Description
The condition based selector that evaluates tasks based on specified criteria failed to handle empty values for task type or datasource. If the condition is empty or null it can be matched to any task, but previous logic checked for task type or task datasource in the empty set resulting in skipping of that selector. Expected behavior is to treat empty sets the same as null.
Fixed the evaluate method in Selector to check if the task type or datasource conditions are null or empty instead of just a null check.
Release note
Fixed: You can now pass empty arrays to
typeanddataSourcekeys in selector based pod template selection strategyThis PR has: