asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Use Chunked Http Response
Date Tue, 31 Jan 2017 22:40:41 GMT
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 <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message