You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/05/19 07:24:06 UTC

[GitHub] [pulsar] poorbarcode opened a new pull request, #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

poorbarcode opened a new pull request, #15181:
URL: https://github.com/apache/pulsar/pull/15181

   ### Motivation
   
   https://github.com/apache/pulsar/blob/732049fc6ca1beb046deb43057be2b130736fbca/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L2645-L2652
   
   `L2468-L2469` make serverCnx disabled auto read, but **there has a beter implements** at `L2723-L2728`:
   
   
   https://github.com/apache/pulsar/blob/732049fc6ca1beb046deb43057be2b130736fbca/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L2723-L2728
   
   
   At the better implements, can reduce false statistics( when a serverCnx already disabled auto read, will not increment another once). 
   
   ### Documentation
   - [ ] `doc-required` 
   - [x] `no-need-doc` 
   - [ ] `doc` 
   - [ ] `doc-added`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1102150994

   /pulsarbot run-failure-checks
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1128865174

   /pulsarbot run-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codelipenghui commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1153136005

   @poorbarcode Please rebase to the master branch.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1100693430

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1100038867

   @michaeljmarshall could you take a look 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#discussion_r853349053


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2645,8 +2645,7 @@ public void startSendOperation(Producer producer, int msgSize, int numMessages)
         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);

Review Comment:
   > 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on a diff in pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#discussion_r853411748


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2645,8 +2645,7 @@ public void startSendOperation(Producer producer, int msgSize, int numMessages)
         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);

Review Comment:
   > is this just an inferred conclusion ?
   
   yes



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1103013521

   @lhotari Could you help me, I do not know what to do.
   
   ![image](https://user-images.githubusercontent.com/25195800/164080815-5d5fa80b-1aab-4488-b8d5-ef8900232cfa.png)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1128895464

   @gaoran10 Could you take a look


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1112864699

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Jason918 merged pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
Jason918 merged PR #15181:
URL: https://github.com/apache/pulsar/pull/15181


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1153136755

   > @poorbarcode Please rebase to the master branch.
   
   ok


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1101031553

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on a diff in pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#discussion_r853419630


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2645,8 +2645,7 @@ public void startSendOperation(Producer producer, int msgSize, int numMessages)
         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);

Review Comment:
   > is this just an inferred conclusion?
   
   yes



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1118725376

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1102932767

   Sorry @315157973, accidentally re-requested your review.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1147093199

   /pulsarbot run-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1160395190

   @gaoran10 Can this PR merge ^_^?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode closed pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode closed pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements
URL: https://github.com/apache/pulsar/pull/15181


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on pull request #15181: [cleanup] [broker] when serverCnx disabled auto read use same implements

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15181:
URL: https://github.com/apache/pulsar/pull/15181#issuecomment-1103021098

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org