Michael Blow has posted comments on this change. Change subject: Use Chunked Http Response ......................................................................
Patch Set 5: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/1474/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiServlet.java: Line 89: String accept = replaceIfNull(request.getHeader("Accept"), ""); Refactor replaceIfNull() logic to overloaded IServletRequest.getParameter() method that takes a defaultValue to return in case parameter is not set in the request? Line 120: boolean wrapperArray = format.equals(OutputFormat.CLEAN_JSON) || format.equals(OutputFormat.LOSSLESS_JSON); use == for enum comparison Line 150: break; Why break? Seems like an unimplemented case, perhaps throw something like IllegalStateException("NYI: " + format)? https://asterix-gerrit.ics.uci.edu/#/c/1474/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java: Line 45: public void write(byte[] b, int off, int len) throws IOException { What is the motivation to relax concurrent-ability with this change? This makes chunked less thread safe than full result processing. https://asterix-gerrit.ics.uci.edu/#/c/1474/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java: Line 75: done = false; redundant? Line 126: ctx.write(response, ctx.channel().voidPromise()); What does providing a voidPromise() do? https://asterix-gerrit.ics.uci.edu/#/c/1474/5/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: Line 70: executor = Executors.newFixedThreadPool(16); Why 16 threads? A thread factory should be supplied which names these threads appropriately. https://asterix-gerrit.ics.uci.edu/#/c/1474/5/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: Line 92: server.getExecutor().submit(this); Use an inner class for Callable impl, which takes the ctx as a parameter... https://asterix-gerrit.ics.uci.edu/#/c/1474/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/IServletResponse.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/IServletResponse.java: Line 84: * resume streaming if stopped /resume/Resume/ -- To view, visit https://asterix-gerrit.ics.uci.edu/1474 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249180f58e92058dd3b264ea17c4196b4baf4348 Gerrit-PatchSet: 5 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
