You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by charsyam <gi...@git.apache.org> on 2015/10/17 18:50:14 UTC
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
GitHub user charsyam opened a pull request:
https://github.com/apache/tajo/pull/829
TAJO-1936: NPE when calling the query result REST api
Currently, Tajo Rest API return NPE when sessionId is null.
because of ConcurrentHashMap's put method throws when value is null.
So. I changed NPE to BadRequestExeception when session id is null.
Thanks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/charsyam/tajo feature/TAJO-1936
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/829.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #829
----
commit 8aad801454ce710c694a5f3bdcaba0aac7d96cc9
Author: charsyam <ch...@charsyamui-macbook-pro.local>
Date: 2015-10-17T16:21:08Z
TAJO-1936: Fix NPE
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by charsyam <gi...@git.apache.org>.
Github user charsyam commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149257375
@jihoonson I applied your review Thanks.
and Junit expected really make it simple :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149114797
Thanks for understanding.
I will review as soon as possible after you update the patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/829#discussion_r42337101
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/ws/rs/resources/TestQueryResultResource.java ---
@@ -151,6 +155,25 @@ public void testGetQueryResult() throws Exception {
}
@Test
+ public void testGetQueryResultWithoutSessionId() throws Exception {
+ String sessionId = generateNewSessionAndGetId();
+ URI queryIdURI = sendNewQueryResquest(sessionId, "select * from lineitem");
+ URI queryResultURI = new URI(queryIdURI + "/result");
+
+ boolean isBadRequest = false;
+ try {
+ GetQueryResultDataResponse response = restClient.target(queryResultURI)
+ .request()
+ .get(new GenericType<>(GetQueryResultDataResponse.class));
+ } catch (BadRequestException e) {
+ isBadRequest = true;
--- End diff --
How about using expected exception of junit? It will make test codes simpler.
Please refer to test cases in TestCatalogAdminClientExceptions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by charsyam <gi...@git.apache.org>.
Github user charsyam closed the pull request at:
https://github.com/apache/tajo/pull/829
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/829#discussion_r42447128
--- Diff: tajo-dist/src/main/conf/tajo-env.sh ---
@@ -22,10 +22,10 @@
# remote nodes.
# Hadoop home. Required
-# export HADOOP_HOME=
+export HADOOP_HOME=/Users/charsyam/hadoop
--- End diff --
It seems your mistake.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by charsyam <gi...@git.apache.org>.
Github user charsyam commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149112007
@jihoonson
Thanks for your review :)
I will apply your advice.
and I also agree with you. We need more failure testcases :)
I will add more failure testcases for REST-API in my space time :)
Thanks :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149448719
+1 LGTM!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149396665
Thanks for quick update.
It seems that some unnecessary codes are included mistakenly.
Would you mind cleaning them up?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by charsyam <gi...@git.apache.org>.
Github user charsyam commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149115162
@jihoonson maybe this tonight :) I will upload patch :) Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/829#discussion_r42447157
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/ws/rs/resources/TestQueryResultResource.java ---
@@ -19,6 +19,8 @@
package org.apache.tajo.ws.rs.resources;
import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
--- End diff --
Please remove unused imports.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149104304
Hi @charsyam, thanks for your work.
It looks nice to me. I left very trivial comments.
Anyway, as you already know, our REST api lacks methods to test failure cases. So, IMHO, it would be great if more methods to test various failure cases are added like ```testGetQueryResultWithoutSessionId()``` you already added in this patch.
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/829#discussion_r42337075
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/ws/rs/resources/TestQueryResultResource.java ---
@@ -58,6 +61,7 @@
import static org.junit.Assert.*;
public class TestQueryResultResource extends QueryTestCaseBase {
+ private static final Log LOG = LogFactory.getLog(TestQueryResultResource.class);
--- End diff --
It seems not be used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149755145
@charsyam, would you please close this pr?
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-1936: NPE when calling the query result RE...
Posted by charsyam <gi...@git.apache.org>.
Github user charsyam commented on the pull request:
https://github.com/apache/tajo/pull/829#issuecomment-149447966
@jihoonson Thanks for your kind advice :)
I applied your review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---