You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/05/13 08:10:15 UTC
[GitHub] [zeppelin] huage1994 opened a new pull request, #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
huage1994 opened a new pull request, #4368:
URL: https://github.com/apache/zeppelin/pull/4368
### What is this PR for?
This PR is to upport time unit setting in zeppelin conf, such as `5s` ,`3m` ,`1h`.
### What type of PR is it?
Feature
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN-5730
### How should this be tested?
* CI passed
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1125951606
> Thank you but it looks like braking configuration after upgrading it. Do you have any plan to support old configuration in a new version? Otherwise, do users need to change it by hand?
Hi @jongyoul, Old configuration is also supported in this PR.
I am sorry to forget to add comments for the code.
If the string passed in doesn't include time unit, method `timeUnitToMill` would fail to parse and return `-1`. And the original code logic is then executed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
Reamer commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1134796637
> > This PR currently seems to cause the test ` K8sStandardInterpreterLauncherTest` to fail, though they looks like to be not related. It's ok to run ` K8sStandardInterpreterLauncherTest` in my local environment. But it failed every time on CI of my fork repository.
> > I'll take some time to resolve this.
>
> Fixed.
How did it happen? How did you fix the error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1125990383
> @huage1994 Could you add unit test in ZeppelinConfigurationTest?
Sure. I would add it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
Reamer commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1145946999
I will merge this PR into the master next week if no further comments are received.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1125981646
@huage1994 Could you add unit test in ZeppelinConfigurationTest?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
jongyoul commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1125928609
Thank you but it looks like braking configuration after upgrading it. Do you have any plan to support old configuration in a new version? Otherwise, do users need to change it by hand?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer merged pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
Reamer merged PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1127457238
> Another improvement `timeUnitToMill` should throw an exception instead of returning `-1`. This exception should be caught and reacted to.
👍 Thanks a lot for your advice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1135320025
> How did it happen? How did you fix the error?
The type of default value of ZEPPELIN_INTERPRETER_CONNECT_TIMEOUT was int before.
When the ZEPPELIN_INTERPRETER_CONNECT_TIMEOUT is not set, we expect to get the default value , but `getLong()` in `getTime()` would get nothing because `intValue` and `longValue` are two independent fields in `ConfVars`.
I change the default value type from `int` to `long` as follow.
```
- ZEPPELIN_INTERPRETER_CONNECT_TIMEOUT("zeppelin.interpreter.connect.timeout", 600000),
+ ZEPPELIN_INTERPRETER_CONNECT_TIMEOUT("zeppelin.interpreter.connect.timeout", 600000L),
```
And I added comment for method `getTime`.
```
/**
* This method is to support time unit like `1s`, `2m`, `3h`.
*
* @param {ConfVars} c . Note：The type of default value of `ConfVars c` should be long.
* @return {long} Milliseconds
*/
public long getTime(ConfVars c) {
try {
return timeUnitToMill(getString(c.name(), c.getVarName(), ""));
} catch (Exception e) {
return getLong(c);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
Reamer commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1127452381
> How do you think about it?
Much better.
Another improvement `timeUnitToMill` should throw an exception instead of returning `-1`. This exception should be caught and reacted to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
Reamer commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1127283568
I do not like this solution and would "veto" it. The readability and simplicity of the code suffers greatly with this change.
I would prefer a conversion directly at the call (e.g. https://github.com/apache/zeppelin/blob/85ed8e2e51e1ea10df38d4710216343efe218d60/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java#L51).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1132495431
CI seems to be unstable recently 🤒 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
zjffdu commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1127206670
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1127446019
> I do not like this solution and would "veto" it. The readability and simplicity of the code suffers greatly with this change. I would prefer a conversion directly at the call (e.g.
>
> https://github.com/apache/zeppelin/blob/85ed8e2e51e1ea10df38d4710216343efe218d60/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/lifecycle/TimeoutLifecycleManager.java#L51
>
> ).
Hi @Reamer , I see what you mean.
Now I plan to keep the original code of method `getLong` without changin.
And then add a new method `getTime`. Maybe like this:
```
public long getTime(String envName, String propertyName, long defaultValue) {
String timeStr = getString(envName, propertyName, defaultValue + "");
if(timeUnitToMill(timeStr) > 0){
return timeUnitToMill(timeStr);
}
return getLong(envName, propertyName, defaultValue);
}
```
Finally ,replace the calls of `getLong` with `getTime` where needed.
How do you think about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4368: [ZEPPELIN-5730] Support time unit setting in zeppelin conf.
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4368:
URL: https://github.com/apache/zeppelin/pull/4368#issuecomment-1134256118
> This PR currently seems to cause the test ` K8sStandardInterpreterLauncherTest` to fail, though they looks like to be not related. It's ok to run ` K8sStandardInterpreterLauncherTest` in my local environment. But it failed every time on CI of my fork repository.
>
> I'll take some time to resolve this.
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org