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/02/25 08:38:03 UTC
[GitHub] [zeppelin] Bowen0729 opened a new pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Bowen0729 opened a new pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061
### What is this PR for?
Add a feature "rename note" for Zeppelin Client,
### What type of PR is it?
Improvement
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-5262
### How should this be tested?
CI
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
----------------------------------------------------------------
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] Bowen0729 commented on pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#issuecomment-788578114
Is there anything else that needs to be modified?@zjffdu
----------------------------------------------------------------
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] Bowen0729 commented on a change in pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r584207573
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +42,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ Assert.assertEquals(newNotePath, renamedNoteResult.getNotePath());
Review comment:
I have added the unit test in ZeppelinClientIntegrationTest, BTW, why put the unit test in modul "zeppelin-interpreter-integration"?
----------------------------------------------------------------
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] Bowen0729 commented on pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#issuecomment-788817760
> @Bowen0729 No, thanks for your contribution, will merge it if no more comment
Thanks
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r584254807
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ assert newNotePath.equals(renamedNoteResult.getNotePath());
Review comment:
Please remove this assert
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ assert newNotePath.equals(renamedNoteResult.getNotePath());
Review comment:
Please remove this assert statement
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583412017
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -22,6 +22,8 @@
import org.apache.zeppelin.client.ParagraphResult;
import org.apache.zeppelin.client.ZeppelinClient;
+import java.util.Objects;
Review comment:
Unnecessary import
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r584254736
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +42,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ Assert.assertEquals(newNotePath, renamedNoteResult.getNotePath());
Review comment:
Because zeppelin-client depends on zeppelin-server, that's why we put zeppelin-client module's test in `zeppelin-interpreter-integration` which includes the end-to-end integration test.
----------------------------------------------------------------
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] Bowen0729 commented on a change in pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583352741
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,9 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ zClient.renameNote(noteId, notePath + "_rename");
+ System.out.println("Rename note: " + noteId + " to " + notePath + "_rename ");
Review comment:
Thank you for your review, I made som changes on 9779df4
----------------------------------------------------------------
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] Bowen0729 edited a comment on pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 edited a comment on pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#issuecomment-788817760
> @Bowen0729 No, thanks for your contribution, will merge it if no more comment
Thanks
-----------
as for the new push: No changes, just misoperation
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#issuecomment-788587897
@Bowen0729 No, thanks for your contribution, will merge it 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] zjffdu commented on a change in pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583359528
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,10 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ zClient.renameNote(noteId, notePath + "_rename");
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ System.out.println("Rename note: " + noteId + " name to " + renamedNoteResult.getNoteName());
Review comment:
Use assert to verify the new note name is the expected value. Don't use `System.out.println` in unit test, it could not verify the correctness
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r582651282
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,9 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ zClient.renameNote(noteId, notePath + "_rename");
+ System.out.println("Rename note: " + noteId + " to " + notePath + "_rename ");
Review comment:
Use getNote to verify the rename operation is successful
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583434365
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +42,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ Assert.assertEquals(newNotePath, renamedNoteResult.getNotePath());
Review comment:
Sorry, my fault, `ZeppelinClientExample` is not unit test class, it is only for demo. So no need assert here.
You should add unit test in `ZeppelinClientIntegrationTest`
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583411948
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +43,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ assert Objects.equals(newNotePath, renamedNoteResult.getNotePath());
Review comment:
Not this assert, should be JUnit's assertEquals method
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583434365
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +42,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ Assert.assertEquals(newNotePath, renamedNoteResult.getNotePath());
Review comment:
Sorry, my fault, `ZeppelinClientExample` is not unit test class, it is only for demo. So need assert here.
You should add unit test in `ZeppelinClientIntegrationTest`
----------------------------------------------------------------
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] Bowen0729 commented on a change in pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583403199
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,10 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ zClient.renameNote(noteId, notePath + "_rename");
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ System.out.println("Rename note: " + noteId + " name to " + renamedNoteResult.getNoteName());
Review comment:
That's certainly right, and I used assert now
----------------------------------------------------------------
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] Bowen0729 commented on a change in pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583352741
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,9 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ zClient.renameNote(noteId, notePath + "_rename");
+ System.out.println("Rename note: " + noteId + " to " + notePath + "_rename ");
Review comment:
Thank you for your review, I made som changes on 76e4da4
----------------------------------------------------------------
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] Bowen0729 commented on a change in pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r584299575
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ assert newNotePath.equals(renamedNoteResult.getNotePath());
Review comment:
ok
----------------------------------------------------------------
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 #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583450426
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +41,10 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ zClient.renameNote(noteId, notePath + "_rename");
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ System.out.println("Rename note: " + noteId + " name to " + renamedNoteResult.getNoteName());
Review comment:
@zjffdu You are sure that you want to use the assert function of JUnit outside of a test class?
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +43,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ assert Objects.equals(newNotePath, renamedNoteResult.getNotePath());
Review comment:
@zjffdu You are sure that you want to use the assert function of JUnit outside of a test class?
----------------------------------------------------------------
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] Bowen0729 commented on a change in pull request #4061: [ZEPPELIN-5262] Add feature 'rename note' for Zeppelin Client
Posted by GitBox <gi...@apache.org>.
Bowen0729 commented on a change in pull request #4061:
URL: https://github.com/apache/zeppelin/pull/4061#discussion_r583429143
##########
File path: zeppelin-client-examples/src/main/java/org/apache/zeppelin/client/examples/ZeppelinClientExample.java
##########
@@ -41,6 +43,13 @@ public static void main(String[] args) throws Exception {
noteId = zClient.createNote(notePath);
System.out.println("Created note: " + noteId);
+ String newNotePath = notePath + "_rename";
+ zClient.renameNote(noteId, newNotePath);
+
+ NoteResult renamedNoteResult = zClient.queryNoteResult(noteId);
+ assert Objects.equals(newNotePath, renamedNoteResult.getNotePath());
Review comment:
got it, how about now?
----------------------------------------------------------------
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