[ZEPPELIN-3014] NPE bug fix and Error message enhancement with Kylin Interpreter #2634

jinxliu · 2017-10-27T07:04:45Z

What is this PR for?

A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://zeppelin.apache.org/contribution/contributions.html

What type of PR is it?

Bug Fix

Todos

- Task

What is the Jira issue?

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

How should this be tested?

Setup Travis CI as described on https://zeppelin.apache.org/contribution/contributions.html#continuous-integration
Use existing unit tests in kylin module.

Screenshots (if appropriate)

before: NPE when result set is empty

after: no NPE when result set is empty, just an empty table

before: when query fails, only error code is returned, no error message

after: when query fails, both error code and error message are displayed to users

Questions:

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

jinxliu · 2017-10-27T07:06:29Z

This PR has two commits, ec68d12 is to enhance the error message parse and display, the second commit is d20872c for NPE bug fix when returned dataset is empty.

merge recent changes

tinkoff-dwh · 2017-10-27T11:33:26Z

@jinxliu
read https://zeppelin.apache.org/contribution/contributions.html (Creating a Pull Request)

felixcheung · 2017-10-27T16:31:18Z

can you update the PR title, description and see https://zeppelin.apache.org/contribution/contributions.html

felixcheung · 2017-10-27T16:33:50Z

kylin/pom.xml

        </dependency>
+        <dependency>
+            <groupId>org.json</groupId>
+            <artifactId>json</artifactId>


is this a new dependency? if so, it should be added to the LICENSE file

zeppelin-interpreter has dependecy Gson

gson yes, and this is org.json? I didn't see it in the LICENSE file

i mean need to use Gson instead of org.json

Thanks very much @tinkoff-dwh and @felixcheung for your comments. I was aware that zeppelin already has the dependency of gson, so is it mandatory to use gson instead of json?

not necessarily. each interpreter can use its own set of things. but new binary dependencies we need to list in LICENSE file for the release.

@felixcheung I have replaced json with gson and I will push the code soon.

felixcheung · 2017-10-27T16:34:45Z

kylin/src/main/java/org/apache/zeppelin/kylin/KylinInterpreter.java

+      StringBuilder errorMessage = new StringBuilder("Failed : HTTP error code " + code);
+      logger.error("failed to execute query: " + result);
+      try {
+        JSONObject content = new JSONObject(result);


what if result is an empty string?

If result is empty string, depending on the response code:
if the code is 200, an empty table will be rendered;
if the code is not 200, there will be error message, with status code, but no detailed error message, e.g. "Failed : HTTP error code 500. Error message: "

felixcheung · 2017-10-27T16:36:25Z

kylin/src/main/java/org/apache/zeppelin/kylin/KylinInterpreter.java

+          errorMessage.append(". Error message: Unauthorized. This request requires "
+              + "HTTP authentication. Please make sure your have set your credentials correctly.");
+        } else {
+          errorMessage.append(". Error message: " + result);


previously we were formatting the output (see L186) before setting it to InterpreterResult - do we still need to do that here?

@felixcheung yes we still need to do that. The code here is to parse the result and get detailed error message from kylin when query fails. The formatting result will be called when the response code is 200 and query successes.

actually, to clarify I was referring to

String output; - logger.info("Output from Server .... \n"); - while ((output = br.readLine()) != null) { - logger.info(output); - sb.append(output).append('\n'); } - InterpreterResult rett = new InterpreterResult(InterpreterResult.Code.SUCCESS, - formatResult(sb.toString()));

does it still need to read the result line by line like this from before, before passing to formatResult()?

@felixcheung no we do not need to do that. I replace the line by line processing with the following statement: result = IOUtils.toString(response.getEntity().getContent(), "UTF-8")

… kylin-intp

keep up to date with apache master branch

jinxliu · 2017-11-01T07:29:33Z

@felixcheung is there something wrong with travis-ci?

felixcheung · 2017-11-01T07:31:47Z

seems like travis problem. please try to kick it off again by close-reopen this PR

jinxliu · 2017-11-01T07:33:24Z

I have tried several times since yesterday, hope this time it works.

jinxliu · 2017-11-01T07:43:47Z

@felixcheung it still does not work.

zjffdu · 2017-11-01T09:35:54Z

@jinxliu Could you rebase and try again ?

jinxliu · 2017-11-02T06:15:31Z

@zjffdu sure I will close this PR and have created another PR #2645. Please review.

zjffdu · 2017-11-02T06:29:32Z

@jinxliu Actually you don't need to create another PR, for the next time you just need to rebase and force push

enhance kylin interpter, add error message parse

ec68d12

NPE bug fix when returned result set is empty

cadaa2b

jinxliu force-pushed the kylin-intp branch from d20872c to cadaa2b Compare October 27, 2017 07:13

jinxliu changed the title ~~Kylin intp~~ NPE bug fix and Error message enhancement with Kylin Interpreter Oct 27, 2017

Merge pull request #1 from apache/master

c1ec9e7

merge recent changes

felixcheung reviewed Oct 27, 2017

View reviewed changes

jinxliu changed the title ~~NPE bug fix and Error message enhancement with Kylin Interpreter~~ [ZEPPELIN-3014] NPE bug fix and Error message enhancement with Kylin Interpreter Oct 30, 2017

jinxliu added 3 commits October 31, 2017 13:45

use gson

933c9d8

Merge branch 'kylin-intp' of https://github.com/jinxliu/zeppelin into…

a45a3b4

… kylin-intp

Merge pull request #2 from apache/master

531318c

keep up to date with apache master branch

jinxliu closed this Nov 1, 2017

jinxliu reopened this Nov 1, 2017

jinxliu closed this Nov 2, 2017

[ZEPPELIN-3014] NPE bug fix and Error message enhancement with Kylin Interpreter #2634

[ZEPPELIN-3014] NPE bug fix and Error message enhancement with Kylin Interpreter #2634

Uh oh!

Conversation

jinxliu commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

before: NPE when result set is empty

after: no NPE when result set is empty, just an empty table

before: when query fails, only error code is returned, no error message

after: when query fails, both error code and error message are displayed to users

Questions:

Uh oh!

jinxliu commented Oct 27, 2017

Uh oh!

tinkoff-dwh commented Oct 27, 2017

Uh oh!

felixcheung commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinxliu commented Nov 1, 2017

Uh oh!

felixcheung commented Nov 1, 2017

Uh oh!

jinxliu commented Nov 1, 2017

Uh oh!

jinxliu commented Nov 1, 2017

Uh oh!

zjffdu commented Nov 1, 2017

Uh oh!

jinxliu commented Nov 2, 2017

Uh oh!

zjffdu commented Nov 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jinxliu commented Oct 27, 2017 •

edited

Loading

felixcheung commented Oct 27, 2017 •

edited

Loading