Add a backpressure mechanism for channel writes #383
Conversation
* Checks the status of `Channel.isWritable()` before attempting to write a notification to the channel. If `isWritable()` returns false, return a new failed future with a cause of `ClientBusyException`. The presence of this exception is a signal to back off write attempts until the underlying channel has sucessfully returned to its low water mark * Adds `ApnsClientBuilder#setChannelWriteBufferWatermark` for setting the channel watermark levels * Adds `ApnsClientBuilder#setByteBufferAllocator` for setting the buffer allocator of the netty channel
|
On the whole, this looks good to me! I'll give it a more detailed read as soon as I can (which will most likely be Friday). Thanks very much for the contribution! |
|
Thanks for taking a look. I just noticed that you have a 0.8.2 release milestone planned. Mind if update to target that release instead(just changing the @SInCE annotations in the comments)? |
|
@stepheno I went back and forth on this a few times, but I think it makes sense to include in 0.8.2. |
There was a problem hiding this comment.
With sincere apologies for the long delay on reviewing this in detail, this looks good to me from a technical perspective. I think we might want to polish the docs just a little but more, and as you pointed out, change the @since version. Otherwise, I think we're good!
Thanks very much for the contribution!
| * | ||
| * @param message a short, human-readable explanation of the cause of this exception | ||
| */ | ||
| public ClientBusyException(final String message) { |
There was a problem hiding this comment.
It looks like we never use this constructor; could we just drop it? If all we're left with is the default passthrough constructor, we could probably drop that, too.
| } | ||
|
|
||
| /** | ||
| * Sets the buffer usage watermark range for the underlying netty channel |
There was a problem hiding this comment.
Can we be a little more descriptive here, too? This seems like a good place to mention the existence of ClientBusyException, for example.
| @@ -349,6 +354,34 @@ public ApnsClientBuilder setGracefulShutdownTimeout(final long gracefulShutdownT | |||
| } | |||
|
|
|||
| /** | |||
| * Sets the buffer allocator for the underlying netty channel | |||
There was a problem hiding this comment.
Can we be a little more descriptive here and maybe explain why a user would want to do this, or what the consequences might be? Also, as a super minor thing, mind adding a period to the end of the sentence? ;)
|
Actually, @stepheno, the things I'm asking for here are pretty minor. I'll go ahead and make the doc changes myself. Please consider yourself off the hook. I'll cobble a thing together and merge/close this shortly. Thanks again! |
|
Hey Jon,
Sorry for the slow response, been a bit busy at work this week. Thanks for
updating the PR.
Also, looking forward to the upcoming token auth changes.
Cheers
…On Tue, Dec 13, 2016 at 2:51 PM, Jon Chambers ***@***.***> wrote:
Actually, @stepheno <https://github.com/stepheno>, the things I'm asking
for here are pretty minor. I'll go ahead and make the doc changes myself.
Please consider yourself off the hook. I'll cobble a thing together and
merge/close this shortly. Thanks again!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAOZMSif62dOw5CInpPJ53jx96IQnQjzks5rHyFpgaJpZM4KyJDT>
.
|
|
Hey, @stepheno, can you tell me more about the use case for setting a custom byte buffer allocator? To my knowledge, the only options are "pooled" and "unpooled," and those can be selected globally by setting the java -Dio.netty.allocator.type=pooled -jar totally-sweet-push-notification-thing.jarDo you have a use case in mind where somebody (a) would need a non-default allocator and (b) would need a different one than the JVM-wide default? To be clear, I'm totally open to this, but I want to double-check that the need isn't already met by the |
|
Hey @jchambers, you're right about the global |
|
And we're good. I did the cleanup stuff in 5882679; please let me know if anything looks amiss to you. Cheers! |
|
Er… by which I actually mean 1e221a3. |
Channel.isWritable()before attempting to write anotification to the channel. If
isWritable()returns false, return a new failedfuture with a cause of
ClientBusyException. The presence of this exception isa signal to back off write attempts until the underlying channel has sucessfully returned
to its low water mark
ApnsClientBuilder#setChannelWriteBufferWatermarkfor settingthe channel watermark levels
ApnsClientBuilder#setByteBufferAllocatorfor setting the bufferallocator of the netty channel