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/01/19 11:45:14 UTC

[GitHub] [zeppelin] Reamer opened a new pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Reamer opened a new pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028


   ### What is this PR for?
   This PR migrates the configuration library to version 2
   
   ### What type of PR is it?
   - Improvement
   
   ### Todos
   * [ ] - Add CI for for parseExceptions
   * [ ] - Update license file
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5210
   
   ### How should this be tested?
   * via CI
   * 
   ### Questions:
   * Does the licenses files need update? Yes
   * 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#issuecomment-762860114


   @Reamer  besides the upgrading, is there any behavior change in the user perspective ?


----------------------------------------------------------------
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 #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028


   


----------------------------------------------------------------
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 pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#issuecomment-762958088


   > @Reamer besides the upgrading, is there any behavior change in the user perspective ?
   
   Not that I'm aware of.


----------------------------------------------------------------
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 #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#discussion_r560723952



##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java
##########
@@ -154,7 +154,6 @@ public ZeppelinServer() {
 
   public static void main(String[] args) throws InterruptedException, IOException {
     ZeppelinServer.conf = ZeppelinConfiguration.create();
-    conf.setProperty("args", args);

Review comment:
       @zjffdu I think that this line has no effect. Is this correct?




----------------------------------------------------------------
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 edited a comment on pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
Reamer edited a comment on pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#issuecomment-762958088


   > @Reamer besides the upgrading, is there any behavior change in the user perspective ?
   
   Not that I'm aware of. The most CI tests are working without a change.


----------------------------------------------------------------
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 a change in pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#discussion_r560732792



##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java
##########
@@ -154,7 +154,6 @@ public ZeppelinServer() {
 
   public static void main(String[] args) throws InterruptedException, IOException {
     ZeppelinServer.conf = ZeppelinConfiguration.create();
-    conf.setProperty("args", args);

Review comment:
       Yes，we could remove 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] Reamer commented on a change in pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#discussion_r560723172



##########
File path: docs/setup/operation/configuration.md
##########
@@ -24,12 +24,14 @@ limitations under the License.
 <div id="toc"></div>
 
 ## Zeppelin Properties
-There are two locations you can configure Apache Zeppelin.
 
-* **Environment variables** can be defined `conf/zeppelin-env.sh`(`conf\zeppelin-env.cmd` for Windows).
-* **Java properties** can be defined in `conf/zeppelin-site.xml`.
+Zeppelin can be configured via several sources.
+
+Sources descending by priority:
+ - system properties
+ - environment variables can be defined `conf/zeppelin-env.sh`(`conf\zeppelin-env.cmd` for Windows).
+ - configuration file can be defined in `conf/zeppelin-site.xml`

Review comment:
       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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#issuecomment-765435792


   I will include this PR in master and branch-0.9 on Monday (25.01.2021) 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#issuecomment-765209670


   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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028


   


----------------------------------------------------------------
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 a change in pull request #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#discussion_r560194922



##########
File path: docs/setup/operation/configuration.md
##########
@@ -24,12 +24,14 @@ limitations under the License.
 <div id="toc"></div>
 
 ## Zeppelin Properties
-There are two locations you can configure Apache Zeppelin.
 
-* **Environment variables** can be defined `conf/zeppelin-env.sh`(`conf\zeppelin-env.cmd` for Windows).
-* **Java properties** can be defined in `conf/zeppelin-site.xml`.
+Zeppelin can be configured via several sources.
+
+Sources descending by priority:
+ - system properties
+ - environment variables can be defined `conf/zeppelin-env.sh`(`conf\zeppelin-env.cmd` for Windows).
+ - configuration file can be defined in `conf/zeppelin-site.xml`

Review comment:
       The order should be:
   * environment variable
   * system property
   * property in zeppelin-site.xml
   ![image](https://user-images.githubusercontent.com/164491/105044427-adf9dd00-5aa1-11eb-87a0-1d3d18a165b9.png)
   




----------------------------------------------------------------
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 #4028: [ZEPPELIN-5210] Upgrade Configuration library to version 2

Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4028:
URL: https://github.com/apache/zeppelin/pull/4028#discussion_r560307125



##########
File path: docs/setup/operation/configuration.md
##########
@@ -24,12 +24,14 @@ limitations under the License.
 <div id="toc"></div>
 
 ## Zeppelin Properties
-There are two locations you can configure Apache Zeppelin.
 
-* **Environment variables** can be defined `conf/zeppelin-env.sh`(`conf\zeppelin-env.cmd` for Windows).
-* **Java properties** can be defined in `conf/zeppelin-site.xml`.
+Zeppelin can be configured via several sources.
+
+Sources descending by priority:
+ - system properties
+ - environment variables can be defined `conf/zeppelin-env.sh`(`conf\zeppelin-env.cmd` for Windows).
+ - configuration file can be defined in `conf/zeppelin-site.xml`

Review comment:
       You are right. I will change the order to prevent breaking changes.




----------------------------------------------------------------
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