Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgraded to Netty 4 #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Upgraded to Netty 4 #14

wants to merge 2 commits into from

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Jun 26, 2015

In addition, made the following changes to the way the handler works:

  • Messages are buffered in a hash map by sequence ID if it's not the right time for the sequence.
  • Removed subsequence ordering - writes for a given request are required to be sequential without the pipelining handler, so with it we can assume the same.
  • Replaced buffer limit with back pressure - if the number of in flight requests exceeds a high water mark, the handler now exerts back pressure on the client by blocking read completed events, and unblocks them, and requests a new read, when a low watermark is reached.

@huntc
Copy link
Contributor

huntc commented Jun 26, 2015

Wow, almost a complete re-write!

Nice job.

@jroper
Copy link
Contributor Author

jroper commented Jun 26, 2015

Yes, Netty 4 is very different to Netty 3 - but also implementing back pressure rather than buffer limits meant it changed significantly.

@huntc
Copy link
Contributor

huntc commented Jun 26, 2015

Yep, I've heard that backpressure is a good thing. ;-)

}
}
});
private final Map<Integer, List<WriteMessage>> bufferedEvents = new HashMap<>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@He-Pin
Copy link

He-Pin commented Jun 26, 2015

really complete rewrite ,so seems like the architecture keep the same ,and the akka stream will work on the HttpMessage level.
great work.

@jroper
Copy link
Contributor Author

jroper commented Jun 26, 2015

Added travis, and it's now passing.

/**
* The number of requests in flight
*/
private final AtomicInteger inFlight = new AtomicInteger();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inFlight should use int
channel is only one to an event loop,and an event loop is backend by a thread,and the bind between channel<->eventloop will keep during the eventloop's lifetime.so I think it's threadsafe.why AtomicInteger?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I did not know that Netty 4 had a new thread model that made things thread safe. That should let me simplify this somewhat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated - I actually removed inFlight since it can be determined from receive sequence and current sending sequence.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@He-Pin
Copy link

He-Pin commented Jun 26, 2015

@normanmaurer would you mind help a little here.Netty3 to Netty4 is a big jump. thanks very much.

@jroper jroper force-pushed the netty-4 branch 2 times, most recently from 8aca1f9 to 268f498 Compare June 26, 2015 06:09
In addition, made the following changes to the way the handler works:

* Messages are buffered in a hash map by sequence ID if it's not the
  right time for the sequence.
* Removed subsequence ordering - writes for a given request are required
  to be sequential without the pipelining handler, so with it we can
  assume the same.
* Replaced buffer limit with back pressure - if the number of in flight
  requests exceeds a high water mark, the handler now exerts back
  pressure on the client by blocking read completed events, and unblocks
  them, and requests a new read, when a low watermark is reached.
@ghost
Copy link

ghost commented Sep 7, 2016

Apparently you need to add channelActive() override and set inactive=false, otherwise pipelining test fails because writeInSequence never puts messages into buffer thinking the channel is inactive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants