[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

2017-02-08 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1156#discussion_r100089847 --- Diff: lib/go/test/tests/thrifttest_driver.go --- @@ -162,7 +162,7 @@ func (p *ThriftTestDriver) Start() { t.Fatal("TestStringMap f

[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets

2017-02-08 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1156 I finally hade some time to take a look at this. Duplicate detection is now implemented with `reflect.DeepEqual`. It isn't really ideal, but Go lacks a way of defining equality for arbitrary types

[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets

2017-01-31 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1156 Hey @Jens-G sorry, I didn't have time to finish this up just yet. The lib part of the PR is still missing, but I'll be doing that in a few days. I'll rebase, push and let you know. --- If your

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

2017-01-25 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1156#discussion_r97843055 --- Diff: lib/go/test/tests/thrifttest_handler.go --- @@ -96,7 +96,7 @@ func (p *ThriftTestHandler) TestStringMap(thing map[string]string) (r map[string

[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets

2017-02-20 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1156 @Jens-G All tests green and rebased from master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets

2017-02-18 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1156 Whoops, I must have missed that one. Should be fixed now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does

[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets

2017-02-13 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1156 @Jens-G Squashed commits and rebased from master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets

2017-01-15 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1156 > Good intentions, absolutely, but the implementation .. FWIW, I agree. However, we still need a solution and as I explained above, we don't have a way of returning an error h

[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets

2017-01-15 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1156 THRIFT-4011 Use slices for Thrift sets As discussed in [THRIFT-4011](https://issues.apache.org/jira/browse/THRIFT-4011), this commit changes the Go generator to use slices, instead of maps

[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets

2017-01-15 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1156 Hey @Jens-G, I'm not sure I follow you, what does this have anything to do with maps? Assuming you meant sets, the docs say: > An unordered set of unique elements. Translates to an STL

[GitHub] thrift pull request #1207: THRIFT-4031: Fix invalid Go code generation for l...

2017-03-03 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1207 THRIFT-4031: Fix invalid Go code generation for list of typedef'ed types This commit reverts 12d430e723b020f7a8ce42a40c19edf88f948367 which caused invalid code to be generated for certain types

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-17 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r127774140 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, &qu

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-17 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r127775849 --- Diff: lib/go/thrift/simple_server2.go --- @@ -1,180 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-17 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r127776518 --- Diff: lib/go/thrift/multiplexed_processor.go --- @@ -1,3 +1,5 @@ +// +build go1.7 --- End diff -- Maybe this file should be named

[GitHub] thrift issue #1287: THRIFT-4219 Refactor Go HTTP Client

2017-07-11 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1287 Not to rush anyone, but will this be merged before 0.11? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-18 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r128154321 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, &qu

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-19 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r128159270 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, &qu

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-19 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r128164607 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, &qu

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-19 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r128159593 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, &qu

[GitHub] thrift issue #1309: Use build tags to support context.

2017-07-21 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1309 Yep, they are unrelated. Thanks for working on this! @Jens-G what do you think? With this PR, we'll have `context` support for pre-1.7 Go versions as well. I'll also be sending

[GitHub] thrift issue #1309: Use build tags to support context.

2017-07-22 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1309 Hmm, turns out @taozle already [updated](https://github.com/apache/thrift/blob/c0d384a38c2b43ee47cef86b1cd054e3f84dc909/tutorial/go/Makefile.am#L37) the Makefile for the tutorial, I just missed

[GitHub] thrift issue #1309: Use build tags to support context.

2017-07-23 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1309 Ah, now I see it. The Makefile for the tutorial has the `legacy_context` flag hardcoded, since it's pinned to the Go version included in the Docker container. If you are running it outside

[GitHub] thrift issue #1312: Add context as first arg for client method.

2017-07-23 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1312 Travis failures seem unrelated. cc @Jens-G --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-18 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r128154458 --- Diff: lib/go/thrift/simple_server2.go --- @@ -1,180 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-18 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r128156875 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, &qu

[GitHub] thrift pull request #1309: Use build tags to support context.

2017-07-19 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1309#discussion_r128301321 --- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc --- @@ -3745,5 +3720,5 @@ THRIFT_REGISTER_GENERATOR(go, &qu

[GitHub] thrift issue #1312: THRIFT-4260: Add context as first arg for client method.

2017-07-24 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1312 LGTM (once again, travis failure is unrelated) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have

[GitHub] thrift issue #1287: THRIFT-4219 Refactor Go HTTP Client

2017-06-28 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1287 @Jens-G Thoughts? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so

[GitHub] thrift pull request #1266: THRIFT-4197 [Go] Transparent gzip support for HTT...

2017-05-12 Thread dcelasun
GitHub user dcelasun reopened a pull request: https://github.com/apache/thrift/pull/1266 THRIFT-4197 [Go] Transparent gzip support for HTTP transport Check if the client supports gzip compressed responses and compress the response if it does. @Jens-G thoughts? You can

[GitHub] thrift pull request #1266: THRIFT-4197 [Go] Transparent gzip support for HTT...

2017-05-12 Thread dcelasun
Github user dcelasun closed the pull request at: https://github.com/apache/thrift/pull/1266 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature

[GitHub] thrift issue #1266: THRIFT-4197 [Go] Transparent gzip support for HTTP trans...

2017-05-11 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1266 Travis failure is unrelated, seems like a Haskell problem: ``` Build log ( /root/.cabal/logs/HUnit-1.6.0.0.log ): cabal: /root/.cabal/logs/HUnit-1.6.0.0.log: does not exist lib

[GitHub] thrift pull request #1266: THRIFT-4197 [Go] Transparent gzip support for HTT...

2017-05-11 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1266 THRIFT-4197 [Go] Transparent gzip support for HTTP transport Check if the client supports gzip compressed responses and compress the response if it does. You can merge this pull request

[GitHub] thrift issue #1266: THRIFT-4197 [Go] Transparent gzip support for HTTP trans...

2017-05-13 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1266 Yes I did, works just fine. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled

[GitHub] thrift issue #1266: THRIFT-4197 [Go] Transparent gzip support for HTTP trans...

2017-05-12 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1266 > I have a life besides Thrift, you know ;-) As do we all :) > Don't panic No panic here, or in the code! :) --- If your project is set up for it, you can

[GitHub] thrift pull request #1281: THRIFT-3703 Unions Field Count Does Not Consider ...

2017-05-30 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1281 THRIFT-3703 Unions Field Count Does Not Consider Map/Set/List Fields Even though maps and slices are not pointer fields in the generated code, they are still nullable. Adding an extra check

[GitHub] thrift issue #1281: THRIFT-3703 Unions Field Count Does Not Consider Map/Set...

2017-05-30 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1281 Travis failures are unrelated to the PR, looks like problems with Java and JS. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] thrift pull request #1285: THRIFT-4215/4216 Make transport factories return ...

2017-06-02 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1285 THRIFT-4215/4216 Make transport factories return errors This commit changes the signature of `TTransportFactory.GetTransport` from ```go GetTransport(trans TTransport) TTransport

[GitHub] thrift issue #1285: THRIFT-4215/4216 Make transport factories return errors

2017-06-02 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1285 Closing PR until all tests are fixed, I don't want to waste Apache's Travis capacity. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] thrift pull request #1285: THRIFT-4215/4216 Make transport factories return ...

2017-06-02 Thread dcelasun
Github user dcelasun closed the pull request at: https://github.com/apache/thrift/pull/1285 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature

[GitHub] thrift pull request #1286: THRIFT-4215 Make transport factories return error...

2017-06-02 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1286 THRIFT-4215 Make transport factories return errors This commit changes the signature of `TTransportFactory.GetTransport` from ```go GetTransport(trans TTransport) TTransport

[GitHub] thrift pull request #1286: THRIFT-4215 Make transport factories return error...

2017-06-05 Thread dcelasun
Github user dcelasun closed the pull request at: https://github.com/apache/thrift/pull/1286 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature

[GitHub] thrift pull request #1287: THRIFT-4219 Refactor Go HTTP Client

2017-06-10 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1287 THRIFT-4219 Refactor Go HTTP Client As discussed in THRIFT-4219, this commit removes support for one-off GET requests, which brings the Go HTTP client inline with implementations in other

[GitHub] thrift issue #1287: THRIFT-4219 Refactor Go HTTP Client

2017-06-10 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1287 Travis failures are unrelated. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled

[GitHub] thrift issue #1285: THRIFT-4215/4216 Make transport factories return errors

2017-06-06 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1285 Yes sorry about that, this was an expected breakage as the interface has changed. I suggest using `0.10` instead of `master`, that way you won't get any surprises like this until `0.11

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-10-10 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 ping @jeking3 ---

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-10-10 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 > Is there a way you can provide a generated NewFooClientFactory as an adapter to run the new code? It would require putting back some of the generated code, but I'll try to find a way

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-10-05 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 @jeking3 All tests green. I've reverted all the Go version changes and made the tests compatible with Go 1.4. I've also tested Thrift from this branch (both compiler and lib) with a large

[GitHub] thrift issue #1375: THRIFT-4346: Allow ZlibTransportFactory to wrap other fa...

2017-09-26 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1375 LGTM, CI failures are unrelated. cc @Jens-G ---

[GitHub] thrift pull request #1381: THRIFT-4285: Move TX/RX methods from gen. code to...

2017-10-02 Thread dcelasun
Github user dcelasun closed the pull request at: https://github.com/apache/thrift/pull/1381 ---

[GitHub] thrift pull request #1381: THRIFT-4285: Move TX/RX methods from gen. code to...

2017-10-02 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1381 THRIFT-4285: Move TX/RX methods from gen. code to library This change removes a lot of duplication from generated code and allows the caller to customize how they can read from / write

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-10-02 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 Travis failure is unrelated, all Go tests are green. cc @jeking3 thoughts? ---

[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...

2017-10-02 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1382 THRIFT-4285 Move TX/RX methods from gen. code to library This change removes a lot of duplication from generated code and allows the caller to customize how they can read from / write

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-10-02 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 > I don't think we can unilaterally require go 1.9 at this point without causing some pain, but I'm not sure. This change doesn't effect the user, it's only needed for the tests. Tho

[GitHub] thrift pull request #1375: THRIFT-4346: Allow ZlibTransportFactory to wrap o...

2017-09-25 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1375#discussion_r140900375 --- Diff: lib/go/thrift/zlib_transport.go --- @@ -39,12 +40,26 @@ type TZlibTransport struct { // GetTransport constructs a new instance

[GitHub] thrift issue #1341: THRIFT-4307: Make ssl-open timeout effective in golang c...

2017-09-04 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1341 LGTM, but Travis build didn't run correctly due to a Docker issue, please close & reopen this to trigger a rebuild. --- If your project is set up for it, you can reply to this email and have

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-10-18 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 @jeking3 Is there a deadline for 0.11? I really want to get this into the next release, but likely won't have enough time to finish it up in the next week or so. ---

[GitHub] thrift issue #1411: Fix remote client for HTTP transport

2017-11-13 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1411 Please revert the `NewTHttpClient` part of this PR, since `THttpPostClient` is deprecated and is just an alias for `THttpClient` since 0dd82358. ---

[GitHub] thrift issue #1411: Fix remote client for HTTP transport

2017-11-20 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1411 @jeking3 LGTM ---

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-11-16 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 > That needs to be documented in the dlang readme - would you be able to document that in a separate PR? Sorry for the delay. I'll open a ticket and send a patch over the weekend. ---

[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...

2017-11-03 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148796351 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \ rm -rf /tmp/* && \

[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...

2017-11-03 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148808675 --- Diff: lib/go/test/tests/client_error_test.go --- @@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T

[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...

2017-11-03 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148808724 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -221,3 +221,4 @@ ENV THRIFT_ROOT /thrift RUN mkdir -p $THRIFT_ROOT/src COPY Dockerfile

[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...

2017-11-03 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148798731 --- Diff: build/docker/ubuntu-trusty/Dockerfile --- @@ -217,6 +217,62 @@ RUN rm -rf /var/cache/apt/* && \ rm -rf /tmp/* && \

[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...

2017-11-03 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1382#discussion_r148797018 --- Diff: lib/go/test/tests/client_error_test.go --- @@ -411,7 +411,7 @@ func TestClientReportTTransportErrors(t *testing.T

[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library

2017-11-03 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1382 @jeking3 All done! I think we are good to merge, what do you think? ---

[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...

2018-01-09 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 ```go type MyServiceClient struct { c thrift.TClient *MyServiceBaseClient } type MyServiceBaseClient struct { c thrift.TClient

[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-01-09 Thread dcelasun
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 some

[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...

2018-01-09 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Ah, you have a service extending another service. I missed that. In that case, I think removing `c thrift.TClient` from `UserServiceClient` (and constructor functions) is the cleanest option

[GitHub] thrift issue #1461: THRIFT-4447: Golang: fix for (deprecated) New*ClientFact...

2018-01-09 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > Why are they deprecated? I probably only overlook sth, so bear with me My apologies, it's an unfortunate result of the patch changing authors and things getting mixed up in the proc

[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-01-09 Thread dcelasun
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

[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...

2018-01-09 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > So basically the logic would be to only add `c: thrift.TClient` to the `*Client` struct if `extends.empty()` in the generator? Exactly. ---

[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...

2018-01-10 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Happy to help. Once Travis is all green, please squash your commits so it'll be easier to merge. ---

[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...

2018-01-10 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 > GitHub can now automatically squash commits in PRs at merge time For Thrift, PRs are not merged, just closed. You'll see a commit with you as the author but someone else as the commit

[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...

2018-01-09 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Exactly. ---

[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...

2018-01-09 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1461 Any capitalized method on the client will potentially conflict with an RPC, e.g: ```thrift service Foo { string C() } ``` will generate uncompilable code. I suggest

[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-02-02 Thread dcelasun
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] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-02-02 Thread dcelasun
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] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-02-02 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1459 Looks *much* cleaner! ---

[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-02-02 Thread dcelasun
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] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-02-01 Thread dcelasun
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] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-02-01 Thread dcelasun
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 u

[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context

2018-02-01 Thread dcelasun
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

[GitHub] thrift issue #1505: Fixing java thrift compiler to generate constants in sta...

2018-03-09 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1505 Please add the issue number to your commit message and PR title. See [here](https://thrift.apache.org/docs/HowToContribute) (point 7 under Github). ---

[GitHub] thrift pull request #1479: THRIFT-4474: generate PHP code use PSR-4 style de...

2018-03-13 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1479#discussion_r174183088 --- Diff: lib/php/README.md --- @@ -20,34 +19,46 @@ KIND, either express or implied. See the License for the specific language governing permissions

[GitHub] thrift pull request #1479: THRIFT-4474: generate PHP code use PSR-4 style de...

2018-03-13 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1479#discussion_r174183533 --- Diff: lib/php/README.md --- @@ -20,34 +19,46 @@ KIND, either express or implied. See the License for the specific language governing permissions

[GitHub] thrift issue #1479: THRIFT-4474: generate PHP code use PSR-4 style default

2018-03-13 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1479 Added some doc comments. The actual code looks fine to me. ---

[GitHub] thrift issue #1479: THRIFT-4474: generate PHP code use PSR-4 style default

2018-03-14 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1479 @RobberPhex can you squash your commits? ---

[GitHub] thrift issue #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10

2018-03-15 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1507 Failures seem unrelated. Should I merge or do you need to something on docker.io? ---

[GitHub] thrift pull request #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10

2018-03-15 Thread dcelasun
GitHub user dcelasun opened a pull request: https://github.com/apache/thrift/pull/1507 THRIFT-4516: Fix "go vet" warnings for Go 1.10 Opening this here because my free Travis account has its builds killed after 50 minutes. You can merge this pull request into a Git

[GitHub] thrift pull request #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10

2018-03-15 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1507#discussion_r174812332 --- Diff: lib/go/test/dontexportrwtest/compile_test.go --- @@ -29,10 +28,10 @@ import ( func TestReadWriteMethodsArePrivate(t *testing.T

[GitHub] thrift issue #1520: THRIFT-4531

2018-03-27 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1520 There are no changes in this PR. Please correct it and force push. ---

[GitHub] thrift issue #1520: THRIFT-4531

2018-03-27 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1520 Also, please squash your commits. ---

[GitHub] thrift issue #1523: Fix condition where TSimpleServer could exit AcceptLoop(...

2018-03-30 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1523 Travis failures are unrelated flaky tests (my own Travis build is fine). ---

[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...

2018-03-28 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1522 I don't think it's relevant either, seems like a flaky test. I think we are good to merge. @jeking3 any idea on the failing appveyor test, have you seen it before? I'm inclined to ignore

[GitHub] thrift issue #1523: Fix condition where TSimpleServer could exit AcceptLoop(...

2018-03-29 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1523 Thanks for the PR. Seems like this was [introduced](https://github.com/apache/thrift/blame/930428438c0b6c8f60560cbb7dcad79042badacb/lib/go/thrift/simple_server.go#L131) by THRIFT-4243 (PR #1302

[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...

2018-03-28 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1522 No need, I'll squash once Travis is all green. ---

[GitHub] thrift issue #1515: add connect timeout, support accurate send timeout

2018-03-21 Thread dcelasun
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1515 Please don't change the code style while you are doing unrelated changes, it makes it very hard to review what *actually* changed. Even with the changes, there are lots of style inconsistencies

[GitHub] thrift pull request #1440: Enhancement binary_protocol with frametransport

2018-03-17 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1440#discussion_r175252293 --- Diff: lib/go/thrift/fast_buffer.go --- @@ -0,0 +1,257 @@ +package thrift + +import ( + "fmt" + "unsafe"

[GitHub] thrift pull request #1440: Enhancement binary_protocol with frametransport

2018-03-17 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1440#discussion_r175252297 --- Diff: lib/go/thrift/fast_buffer.go --- @@ -0,0 +1,257 @@ +package thrift + +import ( + "fmt" + "unsafe"

[GitHub] thrift pull request #1440: Enhancement binary_protocol with frametransport

2018-03-17 Thread dcelasun
Github user dcelasun commented on a diff in the pull request: https://github.com/apache/thrift/pull/1440#discussion_r175252307 --- Diff: lib/go/thrift/fast_buffer.go --- @@ -0,0 +1,257 @@ +package thrift + +import ( + "fmt" + "unsafe"

  1   2   >