-
Notifications
You must be signed in to change notification settings - Fork 279
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
[Feature]: Make local terminal cores configurable #2492
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2492 +/- ##
=========================================
Coverage 32.65% 32.65%
+ Complexity 4484 4483 -1
=========================================
Files 599 599
Lines 50310 50313 +3
Branches 6689 6689
=========================================
+ Hits 16428 16429 +1
- Misses 32566 32568 +2
Partials 1316 1316
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
@@ -313,6 +313,12 @@ public class ArcticManagementConf { | |||
.defaultValue(30) | |||
.withDescription("session timeout in minute"); | |||
|
|||
public static final ConfigOption<Integer> TERMINAL_LOCAL_CORES = |
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.
It seems to be a configuration only for the local terminal.
If yes, we may want to move it into LocalSessionFactory
and name it cores
because the TerminalManager
will reorganize configurations based on different backend types.
You can find similar configurations only for the Kyuubi terminal in KyuubiTerminalSessionFactory
.
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 agree that LocalSessionFactory
is a better place for the config option. But do you think cores
should be prefixed with the terminal type local
, as it's only effective for the local terminal. That makes the full key terminal.local.cores
.
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.
Sorry, my previous description was not clear. The complete configuration name in the configuration file should indeed be terminal.local.cores
. Just as you have implemented, in the code, we only need to name the configuration cores
, and TerminalManager
will handle the decomposition of different backend configurations.
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.
LGTM.
I created #2496 to follow the docs issues. |
Why are the changes needed?
As titled.
Brief change log
As titled.
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation