fishy commented on pull request #2181: URL: https://github.com/apache/thrift/pull/2181#issuecomment-723284850
@zerosnake0 so my guess is by "net/http" you mean [client.go](https://github.com/golang/go/blob/go1.15.4/src/net/http/client.go) and [transport.go](https://github.com/golang/go/blob/go1.15.4/src/net/http/transport.go). Yes there are things we can learn from there, but there are also a lot of key differences between thrift's transport and http transport: 1. In http a transport is a connection pool. It's not only a connection pool to a single server, it's a connection pool to all servers. It's safe for concurrent uses, and is multiplexed so that they can use a lot of channel selections to handle timeout. In thrift a TSocket is a single connection. It's not safe for concurrent use, and every I/O operation is blocking. You can't just create a new goroutine for every blocking I/O operation so that you can throw it away when timeout happens, without a centralized pool management (like in http) that will cause a lot of goroutine leaks if the socket timeout is configured too high (or even not configured at all). 2. In thrift a TTransport is an abstract layer. It could be a remote I/O (TSocket), It could be some additional feature wrapping a remote I/O (TFramedTransport + TSocket). It could also be something completely in memory (TMemoryBuffer). TProtocol implementations, in general (with certain exceptions), don't make any assumption on the underlying TTransport and works with all situations. The actual client/server code also almost never interact with TTransport directly, and they only call the TProtocol code. To do something similar to http library does, TProtocol needs to pass an additional deadline/timeout into every call to the TTransport. This can be done by either: a. Add ctx to every TTransport read/write functions b. Have an additional function to set the deadline/timeout before every TTransport read/write function call. Both options are breaking changes to TTransport, both options were already discussed in the JIRA ticket, and the reason we picked the current implemented option over them can be found there as well. If we completely rewrite the whole thrift go library now, we probably will choose an implementation similar to how http library does over something we have right now. But we are not doing that complete rewrite and the current implementation is the least breaking one, as far as we could tell. If you have a better approach, I would love to see something more concrete from it (e.g. code/PR, or some more detailed design on how to do it). The connectivity check change happened before this change, and is not depending on this change, btw. ---------------------------------------------------------------- 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: [email protected]
