Till Westmann has posted comments on this change. Change subject: Replace Servlets with Netty Based HTTP Servers ......................................................................
Patch Set 1: (24 comments) A few comments. Also, could we move the framework to a new hyracks module hyracks-http? https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ApiLet.java: Line 103: ILetResponse.setCharacterEncoding(response, ILetResponse.TEXT_HTML, ILetResponse.ENCODING_UTF8); change this method name to setContentType Line 143: ResultUtil.webUIErrorHandler(response.writer(), e); reintroduce "out" variable? https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ConnectorApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ConnectorApiLet.java: Line 45: public class ConnectorApiLet extends AbstractLet { Bring back the class comment. Line 51: private void formResponseObject(ObjectNode jsonResponse, FileSplit[] fileSplits, ARecordType recordType, Move this method down to reduce diff size. https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/FullLetResponse.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/FullLetResponse.java: Line 64: String txt = new String(baos.toByteArray(), StandardCharsets.UTF_8); unused. Line 66: fullResponse.headers().setInt(HttpHeaderNames.CONTENT_LENGTH, fullResponse.content().readableBytes()); Check if the HTTP spec is ok not having a length? Or does netty set it? https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/GetLetRequest.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/GetLetRequest.java: Line 42: List<String> parameter = parameters.get(name); Share code with PostLetRequest. https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServer.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServer.java: Line 73: Collections.sort(lets, (l1, l2) -> l2.getPaths()[0].length() - l1.getPaths()[0].length()); Does this work? Is the goal to make sure that we access handlers in the right order or just to ensure consistency? https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServerHandler.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServerHandler.java: Line 69: HttpResponseStatus.NOT_FOUND); Should this be "method not alowed"? Line 76: let.handle(request, response); Should we just catch Throwable here and set the response code to 500? That we we could catch every handler that still throws exceptions ... https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServerInitializer.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/HttpServerInitializer.java: Line 39: p.addLast(new HttpRequestDecoder(131072, 262144, 262144)); Add/document these constants. https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILet.java: Line 28: void handle(ILetRequest request, ILetResponse response) throws Exception; Can we have a specific or no exception here? Would that make sense? https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILetResponse.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ILetResponse.java: Line 36: public static final String ENCODING_UTF8 = "utf-8"; Pull those out into a ContentType and an Encoding? Maybe netty has those already? Line 54: String getHeader(CharSequence name) throws IOException; Why do we need to get headers? I think that we remove it. Line 80: OutputStream outputStream(); Do we need an OutputStream and a Writer? If we keep them, could be put the next to each other? Line 82: public static void setCharacterEncoding(ILetResponse response, String type, String charset) throws IOException { rename to setContentType https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryWebInterfaceLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryWebInterfaceLet.java: Line 31: import org.apache.commons.logging.LogFactory; Use a different logging framework? Line 57: public void doGet(ILetRequest request, ILetResponse response) throws IOException { Should this be protected or private? Line 97: public void doPost(ILetRequest request, ILetResponse response) throws IOException { Should this be protected or private? Also, this post handler is strange - I guess this is for the GSoC query UI? Line 111: response.setStatus(HttpResponseStatus.INTERNAL_SERVER_ERROR); This could be handled in the framework? Line 122: public static String mime(String extension) { Pull this out into a ContentType utility? https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UpdateApiLet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/UpdateApiLet.java: Line 36: protected String getErrorMessage() { Move this down? https://asterix-gerrit.ics.uci.edu/#/c/1429/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java: Line 86: private List<HttpServer> nettyServers; Would it make sense to encapsulate the Netty boilerplate? Line 222: protected ILet createLet(HttpServer server, Lets key, String... paths) { Move this down? -- To view, visit https://asterix-gerrit.ics.uci.edu/1429 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3d552d4eb8c868535ca4c41cbcf7e352217b18ae Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
