You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/06/13 17:26:23 UTC
[GitHub] [thrift] fishy opened a new pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
fishy opened a new pull request #2181:
URL: https://github.com/apache/thrift/pull/2181
Client: go
As discussed in the JIRA ticket, this commit changes how we handle I/O
timeouts in the go library.
This is a breaking change that adds context to all Read*, Write*, and
Skip functions to TProtocol, along with the compiler change to support
that, and also adds context to TStandardClient.Recv, TDeserializer,
TStruct, and a few others.
Along with the function signature changes, this commit also implements
context cancellation check in the following TProtocol's ReadMessageBegin
implementations:
- TBinaryProtocol
- TCompactProtocol
- THeaderProtocol
In those ReadMessageBegin implementations, if the passed in context
object has a deadline attached, it will keep retrying the I/O timeout
errors, until the deadline on the context object passed. They won't
retry I/O timeout errors if the passed in context does not have a
deadline attached (still return on the first error).
<!-- Explain the changes in the pull request below: -->
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] Did you squash your changes to a single commit? (not required, but preferred)
- [ ] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-643705119
@dcelasun OK the CI is "passing" now so it's ready for review. I marked the actual logical changes above, everything else are basically just for propagating context.
----------------------------------------------------------------
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] [thrift] zerosnake0 commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722928987
> > I suggest you take a look at the golang official net/http implementation before any furthur discussion
>
> If I may, "go read net/http" is not a productive attitude. You are arguing against a change that was already discussed, agreed to and merged. That's fine, it's even welcome. But the onus is on _you_ to convince us there is a better way and that we should revert this. You didn't even address @fishy's first and second point.
I surely read the first point and I agree with the rarer implementation so I said nothing. Same for the 3rd point.
And why i mentioned net/http because there is something similar for this kind of issue and I'm not sure if you know it. You quote my phrase in just three word "go read net/http" when I typed such a long phrase. I apologize if there is any word misusage.
----------------------------------------------------------------
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] [thrift] Trane9991 commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
Trane9991 commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-790599343
@fishy I have a couple of clarification questions about the expected behavior of the timeouts. Sorry if I misunderstood something.
1. The logic handles only the retries? What about canceling the reads that are taking longer than the deadline?
For example:
- If I set a context deadline of 1 second and `i/o timeout` appears, before that, a read will be retried. Correct?
- If the context deadline set to 1 second, but read takes 5 seconds, it will not be canceled after 1 second and will not be retried later. Correct?
2. Is the logic applied only to the `ReadMessageBegin`? What about `Read` and `ReadMessageEnd`?
For example:
- Context deadline is set to 5 seconds and `i/o timeout` happens after 1 second of `ReadMessageBegin`, so it will be retried. Right?
- Context deadline is set to 5 seconds, and `ReadMessageBegin` succeeds in 1 second, but the further `Read` fails with the `i/o timeout` in 1 more second (so 3 seconds left until the deadline). Will it be retried?
3. This logic was added to the `TBinaryProtocol`, `TCompactProtocol`, `THeaderProtocol` but not to the `TJSONProtocol`, right?
Thanks!
----------------------------------------------------------------
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] [thrift] dcelasun commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
dcelasun commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722927178
> I suggest you take a look at the golang official net/http implementation before any furthur discussion
If I may, "go read net/http" is not a productive attitude. You are arguing against a change that was already discussed, agreed to and merged. That's fine, it's even welcome. But the onus is on *you* to convince us there is a better way and that we should revert this. You didn't even address @fishy's first and second point.
----------------------------------------------------------------
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] [thrift] zerosnake0 edited a comment on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 edited a comment on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722898457
> @zerosnake0 I replied some of your points below. I would also suggest you to read the associated [JIRA ticket](https://issues.apache.org/jira/browse/THRIFT-5233), which provides slightly more background about what problem this change solves.
>
> > 1. It's a breaking change
>
> Unlike Go1, there's no backward compatibility guarantee in thrift library. If you take a look at https://github.com/apache/thrift/blob/master/CHANGES.md, breaking changes are added in every release.
>
> > 1. the context argument is not really being used in the current implementation, the golang io interface does not use context neither ([golang/go#20280](https://github.com/golang/go/issues/20280))
>
> It's used [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/header_transport.go#L349-L361), [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/compact_protocol.go#L338-L348). and [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/binary_protocol.go#L468-L478).
1. do breaking change only when necessary
2. I've already checked all usage of the context before I posted this, that's why i said "not really". Furthermore, for the later two usage you wrote, i do not think that this kind of loop handling should be processed inside the protocol implementation
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722879339
@zerosnake0 I replied some of your points below. I would also suggest you to read the associated [JIRA ticket](https://issues.apache.org/jira/browse/THRIFT-5233), which provides slightly more background about what problem this change solves.
> 1. It's a breaking change
Unlike Go1, there's no backward compatibility guarantee in thrift library. If you take a look at https://github.com/apache/thrift/blob/master/CHANGES.md, breaking changes are added in every release.
> 2. the context argument is not really being used in the current implementation, the golang io interface does not use context neither ([golang/go#20280](https://github.com/golang/go/issues/20280))
It's used [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/header_transport.go#L349-L361), [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/compact_protocol.go#L338-L348). and [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/binary_protocol.go#L468-L478).
----------------------------------------------------------------
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] [thrift] zerosnake0 commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722898457
> @zerosnake0 I replied some of your points below. I would also suggest you to read the associated [JIRA ticket](https://issues.apache.org/jira/browse/THRIFT-5233), which provides slightly more background about what problem this change solves.
>
> > 1. It's a breaking change
>
> Unlike Go1, there's no backward compatibility guarantee in thrift library. If you take a look at https://github.com/apache/thrift/blob/master/CHANGES.md, breaking changes are added in every release.
>
> > 1. the context argument is not really being used in the current implementation, the golang io interface does not use context neither ([golang/go#20280](https://github.com/golang/go/issues/20280))
>
> It's used [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/header_transport.go#L349-L361), [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/compact_protocol.go#L338-L348). and [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/binary_protocol.go#L468-L478).
1. do breaking change is only when necessary
2. I've already checked all usage of the context before I posted this, that's why i said "really". Furthermore, for the later two usage you wrote, i do not think that this kind of loop handling should be processed inside the protocol implementation
----------------------------------------------------------------
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] [thrift] zerosnake0 commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722822196
In my POV, this context argument should not be added, at least not the way it currently is.
1. It's a breaking change
2. the context argument is not really being used in the current implementation, the golang io interface does not use context neither (https://github.com/golang/go/issues/20280)
3. The context here in the Process method represents only the context of current processing which should be canceled right away after processing before continue (which is not)
4. The abortion of the connection should be treated correctly by the transport internally
5. Imagine that your server has a global context (which exists in the current implementation, however not really being used) which represents the lifecycle of the server, and when it's shut down, the IO operations should be finished gracefully instead of being interrupted instantly. The only thing which should affect the IO operations should be the IO timeout which should be configured inside the server or transport
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [thrift] zerosnake0 commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722915472
> Resolving a real issue sounds like a fitting case of "necessary", won't you think?
>
> In the JIRA ticket me and Can discussed how to implement it, including the alternative of doing it on transport level (`TTransport`). Doing it on transport level would also be a breaking change, and we made a choice to do a breaking change on `TProtocol` over `TTransport`.
>
> On hindsight yes the loop retry fits more naturally in TTransport, but some problems of that are:
>
> 1. As Can described in the JIRA ticket discussion, `TProtocol` implementations in the wild are much rarer than `TTranposrt` implementations.
> 2. Adding context into `TTransport.Read` will make it no longer an implementation of `io.Reader`, which breaks a lot of other things.
> 3. In the vast majority of cases this is really only needed for the first read (vs. every read).
2. When i say put timeout in transport, i do not mean to put context to it.
I suggest you take a look at the golang official net/http implementation before any furthur discussion
----------------------------------------------------------------
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] [thrift] zerosnake0 edited a comment on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 edited a comment on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722898457
> @zerosnake0 I replied some of your points below. I would also suggest you to read the associated [JIRA ticket](https://issues.apache.org/jira/browse/THRIFT-5233), which provides slightly more background about what problem this change solves.
>
> > 1. It's a breaking change
>
> Unlike Go1, there's no backward compatibility guarantee in thrift library. If you take a look at https://github.com/apache/thrift/blob/master/CHANGES.md, breaking changes are added in every release.
>
> > 1. the context argument is not really being used in the current implementation, the golang io interface does not use context neither ([golang/go#20280](https://github.com/golang/go/issues/20280))
>
> It's used [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/header_transport.go#L349-L361), [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/compact_protocol.go#L338-L348). and [here](https://github.com/apache/thrift/blob/05bb55148608b4324a8c91c21cf9a6a0dff282fa/lib/go/thrift/binary_protocol.go#L468-L478).
1. do breaking change only when necessary
2. I've already checked all usage of the context before I posted this, that's why i said "really". Furthermore, for the later two usage you wrote, i do not think that this kind of loop handling should be processed inside the protocol implementation
----------------------------------------------------------------
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] [thrift] fishy commented on a change in pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on a change in pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#discussion_r439778871
##########
File path: lib/go/thrift/binary_protocol.go
##########
@@ -455,16 +455,27 @@ func (p *TBinaryProtocol) Flush(ctx context.Context) (err error) {
return NewTProtocolException(p.trans.Flush(ctx))
}
-func (p *TBinaryProtocol) Skip(fieldType TType) (err error) {
- return SkipDefaultDepth(p, fieldType)
+func (p *TBinaryProtocol) Skip(ctx context.Context, fieldType TType) (err error) {
+ return SkipDefaultDepth(ctx, p, fieldType)
}
func (p *TBinaryProtocol) Transport() TTransport {
return p.origTransport
}
-func (p *TBinaryProtocol) readAll(buf []byte) error {
- _, err := io.ReadFull(p.trans, buf)
+func (p *TBinaryProtocol) readAll(ctx context.Context, buf []byte) (err error) {
+ var read int
+ _, deadlineSet := ctx.Deadline()
+ for {
+ read, err = io.ReadFull(p.trans, buf)
+ if deadlineSet && read == 0 && isTimeoutError(err) && ctx.Err() == nil {
+ // This is I/O timeout without anything read,
+ // and we still have time left, keep retrying.
+ continue
+ }
+ // For anything else, don't retry
+ break
+ }
Review comment:
This for loop is how we implement context deadline check for TBinaryProtocol, as the first read in `ReadMessageBegin` is `ReadI32` which calls `readAll`.
##########
File path: lib/go/thrift/compact_protocol.go
##########
@@ -329,9 +329,20 @@ func (p *TCompactProtocol) WriteBinary(bin []byte) error {
//
// Read a message header.
-func (p *TCompactProtocol) ReadMessageBegin() (name string, typeId TMessageType, seqId int32, err error) {
+func (p *TCompactProtocol) ReadMessageBegin(ctx context.Context) (name string, typeId TMessageType, seqId int32, err error) {
+ var protocolId byte
- protocolId, err := p.readByteDirect()
+ _, deadlineSet := ctx.Deadline()
+ for {
+ protocolId, err = p.readByteDirect()
+ if deadlineSet && isTimeoutError(err) && ctx.Err() == nil {
+ // keep retrying I/O timeout errors since we still have
+ // time left
+ continue
+ }
+ // For anything else, don't retry
+ break
+ }
Review comment:
This for loop is how we implement context deadline check in TCompactProtocol.
##########
File path: lib/go/thrift/transport_exception.go
##########
@@ -64,6 +64,10 @@ func (p *tTransportException) Unwrap() error {
return p.err
}
+func (p *tTransportException) Timeout() bool {
+ return p.typeId == TIMED_OUT
+}
Review comment:
This is also newly added to make `isTimeoutError` implementation easier (so it does not need to try to check for TTransportException and unwrap it).
##########
File path: lib/go/thrift/header_transport.go
##########
@@ -297,18 +297,34 @@ func (t *THeaderTransport) IsOpen() bool {
// ReadFrame tries to read the frame header, guess the client type, and handle
// unframed clients.
-func (t *THeaderTransport) ReadFrame() error {
+func (t *THeaderTransport) ReadFrame(ctx context.Context) error {
if !t.needReadFrame() {
// No need to read frame, skipping.
return nil
}
+
// Peek and handle the first 32 bits.
// They could either be the length field of a framed message,
// or the first bytes of an unframed message.
- buf, err := t.reader.Peek(size32)
+ var buf []byte
+ var err error
+ // This is also usually the first read from a connection,
+ // so handle retries around socket timeouts.
+ _, deadlineSet := ctx.Deadline()
+ for {
+ buf, err = t.reader.Peek(size32)
+ if deadlineSet && isTimeoutError(err) && ctx.Err() == nil {
+ // This is I/O timeout and we still have time,
+ // continue trying
+ continue
+ }
+ // For anything else, do not retry
+ break
+ }
Review comment:
This for loop is how we implement context deadline check in THeaderProtocol, as this is the first read ReadMessageBegin (if `t.reedReadFrame` returned false above, then the actual `ReadMessageBegin` will call the underlying TBinaryProtocol.ReadMessageBegin or TCompactProtocol.ReadMessageBegin, which already handled it).
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-790775860
@Trane9991
1. Yes for both
2. This is because the way the library/framework works, you would only get io timeouts on the first reads (`ReadMessageBegin`). For client side write (server side read), if you look at `TStandardClient`'s implementation, we always call `WriteMessageBegin`, `Write`, `WriteMessageEnd`, `Flush` without delays between them (https://github.com/apache/thrift/blob/a8c041dd580ff37f3e32b0eaafed542f496d5d58/lib/go/thrift/client.go#L44-L53). For server side write (client side read), it's the same in the compiler generated `Process` functions. The only "delay" that could cause io timeouts are the delaye before calling `WriteMessageBegin`. For server writes those delays are just the actual handler handling the request and generating the response. I described that in the [jira ticket](https://issues.apache.org/jira/browse/THRIFT-5233).
3. Right.
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-644216549
> I think you missed the client side :)
>
> ```
> # servicestest/container_test-remote
> gopath/src/servicestest/container_test-remote/container_test-remote.go:164:42: not enough arguments in call to containerStruct0.ReadField1
> have (thrift.TProtocol)
> want (context.Context, thrift.TProtocol)
> gopath/src/servicestest/container_test-remote/container_test-remote.go:190:42: not enough arguments in call to containerStruct0.ReadField1
> have (thrift.TProtocol)
> want (context.Context, thrift.TProtocol)
> ```
Fixed. Sorry I missed that one. We totally ignored those `-remote` directories in our build system as we don't use them.
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722913849
Resolving a real issue sounds like a fitting case of "necessary", won't you think?
In the JIRA ticket me and Can discussed how to implement it, including the alternative of doing it on transport level (`TTransport`). Doing it on transport level would also be a breaking change, and we made a choice to do a breaking change on `TProtocol` over `TTransport`.
On hindsight yes the loop retry fits more naturally in TTransport, but some problems of that are:
1. As Can described in the JIRA ticket discussion, `TProtocol` implementations in the wild are much rarer than `TTranposrt` implementations.
2. Adding context into `TTransport.Read` will make it no longer an implementation of `io.Reader`, which breaks a lot of other things.
3. In the vast majority of cases this is really only needed for the first read (vs. every read).
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-723397818
>Another reason is that currently the only reason cancels the context is the disconnection
This is not true. The feature this change implemented is mainly for clients to use (servers don't care about read timeouts unless they don't intent to support long connections/client pools). It's very common for clients to set a timeout in the context object.
----------------------------------------------------------------
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] [thrift] zerosnake0 edited a comment on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 edited a comment on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-723393352
> @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.
Sorry I didn't specify which file to look yesterday, I mean more to [server.go](https://github.com/golang/go/blob/go1.15.4/src/net/http/server.go), in the readRequest method the approch is more like the 2.b that you pointed. Personally I think 2.b will be more reasonable because we need to set timeout specifically every time the IO reads/writes the request/response
Another reason is that currently the only reason cancels the context is the disconnection, which will surely affect all your IO operations after.
I will try to work something out and let you know
----------------------------------------------------------------
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] [thrift] zerosnake0 commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-723393352
> @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.
Sorry I didn't specify which file to look yesterday, I mean more to [server.go](https://github.com/golang/go/blob/go1.15.4/src/net/http/server.go), in the readRequest method the approch is more like the 2.b that you pointed. Personally I think 2.b will be more reasonable because we need to push deadline every time the IO reads/writes something.
Another reason is that currently the only reason cancels the context is the disconnection, which will surely affect all your IO operations after.
I will try to work something out and let you know
----------------------------------------------------------------
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] [thrift] zerosnake0 edited a comment on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 edited a comment on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722928987
> > I suggest you take a look at the golang official net/http implementation before any furthur discussion
>
> If I may, "go read net/http" is not a productive attitude. You are arguing against a change that was already discussed, agreed to and merged. That's fine, it's even welcome. But the onus is on _you_ to convince us there is a better way and that we should revert this. You didn't even address @fishy's first and second point.
I surely read the first point and I agree with the "rarer implementation point" so I said nothing. Same for the 3rd point.
And why i mentioned net/http because there is something similar for this kind of issue and I'm not sure if you know it. You quote my phrase in just three word "go read net/http" when I typed such a long phrase. I apologize if there is any word misusage.
----------------------------------------------------------------
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] [thrift] zerosnake0 edited a comment on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
zerosnake0 edited a comment on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-722928987
> > I suggest you take a look at the golang official net/http implementation before any furthur discussion
>
> If I may, "go read net/http" is not a productive attitude. You are arguing against a change that was already discussed, agreed to and merged. That's fine, it's even welcome. But the onus is on _you_ to convince us there is a better way and that we should revert this. You didn't even address @fishy's first and second point.
I surely read the first point and I agree with the "rarer implementation point" so I said nothing. Same for the 3rd point.
And why i mentioned net/http because there is something similar for this kind of issue and I'm not sure if you know it. You quote my phrase in just three word "go read net/http" when I typed such a long phrase. I apologize if there is any word misusage.
What's more, this commit is related with many PR done by @fishy (for example, for the connectivity check), that's why I want to post here and discuss with him before doing anything
----------------------------------------------------------------
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] [thrift] dcelasun commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
dcelasun commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-644148695
I think you missed the client side :)
```
# servicestest/container_test-remote
gopath/src/servicestest/container_test-remote/container_test-remote.go:164:42: not enough arguments in call to containerStruct0.ReadField1
have (thrift.TProtocol)
want (context.Context, thrift.TProtocol)
gopath/src/servicestest/container_test-remote/container_test-remote.go:190:42: not enough arguments in call to containerStruct0.ReadField1
have (thrift.TProtocol)
want (context.Context, thrift.TProtocol)
```
----------------------------------------------------------------
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] [thrift] dcelasun merged pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
dcelasun merged pull request #2181:
URL: https://github.com/apache/thrift/pull/2181
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-644325529
Just realized that I also need to fix `lib/go/test/tests/` 🤦 . On it.
----------------------------------------------------------------
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] [thrift] fishy commented on pull request #2181: THRIFT-5233: Handle I/O timeouts in go library
Posted by GitBox <gi...@apache.org>.
fishy commented on pull request #2181:
URL: https://github.com/apache/thrift/pull/2181#issuecomment-644405728
> Just realized that I also need to fix `lib/go/test/tests/` 🤦 . On it.
Fixed. I think that fixed all the issues, but please do let me know if I missed anything else.
----------------------------------------------------------------
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