abdullah alamoudi has posted comments on this change. Change subject: Enable HTTP API processing on NCs ......................................................................
Patch Set 8: (15 comments) Some comments. please look at them carefully. One thing I would suggest is to make the proxy a servlet. it would make things cleaner and ensure no breakage of existing stuff. 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 resultDelivery, IStatementExecutor.ResultMetadata outMetadata, : int resultSetIdCounter) throws HyracksDataException, AlgebricksException; Let's fix this interface so it doesn't change often and breaks the different extensions. What we can do is to have the handle method becomes void handle(IStatementExecutor statementExecutor, MetadataProvider metadataProvider, IRequestContext reqCtx) throws HyracksDataException, AlgebricksException; we would put IHyracksClientConnection hcc, IHyracksDataset hdc, ResultDelivery resultDelivery, IStatementExecutor.ResultMetadata outMetadata, int resultSetIdCounter all in request context. later if we want to add members to the request context, this will not break the extension implementations https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: PS8, Line 64: class ResultMetadata implements Serializable { : private static final long serialVersionUID = 1L; : : private final List<Triple<JobId, ResultSetId, ARecordType>> resultSets = new ArrayList<>(); : : public List<Triple<JobId, ResultSetId, ARecordType>> getResultSets() { : return resultSets; : } : } : : public static class Stats { : private long count; : private long size; : : public long getCount() { : return count; : } : : public void setCount(long count) { : this.count = count; : } : : public long getSize() { : return size; : } : : public void setSize(long size) { : this.size = size; : } : : } Can we take both of those classes out of the interface? 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 session config and not the other way around? 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 (statementExecutorFactory != null) how about making CcQueryServiceServlet a subclass and remove the statementExecutorFactory from the base class completely? 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 +1 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: _ATTR" Can we remove this suffix? 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 points? Should we have instead an interface IUserRequest which is serializable and can be either a query request or some other kind of request? 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 the servers in the start method. We should pass the port and address at that point... 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: Map<UUID, CompletableFuture<IMessage>> futureMap; This is a local map. There is no need to use UUID. just use long? 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); > I was getting frequent failures in asterix-lifecycle/backupRestore/backupRe I pointed out the cause of this. Once it is fixed, there will be no need for this sleep. https://asterix-gerrit.ics.uci.edu/#/c/1709/8/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/ICcApplicationContext.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/ICcApplicationContext.java: PS8, Line 76: public IResourceIdManager getResourceIdManager(); : : public IStorageComponentProvider getStorageComponentProvider(); : : public Object getExtensionManager(); : } Add javadocs? 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!! PS8, Line 88: this.proxyToAddress = proxyToAddress; why don't we make this a subclass and leave the existing http server as it is? https://asterix-gerrit.ics.uci.edu/#/c/1709/8/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java: PS8, Line 76: if (servlet != null) { : submit(ctx, servlet, request); : return; : } : : InetSocketAddress proxyToAddress = server.getProxyToAddress(); : if (proxyToAddress != null) { : if (LOGGER.isLoggable(Level.FINE)) { : LOGGER.fine("Proxy " + request.uri() + " to " + proxyToAddress); : } : proxy(ctx, request, proxyToAddress); : return; : } : : if (LOGGER.isLoggable(Level.WARNING)) { : LOGGER.warning("No servlet for " + request.uri()); : } : respond(ctx, request.protocolVersion(), Htt The proxy handler should be a different implementation. This is taking a clean code and starting to make it harder to follow. No? PS8, Line 143: ProxyHandler(clientChannel)); Not nice! -- 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 <ti...@apache.org> Gerrit-Reviewer: Dmitry Lychagin Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
