RobertIndie commented on PR #1237: URL: https://github.com/apache/pulsar-client-go/pull/1237#issuecomment-2221807210
> The fix above will bring much less code changes and the code will look more simple. This implementation misses some key details. For example, it lacks a backoff mechanism and connection timeout logic as you said. Also, what happens if many goroutines call `grabConn` at the same time? We might end up with multiple goroutines trying to handle reconnections simultaneously. This doesn't seem like a good approach. Although we could use locks or other methods to manage concurrent executions, there are still issues. What if there's a connection timeout and we need to notify the user? Plus, how do we inform other goroutines to stop attempting reconnections in that case? It seems we need complex and less intuitive logic to address these issues. IIUC, the main simplification in this implementation involves transfering the XXOp and the event loop logic, right? The key difference appears to be the use of the event loop model. However, reducing this complexity doesn't seem very significant to me. Both producers and consumers already use the event loop for handling requests. https://github.com/apache/pulsar-client-go/blob/b0111a2dc473317e4b41ab4af09e42b09a72384c/pulsar/consumer_partition.go#L1578-L1582 The Java implementation of the TransactionMetaStoreHandler also uses something like event loop, similar to Go: [TransactionMetaStoreHandler Java code](https://github.com/apache/pulsar/blob/83b86abcb74595d7e8aa31b238a7dbb19a04dde2/pulsar-client/src/main/java/org/apache/pulsar/client/impl/TransactionMetaStoreHandler.java#L220-L226) Moreover, without the event loop, it seems challenging to implement asynchronous methods like `XXXAsync`. For example, [Producer.SendAsync](https://github.com/apache/pulsar-client-go/blob/2a28e21c59d005515e118fed5bf8f333d6699e39/pulsar/producer.go#L230). Although we don't have this feature yet, it would be beneficial to make these operations truly asynchronous in the future. The Java client already supports this feature: [Async support in Java client](https://github.com/apache/pulsar/blob/4e5c0bcc2b44c33a966287b86c2c235be249dc51/pulsar-client/src/main/java/org/apache/pulsar/client/impl/transaction/TransactionCoordinatorClientImpl.java#L176) This PR mainly adopts the eventloop patterns of the the existing producer and consumer. Maybe it's better to start a separate discussion for this. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
