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 2020/02/12 02:46:49 UTC

[GitHub] [zeppelin] zjffdu opened a new pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

zjffdu opened a new pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641
 
 
   ### What is this PR for?
   This PR allow user to set default interpreter in frontend. 
   
   ### What type of PR is it?
   [Feature]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-3751
   
   ### How should this be tested?
   * CI pass
   
   ### Screenshots (if appropriate)
   ![spark_switch](https://user-images.githubusercontent.com/164491/74298501-f33b6800-4d84-11ea-8352-f63d227a996f.gif)
   
   
   ### 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


With regards,
Apache Git Services

[GitHub] [zeppelin] zjffdu commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#discussion_r378218709
 
 

 ##########
 File path: zeppelin-web/src/app/notebook/notebook.controller.js
 ##########
 @@ -1251,11 +1251,11 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
   };
 
   const isSettingDirty = function() {
-    // if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
-    //   return false;
-    // } else {
-    return false;
-    // }
+    if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
+      return false;
+    } else {
+      return false;
 
 Review comment:
   Oop, I think this is a bug, I will fix 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


With regards,
Apache Git Services

[GitHub] [zeppelin] weand commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
weand commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#issuecomment-585600306
 
 
   > @weand The current listed interpreter are the default interpreter type you selected when creating note + interpreter with the same group of the default interpreter type and all the interpreters used in the note. e.g. you create a note with default interpreter `spark` and in your zeppelin instance there's another spark interpreter say `spark2` with other configurations, then both `spark` and `spark2` will be displayed.
   > 
   > Does this work for you ?
   
   Yeah, this will resolve my major concern. Sorry didn't got that from the code / comments initially, so :+1:
   
   

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


With regards,
Apache Git Services

[GitHub] [zeppelin] weand commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
weand commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#issuecomment-585580630
 
 
   sorry :-1: 
   
   the proposed change doesn't cover the use case discussed in the jira.
   
   With the current changeset, endusers still can't select another default interpreter, if it's not explicitly used in at least one paragraph. So users first need to create a dummy paragraph for each interpreter that should be available for selection. this is useless and not intuitive. 
   
   In my understanding the list of interpreters available in the list should NOT be derived from the paragraphs. instead we should list every interpreter, to which the current user has access to + the current default interpreter. in case the current default interpreter X is changed to Y and the user has no access to the X, it will be hidden, as soon as the interpreter binding is saved.
   
   what do you think? 

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


With regards,
Apache Git Services

[GitHub] [zeppelin] zjffdu commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#issuecomment-585635260
 
 
   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


With regards,
Apache Git Services

[GitHub] [zeppelin] zjffdu commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#issuecomment-585601000
 
 
   Awesome, thanks for confirm.

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


With regards,
Apache Git Services

[GitHub] [zeppelin] asfgit closed pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641
 
 
   

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


With regards,
Apache Git Services

[GitHub] [zeppelin] alexott commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
alexott commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#discussion_r378177138
 
 

 ##########
 File path: zeppelin-web/src/app/notebook/notebook.controller.js
 ##########
 @@ -1251,11 +1251,11 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
   };
 
   const isSettingDirty = function() {
-    // if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
-    //   return false;
-    // } else {
-    return false;
-    // }
+    if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
+      return false;
+    } else {
+      return false;
 
 Review comment:
   do we need condition if we just return false in both cases?

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


With regards,
Apache Git Services

[GitHub] [zeppelin] alexott commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
alexott commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#discussion_r378218366
 
 

 ##########
 File path: zeppelin-web/src/app/notebook/notebook.controller.js
 ##########
 @@ -1251,11 +1251,11 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
   };
 
   const isSettingDirty = function() {
-    // if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
-    //   return false;
-    // } else {
-    return false;
-    // }
+    if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
+      return false;
+    } else {
+      return false;
 
 Review comment:
   but we return false in both cases... should `else` return `true`?
   

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


With regards,
Apache Git Services

[GitHub] [zeppelin] zjffdu commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on issue #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#issuecomment-585592355
 
 
   @weand The current listed interpreter are the default interpreter type you selected when creating note + interpreter with the same group of the default interpreter type and all the interpreters used in the note. e.g. you create a note with default interpreter `spark` and in your zeppelin instance there's another spark interpreter say `spark2` with other configurations, then both `spark` and `spark2` will be displayed. 
   
   Does this work for you ?

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


With regards,
Apache Git Services

[GitHub] [zeppelin] zjffdu commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#discussion_r378218709
 
 

 ##########
 File path: zeppelin-web/src/app/notebook/notebook.controller.js
 ##########
 @@ -1251,11 +1251,11 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
   };
 
   const isSettingDirty = function() {
-    // if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
-    //   return false;
-    // } else {
-    return false;
-    // }
+    if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
+      return false;
+    } else {
+      return false;
 
 Review comment:
   Oops, I think this is a bug, I will fix 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


With regards,
Apache Git Services

[GitHub] [zeppelin] zjffdu commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #3641: [ZEPPELIN-3751]. Interpreter Binding menu of note only shows default interpreter
URL: https://github.com/apache/zeppelin/pull/3641#discussion_r378217150
 
 

 ##########
 File path: zeppelin-web/src/app/notebook/notebook.controller.js
 ##########
 @@ -1251,11 +1251,11 @@ function NotebookCtrl($scope, $route, $routeParams, $location, $rootScope,
   };
 
   const isSettingDirty = function() {
-    // if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
-    //   return false;
-    // } else {
-    return false;
-    // }
+    if (angular.equals($scope.interpreterBindings, $scope.interpreterBindingsOrig)) {
+      return false;
+    } else {
+      return false;
 
 Review comment:
   False is for the case when there's no changes in interpreter binding, then we don't need to invoke the rest api.

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


With regards,
Apache Git Services