[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16396423#comment-16396423
]
ASF GitHub Bot commented on THRIFT-4448:
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1459
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Assignee: James E. King, III
>Priority: Major
> Fix For: 0.12.0
>
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16363189#comment-16363189
]
ASF GitHub Bot commented on THRIFT-4448:
Github user johnboiles commented on the issue:
https://github.com/apache/thrift/pull/1459
@jeking3 done
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16350897#comment-16350897
]
ASF GitHub Bot commented on THRIFT-4448:
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
Looks *much* cleaner!
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16350881#comment-16350881
]
ASF GitHub Bot commented on THRIFT-4448:
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 :)
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16350815#comment-16350815
]
ASF GitHub Bot commented on THRIFT-4448:
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 very hit and miss :/
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16350795#comment-16350795
]
ASF GitHub Bot commented on THRIFT-4448:
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 commit?
```
[default] synchronized from https://opam.ocaml.org
Stack overflow
[ERROR] Initialisation failed
Fatal error:
Stack overflow
The command '/bin/sh -c apt-get install -y --no-install-recommends `# OCaml
dependencies` ocaml opam && opam init --yes && opam install
--yes oasis' returned a non-zero code: 1
```
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16350542#comment-16350542
]
ASF GitHub Bot commented on THRIFT-4448:
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.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16350515#comment-16350515
]
ASF GitHub Bot commented on THRIFT-4448:
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 goroutine code begins to execute. Or I could
have some retry logic that tries to contact the server for a second before
failing
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16350182#comment-16350182
]
ASF GitHub Bot commented on THRIFT-4448:
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
```
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16349928#comment-16349928
]
ASF GitHub Bot commented on THRIFT-4448:
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1459
That’s what I get for reviewing code before fully waking up :)
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16349864#comment-16349864
]
ASF GitHub Bot commented on THRIFT-4448:
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 1.7. You can just
remove that line since it's not a long running process, or you can make it
conditional with [`runtime.Version`](https://golang.org/pkg/runtime/#Version).
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16349230#comment-16349230
]
ASF GitHub Bot commented on THRIFT-4448:
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 with longer timers, say 2s sleep
and 1s timeout. The test suite already takes quite a bit of time to run, an
additional second should be fine.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16349043#comment-16349043
]
ASF GitHub Bot commented on THRIFT-4448:
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 risk of introducing a
flakey test, I'm happy to write it.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16348236#comment-16348236
]
ASF GitHub Bot commented on THRIFT-4448:
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
`context.WithTimeout(context.Background(), 50*time.Millisecond)`
- Check if `ctx.Err() == context.Canceled`
Otherwise LGTM.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16347291#comment-16347291
]
ASF GitHub Bot commented on THRIFT-4448:
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?
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>Priority: Major
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16323163#comment-16323163
]
ASF GitHub Bot commented on THRIFT-4448:
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 take a stab at updating this hopefully tomorrow or early next week.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16322347#comment-16322347
]
ASF GitHub Bot commented on THRIFT-4448:
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 something newer from either an Ubuntu PPA
or directly form golang sources.
I also found evidence in a number of other projects that support for golang
1.6 was dropped. So if you want to fix this in the way suggested, you will
need to update the ubuntu-xenial Dockerfile to use golang 1.7 instead, and you
should update the top level LANGUAGES.md file, add a breaking change note into
the lib/go README file, and update the build/docker/README.md file. Thanks.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16321581#comment-16321581
]
ASF GitHub Bot commented on THRIFT-4448:
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:
https://github.com/golang/mock/pull/118
Now I have to think some more about it. :|
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16321531#comment-16321531
]
ASF GitHub Bot commented on THRIFT-4448:
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?
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16321447#comment-16321447
]
ASF GitHub Bot commented on THRIFT-4448:
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 they said back to 1.7 makes
sense, but 1.6 to 1.7 was a breaking change. Separately, we might dump the
trusty CI build environment. Not sure about that yet.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16319058#comment-16319058
]
ASF GitHub Bot commented on THRIFT-4448:
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 thrift:master.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318965#comment-16318965
]
ASF GitHub Bot commented on THRIFT-4448:
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 commit bits. cc
@Jens-G @jeking3
> Related question: why was 'context support' added when it doesn't seem to
be hooked up to anything in the library? Seems like the method signatures were
the only thing that changed.
It was added to support passing arbitrary values into RPC handlers. One of
the big use cases was accessing client IP from an RPC handler. Before
`context`, this was impossible since the underlying transport was not visible
to the handler. Now, you can implement a custom `TProcessor` that embeds the
generated processor, and pass the IP manually, something like:
```go
type MyProcessor struct {
*GeneratedProcessor
}
func (p *MyProcessor) Process(ctx context.Context, in, out TProtocol)
(bool, TException) {
name, _, seqId, err := iprot.ReadMessageBegin()
if err != nil {
return false, err
}
if processor, ok := p.GetProcessorFunction(name); ok {
//TODO: get IP from transport and add to ctx
return processor.Process(ctx, seqId, iprot, oprot)
}
// rest of it...
}
}
```
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318923#comment-16318923
]
ASF GitHub Bot commented on THRIFT-4448:
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 there others we should check with
before I put in the work.
Related question: why _was_ 'context support' added when it doesn't seem to
be hooked up to anything in the library? Seems like the method signatures were
the only thing that changed.
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (THRIFT-4448) Golang: do something with context.Context
[
https://issues.apache.org/jira/browse/THRIFT-4448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16318849#comment-16318849
]
ASF GitHub Bot commented on THRIFT-4448:
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 into 0.12 at the earliest and by that
time 1.7 will be ancient. I haven't reviewed the patch yet, but if you'd like
to drop compatibility, please do so across the codebase (so no more
`x/net/context` anywhere).
> Golang: do something with context.Context
> -
>
> Key: THRIFT-4448
> URL: https://issues.apache.org/jira/browse/THRIFT-4448
> Project: Thrift
> Issue Type: Task
> Components: Go - Library
>Affects Versions: 0.11.0
>Reporter: John Boiles
>
> PR Here: https://github.com/apache/thrift/pull/1459
> This patch wires through {{context.Context}} such that it can be used in in
> {{http.Request}}'s {{WithContext}} method. This allows Thrift HTTP requests
> to canceled or timed out via the context.
> This patch breaks support for go<1.7 so it's not ready to ship, but I'm
> hoping to get some direction on this. When does Thrift expect to drop support
> of go1.7? It looks like the current solution is to duplicate files that need
> to use {{golang.org/x/net/context}} and add a {{// +build !go1.7}} but
> duplication seems unsustainable as the {{context}} package is imported more
> places.
> Go 1.7 was released 15 August 2016. Given Golang has had significant
> performance improvements in most dot releases, I suspect most production
> services stay reasonably up to date. Here at Periscope/Twitter we're on
> go1.9.1, and we're a fairly large organization.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
