asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <>
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:

File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/

Line 89:         String accept = replaceIfNull(request.getHeader("Accept"), "");
> Refactor replaceIfNull() logic to overloaded IServletRequest.getParameter()

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 
File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/

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 

Line 68:         if (buffer.writableBytes() > 0) {
> How about

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.
File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/

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?
covered by existing test cases. I realize coverage should be determined by tools and so I
am removing it.

Line 75:         done = false;
> redundant?

Line 126:             ctx.write(response,;
> 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.
File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/

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?)
File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/

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.
File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/

Line 75:             FullHttpRequest http = (FullHttpRequest) msg;
> s/http/httpReq/ ?

Line 78:             if (servlet == null) {
> Maybe structure the if-then-else as

Line 92:                 server.getExecutor().submit(this);
> Use an inner class for Callable impl, which takes the ctx as a parameter...
File hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/

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,

Line 84:      * resume streaming if stopped
> The description is a little terse. Looking at the API I don't see what is s

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I249180f58e92058dd3b264ea17c4196b4baf4348
Gerrit-PatchSet: 5
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <>
Gerrit-Reviewer: Jenkins <>
Gerrit-Reviewer: Michael Blow <>
Gerrit-Reviewer: Till Westmann <>
Gerrit-Reviewer: abdullah alamoudi <>
Gerrit-HasComments: Yes

View raw message