Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
@jeking3 done
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1459
Would you mind squashing this into a single commit?
---
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
Looks *much* cleaner!
---
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
It bugged me to have a time.Sleep in the test so I wrote the retry logic :)
---
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
The only other way is for someone with Travis crendentials to manually
trigger it. I think it's faster if you push something trivial (whitespace etc.)
Thrift's CI builds are unfortunately
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
It's not clear to me what went wrong in the Travis build. Trusty, for
example, seems to have stack overflow'd while installing ocaml. Is there a way
to retrigger a build without pushing another
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
I think the only one-liner fix is a 500ms sleep before `client.TestVoid()`.
Retry logic would work as well.
---
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
Hmm yeah must be a race condition with the test server starting in the go
routine. Any thoughts on how to wait for it to start up? I guess I could use a
wait group to wait at least until
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
A different failure this time:
```
--- FAIL: TestHttpContextTimeout (0.00s)
context_test.go:74: Unexpected error: dial tcp 127.0.0.1:9096:
getsockopt: connection refused
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
@dcelasun I don't think I can do a `runtime.Version` check since the code
is failing at compilation time. I'll just remove that line.
---
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
The test fails with:
> src/common/context_test.go:45: server.Close undefined (type *http.Server
has no field or method Close)
`Server.Close()` was added in 1.8 but the test uses
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
@dcelasun: done, could you take another look?
---
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
I think it's worth it. The context passes through most parts of the stack
and having a test to ensure it's propagated properly feels imporant.
Also, we can at least reduce the flakiness
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
@dcelasun I'm happy to do that. Though in my experience, tests that depend
on timing will find a way to be flakey, especially in CI.
If you think the value of testing this is worth the
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
It would be nice to add a client test using e.g `context.WithTimeout()`.
Something like:
- In the handler function, sleep for 100ms before returning
- In the client, call the RPC with
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
@jeking3 @dcelasun Travis is green so I think we're almost good to go here.
Mind taking another look and merging if it looks good?
---
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
I think that's the right approach. Itâs pretty easy/fast to install
golang from a tar.gz distribution. I do it pretty frequently to pin a specific
version of go in docker images.
I'll
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1459
My thoughts after sleeping on it are that we should support the current
Ubuntu LTS. That said, I have no problem telling folks not to use the ancient
golang included in the LTS and to use
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1459
Ugh, so it turns out Ubuntu Xenial comes with go 1.6 which is pretty much
the version we use on all of our CI builds for testing (like cross test).
For further details on the issue see:
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
Sounds good want me to take a stab at dropping 1.6 support?
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1459
At this point we should consider dropping go 1.6. Other go projects have
moved on and we can too. I had a discussion with someone about whether it
makes sense to support older go versions, and
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
Thank you for the explainer @dcelasun! Makes sense to me.
@Jens-G, @jeking3: if one of you sign off on this, I'll make the changes to
drop go1.6 and `x/net/context` support from
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
> I personally think dropping go1.6/x/net/context support is best for the
project. Are you the right person to make that call?
Sounds good to me, but we need to run it by someone with
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
I really appreciate your feedback & involvement @dcelasun! I personally
think dropping `go1.6`/`x/net/context` support is best for the project. Are you
the right person to make that call? Or are
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
When context support was added, significant effort went into keeping 1.7
compatibility so I think it would be great if we could maintain that support.
On the other hand, this patch can go
25 matches
Mail list logo
