You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/03/04 02:54:46 UTC

[GitHub] [zookeeper] MuktiKrishnan opened a new pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

MuktiKrishnan opened a new pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620


   


----------------------------------------------------------------
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] [zookeeper] arshadmohammad commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r589597789



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       ` It contains ephemeral files, that won't be present at the next execution of the program.`
   Makes sense to use dynamically created files
   




----------------------------------------------------------------
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] [zookeeper] arshadmohammad commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r591200268



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       it can be used. 




----------------------------------------------------------------
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] [zookeeper] arshadmohammad commented on pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#issuecomment-792885947


   Changes look good to me. After changes is concluded with @eolivelli I will merge.
   @MuktiKrishna this PR also should have been raised on master first.


----------------------------------------------------------------
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] [zookeeper] MuktiKrishnan closed pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
MuktiKrishnan closed pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620


   


----------------------------------------------------------------
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] [zookeeper] eolivelli commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r587207166



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       Can we use this utility function ?
   https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createTempDirectory%28java.nio.file.Path,%20java.lang.String,%20java.nio.file.attribute.FileAttribute...%29




----------------------------------------------------------------
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] [zookeeper] eolivelli commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r589771026



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       Sp why can't we use the official method to create an unique temporary directory?




----------------------------------------------------------------
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] [zookeeper] MuktiKrishnan commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
MuktiKrishnan commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r589191189



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       web resource path has to be a static path. createTempDirectory will make it dynamic which will break the functionality




----------------------------------------------------------------
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] [zookeeper] arshadmohammad commented on pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#issuecomment-796458160


   Merged from https://github.com/apache/zookeeper/pull/1633


----------------------------------------------------------------
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] [zookeeper] eolivelli commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r591258590



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       so I believe it is better to use `createTempDirectory`
   the '/23cxv45345/2131xc2/' suffix is useless
   




----------------------------------------------------------------
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] [zookeeper] arshadmohammad closed pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
arshadmohammad closed pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620


   


----------------------------------------------------------------
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] [zookeeper] arshadmohammad commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
arshadmohammad commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r589568318



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       yes, seems functionality will change if directory is created dynamically. How someone will put web resources in that dynamically created directory. So lets not use createTempDirectory.
   
   What you say @eolivelli 




----------------------------------------------------------------
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] [zookeeper] eolivelli commented on a change in pull request #1620: ZOOKEEPER-4230: tmp is not configurable in zookeeper-contrib-rest

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1620:
URL: https://github.com/apache/zookeeper/pull/1620#discussion_r589588606



##########
File path: zookeeper-contrib/zookeeper-contrib-rest/src/main/java/org/apache/zookeeper/server/jersey/RestMain.java
##########
@@ -53,7 +53,7 @@ public void start() throws IOException {
        System.out.println("Starting grizzly ...");
 
        boolean useSSL = cfg.useSSL();
-       gws = new GrizzlyWebServer(cfg.getPort(), "/tmp/23cxv45345/2131xc2/", useSSL);
+       gws = new GrizzlyWebServer(cfg.getPort(), System.getProperty("java.io.tmpdir","/tmp") + "/23cxv45345/2131xc2/", useSSL);

Review comment:
       Then I don't understand why we want to use tmpdir. It contains ephemeral files, that won't be present at the next execution of the program. 
   
   I thought we just had to set some existing directory, without any particular meaning




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