abdullah alamoudi has posted comments on this change. Change subject: Use Chunked Http Response ......................................................................
Patch Set 5: (23 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() Done Line 120: boolean wrapperArray = format.equals(OutputFormat.CLEAN_JSON) || format.equals(OutputFormat.LOSSLESS_JSON); > use == for enum comparison Done Line 150: break; > Why break? Seems like an unimplemented case, perhaps throw something like Done 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 Done Line 68: if (buffer.writableBytes() > 0) { > How about Done Line 82: response.fullReponse(buffer); > Who releases the buffer here? In Netty, the design is that the last one to use a buffer releases it. Since the full response is passed to the network channel, it will be released once it goes out the network Line 92: int size = buffer.capacity(); > Could this be readableBytes instead of capacity? We use the size to allocate a new buffer. Hence, we are using the capacity. 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 45: * If the response status is non 200, then output bufferred before setting the response status is discarded. > s/bufferred/buffered/ Done Line 46: * If flush() is called on the writer and even if it is less than chunkSize, then the response will be chunked. > s/chunked/flushed/ at the end of the line? Done Line 54: * 4. larger than chunkSize, no error. -> header, data, empty response [covered] > What does "covered" mean in these comments? covered by existing test cases. I realize coverage should be determined by tools and so I am removing it. Line 75: done = false; > redundant? Done Line 126: ctx.write(response, ctx.channel().voidPromise()); > What does providing a voidPromise() do? When we're not interested in listening to future (about the async call), we can provide a void promise to avoid object creation. If a promise is not provided, then netty will create one which could lead to triggering the GC frequently. https://asterix-gerrit.ics.uci.edu/#/c/1474/5/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java: Line 98: public void resume() { > It seems that this class isn't used anymore.Is that right? If so, should we That is correct. I made sure the tests still pass with it. and I could see some kind of use cases for this. For example for servers which will always return small responses (JSON API?) 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 thre Hmmmm, 16 concurrent requests seemed reasonable but I think this should be eventually configurable. 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 75: FullHttpRequest http = (FullHttpRequest) msg; > s/http/httpReq/ ? Done Line 78: if (servlet == null) { > Maybe structure the if-then-else as Done Line 92: server.getExecutor().submit(this); > Use an inner class for Callable impl, which takes the ctx as a parameter... Done 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 31: * A response to an instance of IServLetRequest > s/IServLetRequest/IServletRequest/ Done Line 41: * @throws Exception > Fix the javadoc (descriptions, the correct exception, ..)? Done Line 49: * @throws Exception > Fix the javadoc (descriptions, the correct exception, ..)? Done Line 57: * @throws Exception > Fix the javadoc (descriptions, the correct exception, ..)? Done Line 59: ChannelFuture future() throws IOException; > Could be have a different name for this method? It sounds like an accessor, Done Line 84: * resume streaming if stopped > The description is a little terse. Looking at the API I don't see what is s Done -- 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-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
