[cleanup] [broker] when serverCnx disabled auto read use same implements

[poorbarcode]

Motivation

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Lines 2645 to 2652 in 732049f

	if (++pendingSendRequest == maxPendingSendRequests || isPublishRateExceeded) {
	// When the quota of pending send requests is reached, stop reading from socket to cause backpressure on
	// client connection, possibly shared between multiple producers
	ctx.channel().config().setAutoRead(false);
	recordRateLimitMetrics(producers);
	autoReadDisabledRateLimiting = isPublishRateExceeded;
	throttledConnections.inc();
	}

L2468-L2469 make serverCnx disabled auto read, but there has a beter implements at L2723-L2728:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Lines 2723 to 2728 in 732049f

	public void disableCnxAutoRead() {
	if (ctx != null && ctx.channel().config().isAutoRead()) {
	ctx.channel().config().setAutoRead(false);
	recordRateLimitMetrics(producers);
	}
	}

At the better implements, can reduce false statistics( when a serverCnx already disabled auto read, will not increment another once).

Documentation

• doc-required
• no-need-doc
• doc
• doc-added
• doc-not-needed

[gaozhangmin]

LGTM

[poorbarcode]

@michaeljmarshall could you take a look

[michaeljmarshall]

Sorry @315157973, accidentally re-requested your review.

[michaeljmarshall]

@poorbarcode thanks for your contribution. I had a couple questions about your findings, but overall, the PR looks good to me.

Note that this might be a minor performance optimization for connections with many producers. However, it's probably minimal because once a connection has auto-read disabled, this code would only ever complete the current execution of this method and wouldn't call it again.

[michaeljmarshall]

At the better implements, can reduce false statistics( when a serverCnx already disabled auto read, will not increment another once).

Is there evidence that this metric was getting incremented unnecessarily or is this just an inferred conclusion? Is the issue you're trying to prevent that the producer could have auto-read set to false concurrently? Calling the isAutoRead method as a guard does not prevent the race condition, though it might decrease its probability, because we read from the volatile variable and then update it. The consequence of the race is pretty low since the rate limit metrics are just prometheus metrics.

[poorbarcode]

is this just an inferred conclusion ?

yes

[poorbarcode]

is this just an inferred conclusion?

yes

[poorbarcode]

@lhotari Could you help me, I do not know what to do.

[poorbarcode]

/pulsarbot run-failure-checks

[poorbarcode]

@gaoran10 Could you take a look

[poorbarcode]

/pulsarbot run-failure-checks

[codelipenghui]

@poorbarcode Please rebase to the master branch.

[poorbarcode]

@poorbarcode Please rebase to the master branch.

ok

[poorbarcode]

@gaoran10 Can this PR merge ^_^?