Till Westmann has posted comments on this change. Change subject: Use Chunked Http Response ......................................................................
Patch Set 5: (15 comments) 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 68: if (buffer.writableBytes() > 0) { How about if (buffer.writableBytes() <= 0) { flush(); } buffer.writeByte(b); Line 82: response.fullReponse(buffer); Who releases the buffer here? Line 92: int size = buffer.capacity(); Could this be readableBytes instead of 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/ 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? Line 54: * 4. larger than chunkSize, no error. -> header, data, empty response [covered] What does "covered" mean in these comments? 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 remove it or use it? 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/ ? Line 78: if (servlet == null) { Maybe structure the if-then-else as if (servlet == null) { ... } else if (http.method() != HttpMethod.GET && http.method() != HttpMethod.POST) { ... } else { ... } for symmetry? 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/ Line 41: * @throws Exception Fix the javadoc (descriptions, the correct exception, ..)? Line 49: * @throws Exception Fix the javadoc (descriptions, the correct exception, ..)? Line 57: * @throws Exception Fix the javadoc (descriptions, the correct exception, ..)? Line 59: ChannelFuture future() throws IOException; Could be have a different name for this method? It sounds like an accessor, but it actually seems to have in important role in the lifecycle of a response. Does this question make sense? Line 84: * resume streaming if stopped > /resume/Resume/ The description is a little terse. Looking at the API I don't see what is streamed, how it is stopped, and why is needs to be resumed. Maybe we could add some description? Also, it would be nice to cluster the methods by instance vs class and to order them in the way they are used during the object lifecycle. -- 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
