-
Notifications
You must be signed in to change notification settings - Fork 883
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
Initialize EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH before using #5045
Conversation
…cutorPodNamePrefixForK8s method
I don't think so, if you try to decompile the class file, the assignment should occur inside the constructor. |
Yes. Is there another problem possibly lead to this ? |
Sorry, I missed Please run |
Got it. Thanks. |
@@ -376,7 +380,6 @@ object SparkSQLEngine extends Logging { | |||
// only spark driver pod will build with `SPARK_APPLICATION_ID` env. | |||
Utils.isOnK8s && sys.env.contains("SPARK_APPLICATION_ID") | |||
} | |||
|
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.
nit: restore this blank line to minimize the change
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.
Got it
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, only one minor thing
…using ### _Why are the changes needed?_ I came cross an issue that is the the value of EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH is 0 when the param is accessed in generateExecutorPodNamePrefixForK8s method. I tried to move the EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH ahead of generateExecutorPodNamePrefixForK8s method. Then, I found this issue was gone. So is it necessary to declare the EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH variable before the method definition? ### _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](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request Closes #5045 from zhaohehuhu/Improvement. Closes #5045 c74732f [hezhao2] recover the blank line 99aa14b [hezhao2] fix the code style 29929a2 [hezhao2] declare EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH param before generateExecutorPodNamePrefixForK8s method Authored-by: hezhao2 <hezhao2@cisco.com> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit da4af2a) Signed-off-by: Cheng Pan <chengpan@apache.org>
Thanks, merged to master/1.7 |
…efore using ### _Why are the changes needed?_ I came cross an issue that is the the value of EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH is 0 when the param is accessed in generateExecutorPodNamePrefixForK8s method. I tried to move the EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH ahead of generateExecutorPodNamePrefixForK8s method. Then, I found this issue was gone. So is it necessary to declare the EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH variable before the method definition? ### _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](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request Closes apache#5045 from zhaohehuhu/Improvement. Closes apache#5045 c74732f [hezhao2] recover the blank line 99aa14b [hezhao2] fix the code style 29929a2 [hezhao2] declare EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH param before generateExecutorPodNamePrefixForK8s method Authored-by: hezhao2 <hezhao2@cisco.com> Signed-off-by: Cheng Pan <chengpan@apache.org>
Hi @zhaohehuhu, after this PR, we need new feature about force rewrite executor pod name prefix, would you like to take it? |
Why are the changes needed?
I came cross an issue that is the the value of EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH is 0 when the param is accessed in generateExecutorPodNamePrefixForK8s method.
I tried to move the EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH ahead of generateExecutorPodNamePrefixForK8s method. Then, I found this issue was gone.
So is it necessary to declare the EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH variable before the method definition?
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 make a pull request