[improve][broker,proxy] Use ChannelVoidPromise to avoid useless promise objects creation by nicoloboschi · Pull Request #19141 · apache/pulsar

nicoloboschi · 2023-01-05T16:01:54Z

Motivation

In the broker and proxy most of the writes callback are not considered. In those cases we can save useless allocations of default channel promises by using the voidPromise() method.

Modifications

Use VoidChannelPromise if the promise result is never checked

Verifying this change

Make sure that the change passes the CI checks.

Documentation

doc
doc-required
doc-not-needed
doc-complete

nodece · 2023-01-06T04:08:47Z

Maybe you should move the writeAndFlushVoidPromise method to pulsar-common module to eliminate the duplication.

nicoloboschi · 2023-01-09T15:52:48Z

@nodece done, PTAL again

lhotari · 2023-01-10T06:24:57Z

Related to #16113.

lhotari

Really great improvement. The only small nag is about naming. writeAndFlushVoidPromise is a very clumsy name. I don't have a good name to suggest immediately, but perhaps we could think about something.

It's not only the "useless promise objects creation" that is the reason to use voidPromise. Another reason to use a ctx.voidPromise() is because it adds proper error handling. Exceptions won't be propagated in Netty 4.x unless the the ChannelFuture's listener handles errors or something like ctx.voidPromise() is used.
(Related to SO Q&A https://stackoverflow.com/q/54169262 and https://stackoverflow.com/a/9030420 .)

I think renaming writeAndFlushVoidPromise in PulsarCommandSender and ServerCnx to writeAndFlush would simplify naming. Renaming writeAndFlushVoidPromise to writeAndFlushWithVoidPromise in the utility class would improve the naming.

lhotari · 2023-01-10T06:56:20Z

+ * Contains utility methods for working with Netty Channels.
+ */
+public class NettyChannelUtil {
+


Add proper javadoc to this method. I'd suggest explaining that besides reducing unnecessary object creation, using ctx.voidPromise() will propagate exceptions properly. Related to SO Q&A https://stackoverflow.com/q/54169262 and https://stackoverflow.com/a/9030420 .

lhotari

Please also replace any occurrences of .addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE) with the use of ctx.voidPromise() solution. I realised that I had added .addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE) in #14713 based on SO answer https://stackoverflow.com/a/54377238 . (I added a new answer to SO with the voidPromise solution.)

…se objects creation

nicoloboschi · 2023-01-10T10:38:50Z

Thanks @lhotari
I've renamed the methods and javadoc as you suggested. I've also replaced the .addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE) usages and added unit tests.

Lastly, I've also added a convenience method for closing the channel after the write.
PTAL again

lhotari

Good work @nicoloboschi

lhotari · 2023-01-10T12:09:23Z

/pulsarbot rerun-failure-checks

nodece · 2023-01-10T13:39:58Z

    }

+    private void writeAndFlush(ByteBuf cmd) {
+        NettyChannelUtil.writeAndFlushWithVoidPromise(outboundChannel, cmd);


Do you miss FIRE_EXCEPTION_ON_FAILURE?

@nodece VoidChannelPromise will propagate exceptions.

codecov-commenter · 2023-01-10T13:47:26Z

Codecov Report

❌ Patch coverage is 45.89041% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.13%. Comparing base (9ef54fd) to head (d0e0aa4).
⚠️ Report is 3019 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/apache/pulsar/broker/service/ServerCnx.java	35.48%	40 Missing ⚠️
...rg/apache/pulsar/proxy/server/ProxyConnection.java	34.78%	14 Missing and 1 partial ⚠️
...apache/pulsar/proxy/server/LookupProxyHandler.java	39.13%	14 Missing ⚠️
...pulsar/broker/service/PulsarCommandSenderImpl.java	74.07%	7 Missing ⚠️
...apache/pulsar/proxy/server/DirectProxyHandler.java	72.72%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19141      +/-   ##
============================================
- Coverage     47.22%   47.13%   -0.10%     
+ Complexity    10713    10672      -41     
============================================
  Files           713      713              
  Lines         69697    69703       +6     
  Branches       7485     7485              
============================================
- Hits          32914    32854      -60     
- Misses        33096    33143      +47     
- Partials       3687     3706      +19

Flag	Coverage Δ
unittests	`47.13% <45.89%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...apache/pulsar/proxy/server/DirectProxyHandler.java	`63.79% <72.72%> (+0.15%)`	⬆️
...pulsar/broker/service/PulsarCommandSenderImpl.java	`68.36% <74.07%> (+6.43%)`	⬆️
...apache/pulsar/proxy/server/LookupProxyHandler.java	`57.02% <39.13%> (+0.55%)`	⬆️
...rg/apache/pulsar/proxy/server/ProxyConnection.java	`55.77% <34.78%> (-0.57%)`	⬇️
...va/org/apache/pulsar/broker/service/ServerCnx.java	`47.31% <35.48%> (+2.38%)`	⬆️

... and 41 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

michaeljmarshall

LTMG. Great catch!

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 5, 2023

nicoloboschi mentioned this pull request Jan 5, 2023

[run-tests] [improve][broker,proxy] Use ChannelVoidPromise to avoid useless promise objects creation nicoloboschi/pulsar#48

Closed

nodece reviewed Jan 6, 2023

View reviewed changes

Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/LookupProxyHandler.java Outdated

nicoloboschi requested review from eolivelli, lhotari and michaeljmarshall January 9, 2023 16:45

nodece approved these changes Jan 10, 2023

View reviewed changes

lhotari approved these changes Jan 10, 2023

View reviewed changes

lhotari requested changes Jan 10, 2023

View reviewed changes

nicoloboschi added 3 commits January 10, 2023 10:58

[improve][broker,proxy] Use ChannelVoidPromise to avoid useless promi…

f1c7444

…se objects creation

[improve][broker,proxy] Use ChannelVoidPromise to avoid useless promi…

f239270

…se objects creation

style

88f3dc4

nicoloboschi force-pushed the netty-void-promise branch from 703e38c to 88f3dc4 Compare January 10, 2023 10:21

javadoc

d0e0aa4

lhotari approved these changes Jan 10, 2023

View reviewed changes

nodece reviewed Jan 10, 2023

View reviewed changes

nicoloboschi merged commit d6fcdb8 into apache:master Jan 10, 2023

nicoloboschi self-assigned this Jan 10, 2023

nicoloboschi added this to the 2.12.0 milestone Jan 10, 2023

nicoloboschi added area/broker area/proxy labels Jan 10, 2023

michaeljmarshall approved these changes Jan 10, 2023

View reviewed changes

Uh oh!

Conversation

nicoloboschi commented Jan 5, 2023

Motivation

Modifications

Verifying this change

Documentation

Uh oh!

Uh oh!

nodece commented Jan 6, 2023

Uh oh!

nicoloboschi commented Jan 9, 2023

Uh oh!

lhotari commented Jan 10, 2023

Uh oh!

lhotari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicoloboschi commented Jan 10, 2023

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

lhotari commented Jan 10, 2023

Uh oh!

nodece Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

lhotari Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

nodece Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lhotari left a comment •

edited

Loading

codecov-commenter commented Jan 10, 2023 •

edited

Loading