asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dmitry Lychagin (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Enable HTTP API processing on NCs
Date Tue, 09 May 2017 17:57:50 GMT
Dmitry Lychagin has posted comments on this change.

Change subject: Enable HTTP API processing on NCs
......................................................................


Patch Set 8:

(4 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java:

PS8, Line 468:                     if (err instanceof Exception) {
             :                         throw (Exception) err;
             :                     } else if (err instanceof TokenMgrError) {
             :                         throw (TokenMgrError) err;
             :                     } else if (err instanceof org.apache.asterix.aqlplus.parser.TokenMgrError)
{
             :                         throw (org.apache.asterix.aqlplus.parser.TokenMgrError)
err;
             :                     } else {
             :                         throw new Exception(err.toString());
             :                     }
> why can't we just throw err here, and update catch below (line 493) to catc
Yes, looks like we can. This is a minor fix, I'll address this in a separate change.


https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java
File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java:

PS8, Line 147: e.getMessage()
> same as toString comment above
yes. I'll fix it in a separate change.


https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java
File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java:

Line 823:                 Thread.sleep(8000);
> Why do we need to sleep 8s after every managix command now after this chang
I was getting frequent failures in asterix-lifecycle/backupRestore/backupRestore.9.query.aql
both on my machine and on jenkins. The actual result was empty, while the expected result
was not. I'm not sure what's causing this, but this workaround seems to help. We need to look
into fixing this properly, of course.


https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-installer/src/test/java/org/apache/asterix/installer/test/AsterixLifecycleIT.java
File asterixdb/asterix-installer/src/test/java/org/apache/asterix/installer/test/AsterixLifecycleIT.java:

Line 91:             Thread.sleep(4000);
> What problem are we solving with this?  Is stop returning before the cluste
yes, it seems. It looks like NCs were still not stopped at this point. state.getFailedNCs()
was returning 0 leading to assertion failure at line 95. Thread.sleep() is just a temporary
workaround. We should look into fixing this properly.


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1709
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I19414a23e163fc4deef9805c8f9089609f1ebe07
Gerrit-PatchSet: 8
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Dmitry Lychagin
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message