Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use a separate node pool to run custom connector jobs #19770
Use a separate node pool to run custom connector jobs #19770
Changes from 3 commits
950269b
f4cf12b
8c77be5
6f5ac3f
1b65f74
cf5e2b3
ccef1d8
6adaddc
3740aeb
67a4890
ad95566
5d238a7
7eea1a4
0ff5332
8c281c1
8616bba
6fd4f8f
1ecd501
d237974
1d4078e
6a063c0
af74bc0
219552b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Hm, I wonder if we should just thrown an exception if
usesIsolatedPool
is true but thegetWorkerIsolatedKubeNodeSelectors
Optional is empty. I think it would be safer for us to fail loudly if we didn't configure the isolated pool properly, rather than accidentally letting custom connectors run in our main pool. What do you think?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.
good call! fixed
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.
Realized this is not true - usesIsolatedPool here is actually passed by callers (isCustomConnector flag). Thus for OSS users who do not run them in a separate pool, usesIsolatedPool might still be true because they are custom connectors. I should rename this to isCustomConnectors instead.
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.
Oh good call, OSS users should be able to use custom connectors without an isolated pool indeed
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.
Similar comment here, I think it's more dangerous to fall back on our primary node selectors, and perhaps we should throw an exception instead.
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.
I added this check in
WorkerConfigurationBeanFactory.java
instead - because 1) workerConfigs does not carryuseIsolatedPool
flag, 2) should throw this error as early as possibleThere 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.
Nice!