Dmitry Lychagin has posted comments on this change. Change subject: Enable HTTP API processing on NCs ......................................................................
Patch Set 12: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java: PS8, Line 51: */ : void handle(IStatementExecutor statementExecutor, MetadataProvider metadataProvider, : IHyracksClientConnection hcc, IHyracksDataset hdc, ResultDelivery resultD > Let's fix this interface so it doesn't change often and breaks the differen I was able to keep the exisitng interface definition. So no changes required here. https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SessionOutput.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SessionOutput.java: PS8, Line 26: : public class SessionOutput { : private final SessionConfig config; > doesn't it make more sense to have the session output as part of the sessio I don't think so. SessionOutput now contains output writer (+decorators) and configuration (SessionConfig) which logically makes sense. SessionConfig is now serializable and can be sent to other nodes. 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 429: if (execStartEnd[0] == -1) { > how about making CcQueryServiceServlet a subclass and remove the statementE yes. I've instead refactored NC-specific code into NCQueryServiceServlet sublcass. QueryServiceServlet will remain as a CC servlet as it used to be. https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ServletConstants.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ServletConstants.java: PS8, Line 25: "; > Can we remove this suffix? ok 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 63: private final ILangExtension.Language lang; : : private final String statementsText; : > Shouldn't this also support non language requests? like other http end poin yes, may be in the future. https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplication.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplication.java: PS8, Line 179: public void startupCompleted() throws Exception { : // configure servlets after joining the cluster, so we can create HyracksClientConnection : configureServers(); : webManager.start(); > This is exactly why you needed to introduce the Thread.sleep(). Let's start no, it doesn't seem so. the failures were in the AsterixInstaller tests and were observed even before this change. besides, the http endpoint is not started on NCs in Asterix. https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-app/src/main/java/org/apache/asterix/messaging/NCMessageBroker.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/messaging/NCMessageBroker.java: PS8, Line 51: int maxMsgSize; > This is a local map. There is no need to use UUID. just use long? yes. fixed now. https://asterix-gerrit.ics.uci.edu/#/c/1709/8/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServer.java: PS8, Line 228: > Why did we remove this? it helps for debugging!! this warning has been moved into HttpServerHandler, so would still be in the logs PS8, Line 88: ic final void start() throws Exceptio > why don't we make this a subclass and leave the existing http server as it Recfactored as you suggested. -- 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: 12 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Till Westmann <[email protected]> Gerrit-Reviewer: Dmitry Lychagin Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
