[ZEPPELIN-1069]Ignore implicit interpreter when user enter wrong interpreter name #1248

mwkang · 2016-07-30T05:51:56Z

What is this PR for?

Ignore implicit interpreter when user enter wrong interpreter name
linked to #806 (comment)

This PR is related to #1113
ZEPPELIN-1069 branch was force push, so couldn't reopen that PR.

What type of PR is it?

Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1069

How should this be tested?

Unit test
Run-time checking

Screenshots (if appropriate)

Questions:

Does the licenses files need update? no
Is there breaking changes for older versions? no
Does this needs documentation? no

mwkang · 2016-08-02T03:17:25Z

@jongyoul Can you review this PR?

bzz · 2016-08-02T09:55:43Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java

@@ -1088,7 +1088,10 @@ private InterpreterSetting getDefaultInterpreterSetting(List<InterpreterSetting>
  public InterpreterSetting getDefaultInterpreterSetting(String noteId) {
    return getDefaultInterpreterSetting(getInterpreterSettings(noteId));
  }
-
+
+  public boolean isBinding(String noteId, String replName) {


Would isBinded() make more sense as a name for a method that checks weather a particular note is binded (bound) to a particular interpreter?

Yes, I agree with you. Thanks for comments.
I will change method name.

@bzz ~~I want to ask your opinion. If I make Note#isBinding method, I remove InterpreterFactory#isBinded method. Do you agree with me?~~
Um.. Sorry I thinks that Keeping both methods is better.

bzz · 2016-08-02T10:07:13Z

👍 for having a test!

Looks good to me, modulo the issue raised above. @mwkang can you take a look please?

astroshim · 2016-08-02T11:45:04Z

tested. LGTM

mwkang · 2016-08-03T04:50:46Z

I refactor method name.

bzz · 2016-08-03T09:41:01Z

@bzz ~~Thanks for comment, But could you explain more why it doesn't make sense?~~
Ah.. Your meaning is that org.apache.zeppelin.interpreter.InterpreterFactory#isBinded move to Note.java. Is it right?

Yes, that is exactly what I meant @mwkang !
And while doing so, readability of the piece of the code you are changing could also be improved with static import.

What do you think?

mwkang · 2016-08-03T09:54:23Z

@bzz I'm worried about that static import confused.

mwkang · 2016-08-05T01:29:55Z

@bzz I changed StringUtils is static import. Can you check the code?

jongyoul · 2016-08-05T05:39:50Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java

@@ -17,6 +17,8 @@

 package org.apache.zeppelin.notebook;

+import static org.apache.commons.lang.StringUtils.*;
+


static import statement is located at the end of all imports statements. Could you please move it?

@jongyoul Oh sorry, I move static import position.
Thanks for review.

jongyoul · 2016-08-08T06:04:09Z

Do you check the error of CI?

Failed tests: 
  NoteTest.insertParagraphWithLastReplName:159 expected:<%spark > but was:<null>
  NoteTest.addParagraphWithLastReplName:144 expected:<%spark > but was:<null>

I think it's meaningful error and it needs to be fixed.

mwkang · 2016-08-08T11:01:14Z

@jongyoul Sorry, I missed it. I will check. I appreciate your review.

mwkang · 2016-08-09T00:43:06Z

re-trigger CI

mwkang · 2016-08-09T04:16:51Z

I am not sure this error is related with this issue.

Failed tests: 
  ZeppelinSparkClusterTest.zRunTest:204 expected:<FINISHED> but was:<ERROR>

bzz · 2016-08-09T06:39:55Z

@mwkang thank you for prompt addressing reviews!

If you re-trigger CI more (close&re-open this PR) - does the same issue persists every time?

bzz · 2016-08-09T06:41:42Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java

@@ -1088,7 +1088,10 @@ private InterpreterSetting getDefaultInterpreterSetting(List<InterpreterSetting>
  public InterpreterSetting getDefaultInterpreterSetting(String noteId) {
    return getDefaultInterpreterSetting(getInterpreterSettings(noteId));
  }
-
+
+  public boolean isBinded(String noteId, String replName) {


I think this can be removed now, in favor of Note.isBinded(), no?

Yes, I will remove it.

mwkang · 2016-08-09T11:30:10Z

If you re-trigger CI more (close&re-open this PR) - does the same issue persists every time?

I'm not sure if it persists. I will re-tigger after fix the issue.

mwkang · 2016-08-11T03:53:41Z

@bzz No, It does not persists every time.

bzz · 2016-08-11T04:44:02Z

Ok, good to know, let me look into that

bzz · 2016-08-11T13:51:36Z

This looks like an issue fixed in master 10h ago:

�[31m- should provide onclick method *** FAILED ***�[0m
�[31m  The code passed to eventually never returned normally. Attempted 1 times over 231.694296 milliseconds. Last failure message: 0 was not equal to 1. (AbstractAngularElemTest.scala:72)�[0m

Could you please try rebasing on latest master git pull ; git rebase master; git push -f ?

mwkang · 2016-08-11T14:29:13Z

@bzz Thanks for your review. I rebase on to master and force push.

bzz · 2016-08-12T03:53:38Z

Thank you! BTW in my experience, if in case of CI failure PR author posts the failure result from logs - that speeds us review a lot!

CI fails now on Spark 1.6 Scala 2.10 and Scala 2.11 profiles with

[INFO] Zeppelin: web Application .......................... FAILURE [01:31 min]
[INFO] make: Leaving directory `/home/travis/build/apache/zeppelin/zeppelin-web/node_modules/karma/node_modules/socket.io/node_modules/socket.io-client/node_modules/ws/build'
[ERROR] npm ERR! Linux 3.13.0-40-generic
[ERROR] npm ERR! argv "/home/travis/build/apache/zeppelin/zeppelin-web/node/node" "/home/travis/build/apache/zeppelin/zeppelin-web/node/node_modules/npm/bin/npm-cli.js" "install" "--color=false"
[ERROR] npm ERR! node v0.12.13
[ERROR] npm ERR! npm  v2.15.0
[ERROR] npm ERR! code ECONNRESET
[ERROR] npm ERR! errno ECONNRESET
[ERROR] npm ERR! syscall read
[ERROR] 
[ERROR] npm ERR! network read ECONNRESET
[ERROR] npm ERR! network This is most likely not a problem with npm itself
[ERROR] npm ERR! network and is related to network connectivity.
[ERROR] npm ERR! network In most cases you are behind a proxy or have bad network settings.
[ERROR] npm ERR! network 
[ERROR] npm ERR! network If you are behind a proxy, please make sure that the
[ERROR] npm ERR! network 'proxy' config is set properly.  See: 'npm help config'

Which seems to be networking issue on Travis side 😒

Thank you @mwkang for contribution, looks great to me, merging to master if there is no further discussion.

mwkang · 2016-08-13T04:42:51Z

@bzz I think This PR have some trouble merging to master.
Because When bzz leave comment, I also leave comment immediately.
So bot did not recognize this PR.
Could you mind if you check again? (I'm worried about this PR open forever.)

bzz · 2016-08-16T08:31:14Z

@mwkang thank you for friendly ping!

bzz · 2016-08-16T08:33:29Z

zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/Note.java

@@ -56,6 +55,8 @@
 import org.apache.zeppelin.user.AuthenticationInfo;
 import org.apache.zeppelin.user.Credentials;

+import static org.apache.commons.lang.StringUtils.*;


As per project styleguide#s3.3.1-wildcard-imports - could you please replace whiledcard imports?

@bzz I'm sorry. I removed wild card import.

bzz · 2016-08-16T08:35:14Z

Thank you for your patience @mwkang !
Sorry for the late notice, but could you please take a look and make sure that code conforms the project styleguilde?

Will merge to master asap, right after that, if there is no further discussion!

mwkang · 2016-08-20T06:06:47Z

ping @bzz

bzz · 2016-09-02T07:46:02Z

I'm sorry for delay, @mwkang - my plate was full last week.
Thank you for contributing and addressing feedback promptly, merging it in, as there is no further discussion.

bzz reviewed Aug 2, 2016
View reviewed changes

jongyoul reviewed Aug 5, 2016
View reviewed changes

mwkang closed this Aug 9, 2016

mwkang reopened this Aug 9, 2016

bzz reviewed Aug 9, 2016
View reviewed changes

mwkang added 7 commits August 11, 2016 23:24

Add function that is replName is binding.

b6520be

Add test cases

4912c5c

Refactor method name

02a1c2a

Modify that StringUtils is static import

6082e28

Modify static import position.

046f5f4

Fixed CI

dd60d72

Remove unnecessary method.

6e91dfe

mwkang added 2 commits August 11, 2016 23:24

Remove unnecessary method.

f0eedeb

Fix CI failed

fa7a194

mwkang force-pushed the ZEPPELIN-1069 branch from 5211079 to fa7a194 Compare August 11, 2016 14:26

bzz reviewed Aug 16, 2016
View reviewed changes

Remove wild card import

3976a96

asfgit closed this in b89e35e Sep 2, 2016

minahlee mentioned this pull request Sep 2, 2016

[ZEPPELIN-1399] Refactor automatic interpreter name insert #1388

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-1069]Ignore implicit interpreter when user enter wrong interpreter name #1248

[ZEPPELIN-1069]Ignore implicit interpreter when user enter wrong interpreter name #1248

mwkang commented Jul 30, 2016

mwkang commented Aug 2, 2016

bzz Aug 2, 2016 •

edited

mwkang Aug 3, 2016

mwkang Aug 3, 2016 •

edited

bzz commented Aug 2, 2016 •

edited

astroshim commented Aug 2, 2016

mwkang commented Aug 3, 2016

bzz commented Aug 3, 2016

mwkang commented Aug 3, 2016

mwkang commented Aug 5, 2016

jongyoul Aug 5, 2016

mwkang Aug 5, 2016

jongyoul commented Aug 8, 2016

mwkang commented Aug 8, 2016

mwkang commented Aug 9, 2016

mwkang commented Aug 9, 2016

bzz commented Aug 9, 2016

bzz Aug 9, 2016

mwkang Aug 9, 2016

mwkang commented Aug 9, 2016 •

edited

mwkang commented Aug 11, 2016

bzz commented Aug 11, 2016

bzz commented Aug 11, 2016

mwkang commented Aug 11, 2016

bzz commented Aug 12, 2016 •

edited

mwkang commented Aug 13, 2016

bzz commented Aug 16, 2016

bzz Aug 16, 2016

mwkang Aug 17, 2016 •

edited

bzz commented Aug 16, 2016

mwkang commented Aug 20, 2016

bzz commented Sep 2, 2016 •

edited

		@@ -17,6 +17,8 @@

		package org.apache.zeppelin.notebook;

		import static org.apache.commons.lang.StringUtils.*;

[ZEPPELIN-1069]Ignore implicit interpreter when user enter wrong interpreter name #1248

[ZEPPELIN-1069]Ignore implicit interpreter when user enter wrong interpreter name #1248

Conversation

mwkang commented Jul 30, 2016

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

mwkang commented Aug 2, 2016

bzz Aug 2, 2016 • edited

Choose a reason for hiding this comment

mwkang Aug 3, 2016

Choose a reason for hiding this comment

mwkang Aug 3, 2016 • edited

Choose a reason for hiding this comment

bzz commented Aug 2, 2016 • edited

astroshim commented Aug 2, 2016

mwkang commented Aug 3, 2016

bzz commented Aug 3, 2016

mwkang commented Aug 3, 2016

mwkang commented Aug 5, 2016

jongyoul Aug 5, 2016

Choose a reason for hiding this comment

mwkang Aug 5, 2016

Choose a reason for hiding this comment

jongyoul commented Aug 8, 2016

mwkang commented Aug 8, 2016

mwkang commented Aug 9, 2016

mwkang commented Aug 9, 2016

bzz commented Aug 9, 2016

bzz Aug 9, 2016

Choose a reason for hiding this comment

mwkang Aug 9, 2016

Choose a reason for hiding this comment

mwkang commented Aug 9, 2016 • edited

mwkang commented Aug 11, 2016

bzz commented Aug 11, 2016

bzz commented Aug 11, 2016

mwkang commented Aug 11, 2016

bzz commented Aug 12, 2016 • edited

mwkang commented Aug 13, 2016

bzz commented Aug 16, 2016

bzz Aug 16, 2016

Choose a reason for hiding this comment

mwkang Aug 17, 2016 • edited

Choose a reason for hiding this comment

bzz commented Aug 16, 2016

mwkang commented Aug 20, 2016

bzz commented Sep 2, 2016 • edited

bzz Aug 2, 2016 •

edited

mwkang Aug 3, 2016 •

edited

bzz commented Aug 2, 2016 •

edited

mwkang commented Aug 9, 2016 •

edited

bzz commented Aug 12, 2016 •

edited

mwkang Aug 17, 2016 •

edited

bzz commented Sep 2, 2016 •

edited