[improve] [proxy] Not close the socket if lookup failed caused by too many requests #21216

poorbarcode · 2023-09-21T05:11:18Z

Motivation

Background: The Pulsar client will close the socket if it receives a ServiceNotReady error when doing a lookup.
see

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java

Lines 1193 to 1200 in af20a8a

    
           private void checkServerError(ServerError error, String errMsg) { 
        
               if (ServerError.ServiceNotReady.equals(error)) { 
        
                   log.error("{} Close connection because received internal-server error {}", ctx.channel(), errMsg); 
        
                   ctx.close(); 
        
               } else if (ServerError.TooManyRequests.equals(error)) { 
        
                   incrementRejectsAndMaybeClose(); 
        
               } 
        
           }

The Broker will respond to the client with a TooManyRequests error if there are too many lookup requests in progress, but the Pulsar Proxy responds to the client with a ServiceNotReady error in the same scenario.

Modifications

Make Pulsar Proxy respond to the client with a TooManyRequests error if there are too many lookup requests in progress.

Documentation

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

Matching PR in forked repository

PR in forked repository: x

… many requests

Technoboy- · 2023-09-21T05:37:11Z

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyLookupThrottlingTest.java

+
+    @Test
+    public void testLookupThrottling() throws Exception {
+        PulsarClientImpl client = (PulsarClientImpl) PulsarClient.builder()


close the client

codecov-commenter · 2023-09-21T10:40:58Z

Codecov Report

Merging #21216 (addd907) into master (eefc517) will increase coverage by 36.09%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master   #21216       +/-   ##
=============================================
+ Coverage     36.75%   72.85%   +36.09%     
+ Complexity    12193     3684     -8509     
=============================================
  Files          1698     1887      +189     
  Lines        130430   140118     +9688     
  Branches      14250    15426     +1176     
=============================================
+ Hits          47940   102081    +54141     
+ Misses        76167    29931    -46236     
- Partials       6323     8106     +1783

Flag	Coverage Δ
inttests	`24.21% <0.00%> (+0.07%)`	⬆️
systests	`24.79% <0.00%> (+0.02%)`	⬆️
unittests	`72.11% <100.00%> (+40.22%)`	⬆️

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

Files Changed	Coverage Δ
...apache/pulsar/proxy/server/LookupProxyHandler.java	`60.16% <100.00%> (+17.37%)`	⬆️

... and 1456 files with indirect coverage changes

heesung-sn · 2023-09-21T16:01:02Z

LGTM

… many requests (#21216) Motivation: The Pulsar client will close the socket if it receives a `ServiceNotReady` error when doing a lookup. The Broker will respond to the client with a `TooManyRequests` error if there are too many lookup requests in progress, but the Pulsar Proxy responds to the client with a `ServiceNotReady` error in the same scenario. Modifications: Make Pulsar Proxy respond to the client with a `TooManyRequests` error if there are too many lookup requests in progress. (cherry picked from commit d6c3fa4)

… many requests (apache#21216) Motivation: The Pulsar client will close the socket if it receives a `ServiceNotReady` error when doing a lookup. The Broker will respond to the client with a `TooManyRequests` error if there are too many lookup requests in progress, but the Pulsar Proxy responds to the client with a `ServiceNotReady` error in the same scenario. Modifications: Make Pulsar Proxy respond to the client with a `TooManyRequests` error if there are too many lookup requests in progress. (cherry picked from commit d6c3fa4) (cherry picked from commit 9a7c4bb)

… too many requests (apache#21216)

… many requests (apache#21216) Motivation: The Pulsar client will close the socket if it receives a `ServiceNotReady` error when doing a lookup. The Broker will respond to the client with a `TooManyRequests` error if there are too many lookup requests in progress, but the Pulsar Proxy responds to the client with a `ServiceNotReady` error in the same scenario. Modifications: Make Pulsar Proxy respond to the client with a `TooManyRequests` error if there are too many lookup requests in progress. (cherry picked from commit d6c3fa4) (cherry picked from commit 9a7c4bb)

… too many requests (apache#21216)

[improve] [proxy] Not close the socket if lookup failed caused by too…

f9b49a5

… many requests

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 21, 2023

poorbarcode requested review from hangc0276, 315157973, codelipenghui and heesung-sn September 21, 2023 05:11

poorbarcode self-assigned this Sep 21, 2023

poorbarcode added release/3.0.2 release/2.11.3 release/2.10.6 category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost labels Sep 21, 2023

poorbarcode added this to the 3.2.0 milestone Sep 21, 2023

Technoboy- reviewed Sep 21, 2023

View reviewed changes

Technoboy- approved these changes Sep 21, 2023

View reviewed changes

Technoboy- added the ready-to-test label Sep 21, 2023

release pulsar client after test done

addd907

coderzc approved these changes Sep 21, 2023

View reviewed changes

poorbarcode merged commit d6c3fa4 into apache:master Sep 21, 2023

poorbarcode added the cherry-picked/branch-3.0 label Sep 24, 2023

poorbarcode added the cherry-picked/branch-2.11 label Sep 24, 2023

poorbarcode added the cherry-picked/branch-2.10 label Sep 24, 2023

mukesh-ctds added a commit to datastax/pulsar that referenced this pull request Apr 22, 2024

Fix [improve] [proxy] Not close the socket if lookup failed caused by…

776bcb3

… too many requests (apache#21216)

srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024

Fix [improve] [proxy] Not close the socket if lookup failed caused by…

b336996

… too many requests (apache#21216)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve] [proxy] Not close the socket if lookup failed caused by too many requests #21216

[improve] [proxy] Not close the socket if lookup failed caused by too many requests #21216

poorbarcode commented Sep 21, 2023

Technoboy- Sep 21, 2023

poorbarcode Sep 21, 2023

codecov-commenter commented Sep 21, 2023

heesung-sn commented Sep 21, 2023

	private void checkServerError(ServerError error, String errMsg) {
	if (ServerError.ServiceNotReady.equals(error)) {
	log.error("{} Close connection because received internal-server error {}", ctx.channel(), errMsg);
	ctx.close();
	} else if (ServerError.TooManyRequests.equals(error)) {
	incrementRejectsAndMaybeClose();
	}
	}

[improve] [proxy] Not close the socket if lookup failed caused by too many requests #21216

[improve] [proxy] Not close the socket if lookup failed caused by too many requests #21216

Conversation

poorbarcode commented Sep 21, 2023

Motivation

Modifications

Documentation

Matching PR in forked repository

Technoboy- Sep 21, 2023

Choose a reason for hiding this comment

poorbarcode Sep 21, 2023

Choose a reason for hiding this comment

codecov-commenter commented Sep 21, 2023

Codecov Report

heesung-sn commented Sep 21, 2023