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 2021/05/20 03:13:29 UTC
[GitHub] [zeppelin] mnmhouse opened a new pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
mnmhouse opened a new pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122
### What is this PR for?
when download distirbution package , run zeppelin server ,do not config zeppelin-site.xml ,i found LIST_CONFIGURATIONS command cannot return the default configuration in the ZeppelinConfiguration class code,so the web lack some configuration
### What type of PR is it?
[Bug Fix]
### Todos
* [ ] - Task
### What is the Jira issue?
* [ZEPPELIN-5379] https://issues.apache.org/jira/browse/ZEPPELIN-5379
### How should this be tested?
* Ci pass and manually tested
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update?
* Is there breaking changes for older versions?
* Does this needs documentation?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122#issuecomment-853596439
LGTM, will merge if no more comment
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122#discussion_r635821821
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -871,6 +871,10 @@ public boolean isPrometheusMetricEnabled() {
completeConfiguration.put(c.getVarName(), sysConfig.getString(c.getVarName()));
} else if (envConfig.containsKey(c.name())) {
completeConfiguration.put(c.getVarName(), envConfig.getString(c.name()));
+ }else {
+ if (getString(c)!=null){
+ completeConfiguration.put(c.getVarName(),getString(c));
+ }
Review comment:
> Since `envConfig` and `sysConfig` are checked in `getString()`, would it be okay to delete lines 870~873?
You should be right. I think we can also start with an empty Hashmap.
```java
public Map<String, String> getCompleteConfiguration() {
Map<String, String> completeConfiguration = new HashMap<>();
for (ConfVars c : ConfVars.values()) {
if (getString(c) != null){
completeConfiguration.put(c.getVarName(), getString(c));
}
}
return completeConfiguration;
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] asfgit closed pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] cuspymd commented on a change in pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
cuspymd commented on a change in pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122#discussion_r635757279
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -871,6 +871,10 @@ public boolean isPrometheusMetricEnabled() {
completeConfiguration.put(c.getVarName(), sysConfig.getString(c.getVarName()));
} else if (envConfig.containsKey(c.name())) {
completeConfiguration.put(c.getVarName(), envConfig.getString(c.name()));
+ }else {
+ if (getString(c)!=null){
+ completeConfiguration.put(c.getVarName(),getString(c));
+ }
Review comment:
Since `envConfig` and `sysConfig` are checked in `getString()`, would it be okay to delete lines 870~873?
However, it seems that the check order of `envConfig` and `sysConfig` is different only in `getCompleteConfiguration()`.
@zjffdu @Reamer Which of `envConfig` and `sysConfig` takes precedence?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] asfgit closed pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] mnmhouse commented on a change in pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
mnmhouse commented on a change in pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122#discussion_r635916805
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -871,6 +871,10 @@ public boolean isPrometheusMetricEnabled() {
completeConfiguration.put(c.getVarName(), sysConfig.getString(c.getVarName()));
} else if (envConfig.containsKey(c.name())) {
completeConfiguration.put(c.getVarName(), envConfig.getString(c.name()));
+ }else {
+ if (getString(c)!=null){
+ completeConfiguration.put(c.getVarName(),getString(c));
+ }
Review comment:
@cuspymd You look very carefully，i did not notice the getString() method
@Reamer thank you ,I will modify according to the code you provided
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122#issuecomment-853596439
LGTM, will merge if no more comment
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4122: [ZEPPELIN-5379] LIST_CONFIGURATIONS command cannot return the default…
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4122:
URL: https://github.com/apache/zeppelin/pull/4122#discussion_r635814385
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -871,6 +871,10 @@ public boolean isPrometheusMetricEnabled() {
completeConfiguration.put(c.getVarName(), sysConfig.getString(c.getVarName()));
} else if (envConfig.containsKey(c.name())) {
completeConfiguration.put(c.getVarName(), envConfig.getString(c.name()));
+ }else {
+ if (getString(c)!=null){
+ completeConfiguration.put(c.getVarName(),getString(c));
+ }
Review comment:
@cuspymd You can find the order in the [current docs](https://github.com/apache/zeppelin/blob/master/docs/setup/operation/configuration.md).
Comment by @zjffdu: https://github.com/apache/zeppelin/pull/4028#discussion_r560194922
I noticed a small typo. PR #4123 should fix this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org