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/12/03 14:37:04 UTC
[GitHub] [zeppelin] skyer9 opened a new pull request #4271: [ZEPPELIN-3493]
skyer9 opened a new pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271
### What is this PR for?
First of all, thanks to https://github.com/babokim/zeppelin/tree/presto_acl_20160809
My work is heavily based on his code & idea.
Second, this is for JDBCInterpreter only.
But, you can find it is easy porting to other Interpreter.
### What type of PR is it?
Bug Fix
### Todos
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-3493
### How should this be tested?
* execute select sql responsing more then 1000 rows.
* check CSV(ALL) menu works.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? not necessary, but good for convenience
--
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 a change in pull request #4271: [ZEPPELIN-3493] "Export all data as csv" not exporting all data
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271#discussion_r763113291
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
##########
@@ -614,6 +619,51 @@ public Response updateParagraphConfig(@PathParam("noteId") String noteId,
return new JsonResponse<>(Status.OK, "", p).build();
}
+ /**
+ * Download paragraph
+ * @param noteId
+ * @param paragraphId
+ * @return data stream
+ * @throws IOException
+ */
+ @GET
+ @Produces("text/csv")
+ @Path("{notebookId}/paragraph/{paragraphId}/download")
+ @ZeppelinApi
+ public Response downParagraph(@PathParam("notebookId") String noteId,
+ @PathParam("paragraphId") String paragraphId)
+ throws IOException {
+ LOGGER.info("Download paragraph, node.id: {}, paragrap.id: {}", noteId, paragraphId);
+
+ Note note = notebook.getNote(noteId);
+ if (note == null) {
+ return new JsonResponse(Status.NOT_FOUND, "note not found.").build();
+ }
+
+ Paragraph p = note.getParagraph(paragraphId);
+ if (p == null) {
+ return new JsonResponse(Status.NOT_FOUND, "paragraph not found.").build();
+ }
+
+ String resultFileParent = conf.getString(ZeppelinConfiguration.ConfVars.ZEPPELIN_RESULT_DATA_DIR);
+
+ String fileName = noteId + "_" + paragraphId;
+ final File file = new File(resultFileParent + "/" + fileName);
Review comment:
Please use the following constructor, which takes the OS path separator into account. https://docs.oracle.com/javase/7/docs/api/java/io/File.html#File(java.lang.String,%20java.lang.String)
--
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] skyer9 commented on pull request #4271: [ZEPPELIN-3493] "Export all data as csv" not exporting all data
Posted by GitBox <gi...@apache.org>.
skyer9 commented on pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271#issuecomment-985578805
# detailed action
- when execute sql, all data is saved to file
- when requested, download saved file.
- after four hour, saved file is deleted automatically.
- when there is no saved file, displayed data is downloaded.
--
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 #4271: [ZEPPELIN-3493] "Export all data as csv" not exporting all data
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271#issuecomment-985604025
@skyer9 Thanks for the contribution, could you paste a screenshot about how this feature is used by users?
--
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] skyer9 commented on a change in pull request #4271: [ZEPPELIN-3493] "Export all data as csv" not exporting all data
Posted by GitBox <gi...@apache.org>.
skyer9 commented on a change in pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271#discussion_r763685131
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/rest/NotebookRestApi.java
##########
@@ -614,6 +619,51 @@ public Response updateParagraphConfig(@PathParam("noteId") String noteId,
return new JsonResponse<>(Status.OK, "", p).build();
}
+ /**
+ * Download paragraph
+ * @param noteId
+ * @param paragraphId
+ * @return data stream
+ * @throws IOException
+ */
+ @GET
+ @Produces("text/csv")
+ @Path("{notebookId}/paragraph/{paragraphId}/download")
+ @ZeppelinApi
+ public Response downParagraph(@PathParam("notebookId") String noteId,
+ @PathParam("paragraphId") String paragraphId)
+ throws IOException {
+ LOGGER.info("Download paragraph, node.id: {}, paragrap.id: {}", noteId, paragraphId);
+
+ Note note = notebook.getNote(noteId);
+ if (note == null) {
+ return new JsonResponse(Status.NOT_FOUND, "note not found.").build();
+ }
+
+ Paragraph p = note.getParagraph(paragraphId);
+ if (p == null) {
+ return new JsonResponse(Status.NOT_FOUND, "paragraph not found.").build();
+ }
+
+ String resultFileParent = conf.getString(ZeppelinConfiguration.ConfVars.ZEPPELIN_RESULT_DATA_DIR);
+
+ String fileName = noteId + "_" + paragraphId;
+ final File file = new File(resultFileParent + "/" + fileName);
Review comment:
thank you for review my code.
changed to OS path separator.
and replaced "/tmp" to System.getProperty("java.io.tmpdir")
--
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] skyer9 commented on pull request #4271: [ZEPPELIN-3493] "Export all data as csv" not exporting all data
Posted by GitBox <gi...@apache.org>.
skyer9 commented on pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271#issuecomment-985879454
i added screenshot.
but it can take 10 seconds or more.
it's image is served on nano aws server
--
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 a change in pull request #4271: [ZEPPELIN-3493] "Export all data as csv" not exporting all data
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271#discussion_r777325332
##########
File path: jdbc/src/main/java/org/apache/zeppelin/jdbc/ResultDataFileProcessor.java
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.zeppelin.jdbc;
+
+import org.apache.zeppelin.interpreter.InterpreterContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+
+import static java.lang.System.getProperty;
+
+public class ResultDataFileProcessor {
+
+ Logger logger = LoggerFactory.getLogger(ResultDataFileProcessor.class);
+
+ static final String RESULT_DATA_PATH = "zeppelin.result.data.path";
+
+ String resultDataDir;
+ Properties property;
+ Map<Integer, File> map = new HashMap<>();
+
+ public ResultDataFileProcessor(Properties property) {
+
+ if (property == null) {
+ property = new Properties();
+ }
+ this.property = property;
+
+ prepare();
+ }
+
+ public synchronized void createFile(InterpreterContext context, int hashCode)
+ throws IOException, InterruptedException {
+
+ String resultFilePath = getFileName(context);
+ File resultFile = new File(resultFilePath);
+ if (resultFile.exists()) {
+ if (!resultFile.delete()) {
+ return;
+ }
+ }
+
+ if (!resultFile.createNewFile()) {
+ return;
+ }
+
+ map.put(hashCode, resultFile);
+ }
+
+ public File getFile(int hashCode) {
+ File file = map.get(hashCode);
+ map.remove(hashCode);
+ return file;
+ }
+
+ public void deleteFile(InterpreterContext context) {
+ String resultFilePath = getFileName(context);
+ File resultFile = new File(resultFilePath);
+ if (resultFile.exists()) {
+ resultFile.delete();
+ }
+ }
+
+ void prepare() {
+ resultDataDir = getProperty(RESULT_DATA_PATH);
+ if (resultDataDir == null) {
+ String defaultBaseDir = System.getProperty("java.io.tmpdir");
+ // set tmpdir to /tmp on MacOS, because docker can not share the /var folder which will
+ // cause PythonDockerInterpreter fails.
+ // https://stackoverflow.com/questions/45122459/docker-mounts-denied-the-paths-are-not-shared-
+ // from-os-x-and-are-not-known
+ if (System.getProperty("os.name", "").contains("Mac")) {
Review comment:
Please use [createTempDirectory(....)](https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createTempDirectory(java.lang.String,%20java.nio.file.attribute.FileAttribute...)) to create your temp directory.
This should work on MacOS, Windows, Linux etc.
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java
##########
@@ -989,6 +1003,9 @@ public void save(String location) throws ConfigurationException {
ZEPPELIN_NOTEBOOK_AUTO_INTERPRETER_BINDING("zeppelin.notebook.autoInterpreterBinding", true),
ZEPPELIN_CONF_DIR("zeppelin.conf.dir", "conf"),
ZEPPELIN_CONFIG_FS_DIR("zeppelin.config.fs.dir", ""),
+ ZEPPELIN_RESULT_DATA_DIR("zeppelin.result.data.path",
Review comment:
A default property should not depend on a `user.name`. At the moment of review, I don't even know what value is returned at this point.
A system property `user.name`, which is part of the Zeppelin startup parameters, is unknown to me.
##########
File path: jdbc/src/main/java/org/apache/zeppelin/jdbc/ResultDataFileCleaner.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.zeppelin.jdbc;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.util.Objects;
+
+public class ResultDataFileCleaner extends Thread {
Review comment:
Instead of a long-lived thread, I would prefer to delete the files when the [interpreter is closed](https://github.com/apache/zeppelin/blob/de3a88ee1732ece6ca167f3399f759c43c7cb76d/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/Interpreter.java#L65).
--
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] skyer9 edited a comment on pull request #4271: [ZEPPELIN-3493] "Export all data as csv" not exporting all data
Posted by GitBox <gi...@apache.org>.
skyer9 edited a comment on pull request #4271:
URL: https://github.com/apache/zeppelin/pull/4271#issuecomment-985879454
https://www.skyer9.pe.kr/wordpress/wp-content/uploads/2021/12/2021-12-03-01.png
i added screenshot.
but it can take 10 seconds or more.
it's image is served on nano aws server
--
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