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 2021/03/24 02:32:21 UTC

[GitHub] [pulsar-client-go] jonyhy96 opened a new pull request #492: [Issue 491][rpc_client]fix: timeout guarantee for RequestOnCnx

jonyhy96 opened a new pull request #492:
URL: https://github.com/apache/pulsar-client-go/pull/492


   Fixes #491 
   
   ### Motivation
   
   see #491
   
   ### Modifications
   
   add timeout guarantee to RequestOnCnx
   
   ### Verifying this change
   
   - [ ] Make sure that the change passes the CI checks.
   
   ### Does this pull request potentially affect one of the following parts:
   
     - Dependencies (does it add or upgrade a dependency): (no)
     - **The public API: (yes)**
     - The schema: (no)
     - The default values of configurations: (no)
     - The wire protocol: (no)
   
   ### Documentation
   
     - Does this pull request introduce a new feature? (no)
   


-- 
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.

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

[GitHub] [pulsar-client-go] wolfstudy merged pull request #492: [Issue 491][rpc_client]fix: timeout guarantee for RequestOnCnx

Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #492:
URL: https://github.com/apache/pulsar-client-go/pull/492


   


-- 
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.

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

[GitHub] [pulsar-client-go] omnilight commented on pull request #492: [Issue 491][rpc_client]fix: timeout guarantee for RequestOnCnx

Posted by GitBox <gi...@apache.org>.
omnilight commented on pull request #492:
URL: https://github.com/apache/pulsar-client-go/pull/492#issuecomment-810860528


   @wolfstudy @jonyhy96 Hi. Could we proceed with this PR? Lack of the timeout on connection is a problem causing locking producer, would be great if we can release this fix


-- 
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.

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

[GitHub] [pulsar-client-go] jonyhy96 commented on a change in pull request #492: [Issue 491][rpc_client]fix: timeout guarantee for RequestOnCnx

Posted by GitBox <gi...@apache.org>.
jonyhy96 commented on a change in pull request #492:
URL: https://github.com/apache/pulsar-client-go/pull/492#discussion_r602090789



##########
File path: pulsar/internal/rpc_client.go
##########
@@ -139,29 +144,30 @@ func (c *rpcClient) Request(logicalAddr *url.URL, physicalAddr *url.URL, request
 	case res := <-ch:
 		return res.RPCResult, res.error
 	case <-time.After(c.requestTimeout):
-		return nil, errors.New("request timed out")
+		return nil, ErrRequestTimeOut
 	}
 }
 
 func (c *rpcClient) RequestOnCnx(cnx Connection, requestID uint64, cmdType pb.BaseCommand_Type,
 	message proto.Message) (*RPCResult, error) {
 	c.metrics.RPCRequestCount.Inc()
-	wg := sync.WaitGroup{}
-	wg.Add(1)
 
-	rpcResult := &RPCResult{
-		Cnx: cnx,
-	}
+	ch := make(chan result, 1)

Review comment:
       0 maybe enough for this because there will be only one result sends to `ch` . Set it to 1  to prevent block happens on the sender side.




-- 
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.

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

[GitHub] [pulsar-client-go] wolfstudy commented on a change in pull request #492: [Issue 491][rpc_client]fix: timeout guarantee for RequestOnCnx

Posted by GitBox <gi...@apache.org>.
wolfstudy commented on a change in pull request #492:
URL: https://github.com/apache/pulsar-client-go/pull/492#discussion_r602066698



##########
File path: pulsar/internal/rpc_client.go
##########
@@ -139,29 +144,30 @@ func (c *rpcClient) Request(logicalAddr *url.URL, physicalAddr *url.URL, request
 	case res := <-ch:
 		return res.RPCResult, res.error
 	case <-time.After(c.requestTimeout):
-		return nil, errors.New("request timed out")
+		return nil, ErrRequestTimeOut
 	}
 }
 
 func (c *rpcClient) RequestOnCnx(cnx Connection, requestID uint64, cmdType pb.BaseCommand_Type,
 	message proto.Message) (*RPCResult, error) {
 	c.metrics.RPCRequestCount.Inc()
-	wg := sync.WaitGroup{}
-	wg.Add(1)
 
-	rpcResult := &RPCResult{
-		Cnx: cnx,
-	}
+	ch := make(chan result, 1)

Review comment:
       Can we consider adjusting the size of the channel buffer appropriately? It looks like 10 is a good choice?




-- 
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.

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

[GitHub] [pulsar-client-go] jonyhy96 commented on pull request #492: [Issue 491][rpc_client]fix: timeout guarantee for RequestOnCnx

Posted by GitBox <gi...@apache.org>.
jonyhy96 commented on pull request #492:
URL: https://github.com/apache/pulsar-client-go/pull/492#issuecomment-845034430


   ping @wolfstudy 


-- 
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.

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