[GitHub] thrift issue #1523: Fix condition where TSimpleServer could exit AcceptLoop(...
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 #1523: Fix condition where TSimpleServer could exit AcceptLoop(...
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) and unfortunately made its way into the 0.11 release. ---
[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...
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 it unless you say otherwise. ---
[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...
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 #1520: THRIFT-4531
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1520 Also, please squash your commits. ---
[GitHub] thrift issue #1520: THRIFT-4531
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 #1515: add connect timeout, support accurate send timeout
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 like opening braces for functions are sometimes on the same line, sometimes on a new line. So please revert any style-only changes and follow the existing style. If you want to also change/improve code style, open a separate PR for that. Thank you. ---
[GitHub] thrift pull request #1440: Enhancement binary_protocol with frametransport
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"
+)
+
+type FastFrameBuffer struct {
+ w *TFramedTransport //
+ b []byte
+ wIdx int // write-offset, reset when Flush
+ frameSize int
+ rIdx int // read-offset, reset when read-new-frame
+ buf [4]byte // only for write data-len
+}
+
+func NewFastBuffer(w *TFramedTransport, size int) *FastFrameBuffer {
+ return {
+ w:w,
+ b:make([]byte, size),
+ rIdx: 0,
+ wIdx: 0,
+ }
+}
+
+func (p *FastFrameBuffer) WritByte(b int8) {
+ if p.wIdx+1 > p.Cap() {
+ p.grow(2*p.Cap() + 1)
+ }
+ p.b[p.wIdx] = byte(b)
+ p.wIdx += 1
+}
+
+func (p *FastFrameBuffer) WriteBytes(b []byte) {
+ if p.wIdx+len(b) > p.Cap() {
+ p.grow(2*p.Cap() + len(b))
+ }
+ copy(p.b[p.wIdx:], b)
+ p.wIdx += len(b)
+}
+
+func (p *FastFrameBuffer) WriteI16(i uint16) {
+ if p.wIdx+2 > p.Cap() {
+ p.grow(2*p.Cap() + 2)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 8), byte(i)})
+ p.wIdx += 2
+}
+
+func (p *FastFrameBuffer) WriteI32(i uint32) {
+ if p.wIdx+4 > p.Cap() {
+ p.grow(2*p.Cap() + 4)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 24), byte(i >> 16), byte(i >> 8),
byte(i)})
+ p.wIdx += 4
+}
+
+func (p *FastFrameBuffer) WriteI64(i uint64) {
+ if p.wIdx+8 > p.Cap() {
+ p.grow(2*p.Cap() + 8)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 56), byte(i >> 48), byte(i >> 40),
byte(i >> 32), byte(i >> 24), byte(i >> 16), byte(i >> 8), byte(i)})
+ p.wIdx += 8
+}
+
+func (p *FastFrameBuffer) WriteString(s string) {
+ if p.wIdx+len(s) > p.Cap() {
+ p.grow(2*p.Cap() + len(s))
+ }
+ copy(p.b[p.wIdx:], str2bytes(s))
+ p.wIdx += len(s)
+}
+
+func (p *FastFrameBuffer) Len() int {
+ return len(p.b)
+}
+
+func (p *FastFrameBuffer) Cap() int {
+ return cap(p.b)
+}
+
+func (p *FastFrameBuffer) Flush() error {
+ p.buf[0] = byte(p.wIdx >> 24)
+ p.buf[1] = byte(p.wIdx >> 16)
+ p.buf[2] = byte(p.wIdx >> 8)
+ p.buf[3] = byte(p.wIdx)
+ _, err := p.w.transport.Write(p.buf[:4])
+
+ if err != nil {
+ return fmt.Errorf("Flush Write-Len failed, err: %v\n", err)
+ }
+ _, err = p.w.transport.Write(p.b[:p.wIdx])
+ if err != nil {
+ return fmt.Errorf("Flush Write-Dat failed, err: %v\n", err)
+ }
+ p.ResetWriter()
+ p.w.transport.Flush()
+ return nil
+}
+
+func (p *FastFrameBuffer) ResetWriter() {
+ p.wIdx = 0
+}
+
+func (p *FastFrameBuffer) ResetReader() {
+ p.rIdx = 0
+}
+
+func (p *FastFrameBuffer) grow(n int) {
+ b := make([]byte, n)
+ copy(b, p.b[0:])
+ p.b = b
+}
+
+func (p *FastFrameBuffer) ReadByte() (c byte, err error) {
+ if p.frameSize == 0 {
+ p.frameSize, err = p.readFrameHeader()
+ if err != nil {
+ return
+ }
+ _, err = p.readAll(p.frameSize)
+ if err != nil {
+ return
+ }
+ }
+ if p.frameSize < 1 {
+ return 0, fmt.Errorf("Not enought frame size %d to read %d
bytes", p.frameSize, 1)
--- End diff --
s/enought/enough everywhere in the file.
---
[GitHub] thrift pull request #1440: Enhancement binary_protocol with frametransport
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"
+)
+
+type FastFrameBuffer struct {
+ w *TFramedTransport //
+ b []byte
+ wIdx int // write-offset, reset when Flush
+ frameSize int
+ rIdx int // read-offset, reset when read-new-frame
+ buf [4]byte // only for write data-len
+}
+
+func NewFastBuffer(w *TFramedTransport, size int) *FastFrameBuffer {
+ return {
+ w:w,
+ b:make([]byte, size),
+ rIdx: 0,
+ wIdx: 0,
+ }
+}
+
+func (p *FastFrameBuffer) WritByte(b int8) {
+ if p.wIdx+1 > p.Cap() {
+ p.grow(2*p.Cap() + 1)
+ }
+ p.b[p.wIdx] = byte(b)
+ p.wIdx += 1
+}
+
+func (p *FastFrameBuffer) WriteBytes(b []byte) {
+ if p.wIdx+len(b) > p.Cap() {
+ p.grow(2*p.Cap() + len(b))
+ }
+ copy(p.b[p.wIdx:], b)
+ p.wIdx += len(b)
+}
+
+func (p *FastFrameBuffer) WriteI16(i uint16) {
+ if p.wIdx+2 > p.Cap() {
+ p.grow(2*p.Cap() + 2)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 8), byte(i)})
+ p.wIdx += 2
+}
+
+func (p *FastFrameBuffer) WriteI32(i uint32) {
+ if p.wIdx+4 > p.Cap() {
+ p.grow(2*p.Cap() + 4)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 24), byte(i >> 16), byte(i >> 8),
byte(i)})
+ p.wIdx += 4
+}
+
+func (p *FastFrameBuffer) WriteI64(i uint64) {
+ if p.wIdx+8 > p.Cap() {
+ p.grow(2*p.Cap() + 8)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 56), byte(i >> 48), byte(i >> 40),
byte(i >> 32), byte(i >> 24), byte(i >> 16), byte(i >> 8), byte(i)})
+ p.wIdx += 8
+}
+
+func (p *FastFrameBuffer) WriteString(s string) {
+ if p.wIdx+len(s) > p.Cap() {
+ p.grow(2*p.Cap() + len(s))
+ }
+ copy(p.b[p.wIdx:], str2bytes(s))
+ p.wIdx += len(s)
+}
+
+func (p *FastFrameBuffer) Len() int {
+ return len(p.b)
+}
+
+func (p *FastFrameBuffer) Cap() int {
+ return cap(p.b)
+}
+
+func (p *FastFrameBuffer) Flush() error {
+ p.buf[0] = byte(p.wIdx >> 24)
+ p.buf[1] = byte(p.wIdx >> 16)
+ p.buf[2] = byte(p.wIdx >> 8)
+ p.buf[3] = byte(p.wIdx)
+ _, err := p.w.transport.Write(p.buf[:4])
+
+ if err != nil {
+ return fmt.Errorf("Flush Write-Len failed, err: %v\n", err)
+ }
+ _, err = p.w.transport.Write(p.b[:p.wIdx])
+ if err != nil {
+ return fmt.Errorf("Flush Write-Dat failed, err: %v\n", err)
+ }
+ p.ResetWriter()
+ p.w.transport.Flush()
+ return nil
+}
+
+func (p *FastFrameBuffer) ResetWriter() {
+ p.wIdx = 0
+}
+
+func (p *FastFrameBuffer) ResetReader() {
+ p.rIdx = 0
+}
+
+func (p *FastFrameBuffer) grow(n int) {
+ b := make([]byte, n)
+ copy(b, p.b[0:])
+ p.b = b
+}
+
+func (p *FastFrameBuffer) ReadByte() (c byte, err error) {
+ if p.frameSize == 0 {
+ p.frameSize, err = p.readFrameHeader()
+ if err != nil {
+ return
+ }
+ _, err = p.readAll(p.frameSize)
+ if err != nil {
+ return
+ }
+ }
+ if p.frameSize < 1 {
+ return 0, fmt.Errorf("Not enought frame size %d to read %d
bytes", p.frameSize, 1)
+ }
+ c = p.b[p.rIdx]
+ if err == nil {
+ p.frameSize--
+ p.rIdx += 1
+ }
+ return
+}
+
+// maybe read-bytes means ReadN
+func (p *FastFrameBuffer) ReadN(num int) (b []byte, err error) {
+ if p.frameSize == 0 {
+ p.frameSize, err = p.readFrameHeader()
+ if err != nil {
+ return
+ }
+ _, err = p.readAll(p.frameSize)
+ if err != nil {
+ return
+ }
+ }
+ if p.frameSize < num {
+ return nil, fmt.Errorf("Not enought frame size %d to read %d
bytes", p.frameSize, num)
+ }
+ b = p.b[p.rIdx : p.rIdx+num]
+ p.frameSize = p.frameSize - num
+ if p.frameSize < 0 {
+ return nil, fmt.Errorf("Negative frame size")
+ }
+ p.rIdx += num
+ return b, nil
+}
+
+func (p *FastFrameBuffer) R
[GitHub] thrift pull request #1440: Enhancement binary_protocol with frametransport
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"
+)
+
+type FastFrameBuffer struct {
+ w *TFramedTransport //
+ b []byte
+ wIdx int // write-offset, reset when Flush
+ frameSize int
+ rIdx int // read-offset, reset when read-new-frame
+ buf [4]byte // only for write data-len
+}
+
+func NewFastBuffer(w *TFramedTransport, size int) *FastFrameBuffer {
+ return {
+ w:w,
+ b:make([]byte, size),
+ rIdx: 0,
+ wIdx: 0,
+ }
+}
+
+func (p *FastFrameBuffer) WritByte(b int8) {
+ if p.wIdx+1 > p.Cap() {
+ p.grow(2*p.Cap() + 1)
+ }
+ p.b[p.wIdx] = byte(b)
+ p.wIdx += 1
+}
+
+func (p *FastFrameBuffer) WriteBytes(b []byte) {
+ if p.wIdx+len(b) > p.Cap() {
+ p.grow(2*p.Cap() + len(b))
+ }
+ copy(p.b[p.wIdx:], b)
+ p.wIdx += len(b)
+}
+
+func (p *FastFrameBuffer) WriteI16(i uint16) {
+ if p.wIdx+2 > p.Cap() {
+ p.grow(2*p.Cap() + 2)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 8), byte(i)})
+ p.wIdx += 2
+}
+
+func (p *FastFrameBuffer) WriteI32(i uint32) {
+ if p.wIdx+4 > p.Cap() {
+ p.grow(2*p.Cap() + 4)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 24), byte(i >> 16), byte(i >> 8),
byte(i)})
+ p.wIdx += 4
+}
+
+func (p *FastFrameBuffer) WriteI64(i uint64) {
+ if p.wIdx+8 > p.Cap() {
+ p.grow(2*p.Cap() + 8)
+ }
+ copy(p.b[p.wIdx:], []byte{byte(i >> 56), byte(i >> 48), byte(i >> 40),
byte(i >> 32), byte(i >> 24), byte(i >> 16), byte(i >> 8), byte(i)})
+ p.wIdx += 8
+}
+
+func (p *FastFrameBuffer) WriteString(s string) {
+ if p.wIdx+len(s) > p.Cap() {
+ p.grow(2*p.Cap() + len(s))
+ }
+ copy(p.b[p.wIdx:], str2bytes(s))
+ p.wIdx += len(s)
+}
+
+func (p *FastFrameBuffer) Len() int {
+ return len(p.b)
+}
+
+func (p *FastFrameBuffer) Cap() int {
+ return cap(p.b)
+}
+
+func (p *FastFrameBuffer) Flush() error {
+ p.buf[0] = byte(p.wIdx >> 24)
+ p.buf[1] = byte(p.wIdx >> 16)
+ p.buf[2] = byte(p.wIdx >> 8)
+ p.buf[3] = byte(p.wIdx)
+ _, err := p.w.transport.Write(p.buf[:4])
+
+ if err != nil {
+ return fmt.Errorf("Flush Write-Len failed, err: %v\n", err)
+ }
+ _, err = p.w.transport.Write(p.b[:p.wIdx])
+ if err != nil {
+ return fmt.Errorf("Flush Write-Dat failed, err: %v\n", err)
+ }
+ p.ResetWriter()
+ p.w.transport.Flush()
+ return nil
+}
+
+func (p *FastFrameBuffer) ResetWriter() {
+ p.wIdx = 0
+}
+
+func (p *FastFrameBuffer) ResetReader() {
+ p.rIdx = 0
+}
+
+func (p *FastFrameBuffer) grow(n int) {
+ b := make([]byte, n)
+ copy(b, p.b[0:])
+ p.b = b
+}
+
+func (p *FastFrameBuffer) ReadByte() (c byte, err error) {
+ if p.frameSize == 0 {
+ p.frameSize, err = p.readFrameHeader()
+ if err != nil {
+ return
+ }
+ _, err = p.readAll(p.frameSize)
+ if err != nil {
+ return
+ }
+ }
+ if p.frameSize < 1 {
+ return 0, fmt.Errorf("Not enought frame size %d to read %d
bytes", p.frameSize, 1)
+ }
+ c = p.b[p.rIdx]
+ if err == nil {
+ p.frameSize--
+ p.rIdx += 1
+ }
+ return
+}
+
+// maybe read-bytes means ReadN
+func (p *FastFrameBuffer) ReadN(num int) (b []byte, err error) {
+ if p.frameSize == 0 {
+ p.frameSize, err = p.readFrameHeader()
+ if err != nil {
+ return
+ }
+ _, err = p.readAll(p.frameSize)
+ if err != nil {
+ return
+ }
+ }
+ if p.frameSize < num {
+ return nil, fmt.Errorf("Not enought frame size %d to read %d
bytes", p.frameSize, num)
+ }
+ b = p.b[p.rIdx : p.rIdx+num]
+ p.frameSize = p.frameSize - num
+ if p.frameSize < 0 {
+ return nil, fmt.Errorf("Negative frame size")
+ }
+ p.rIdx += num
+ return b, nil
+}
+
+func (p *FastFrameBuffer) R
[GitHub] thrift issue #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10
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
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) {
// This will only compile if read/write methods exist
s := NewTestStruct()
- fmt.Sprintf("%v", s.read)
- fmt.Sprintf("%v", s.write)
+ _ = s.read
--- End diff --
The problem is `s.read` and `s.write` are functions and there just isn't
any way to print functions as strings, so I had to find another way to use them
without trying to stringify them.
---
[GitHub] thrift pull request #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10
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 repository by running: $ git pull https://github.com/dcelasun/thrift THRIFT-4516 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1507.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1507 commit a9efd1abd4fd4862d8e967ec207015af79494b6c Author: D. Can Celasun <can@...> Date: 2018-03-15T11:52:37Z THRIFT-4516: Fix "go vet" warnings for Go 1.10 Client: go ---
[GitHub] thrift issue #1479: THRIFT-4474: generate PHP code use PSR-4 style default
Github user dcelasun commented on the issue: https://github.com/apache/thrift/pull/1479 @RobberPhex can you squash your commits? ---
[GitHub] thrift pull request #1479: THRIFT-4474: generate PHP code use PSR-4 style de...
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 and limitations under the License. -Using Thrift with PHP -= +# Using Thrift with PHP Thrift requires PHP 5. Thrift makes as few assumptions about your PHP environment as possible while trying to make some more advanced PHP features (i.e. APC cacheing using asbolute path URLs) as simple as possible. To use Thrift in your PHP codebase, take the following steps: -#1) Copy all of thrift/lib/php/lib into your PHP codebase -#2) Configure Symfony Autoloader (or whatever you usually use) +1. Copy all of thrift/lib/php/lib into your PHP codebase +2. Configure Symfony Autoloader (or whatever you usually use) After that, you have to manually include the Thrift package created by the compiler: +``` require_once 'packages/Service/Service.php'; require_once 'packages/Service/Types.php'; +``` -Dependencies - +# Dependencies PHP_INT_SIZE - This built-in signals whether your architecture is 32 or 64 bit and is - used by the TBinaryProtocol to properly use pack() and unpack() to - serialize data. +This built-in signals whether your architecture is 32 or 64 bit and is +used by the TBinaryProtocol to properly use pack() and unpack() to +serialize data. apc_fetch(), apc_store() - APC cache is used by the TSocketPool class. If you do not have APC installed, - Thrift will fill in null stub function definitions. +APC cache is used by the TSocketPool class. If you do not have APC installed, +Thrift will fill in null stub function definitions. + +# Breaking Changes + +## 0.12.0 + +1. For thrift compiler, `psr4` flag is opened default, generated code match psr4 code struct. --- End diff -- Better phrasing: `PSR-4` (make this a link to https://www.php-fig.org/psr/psr-4/) loader is now the default. If you want to use class maps instead, use `-gen php:classmap`. ---
[GitHub] thrift pull request #1479: THRIFT-4474: generate PHP code use PSR-4 style de...
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 and limitations
under the License.
-Using Thrift with PHP
-=
+# Using Thrift with PHP
Thrift requires PHP 5. Thrift makes as few assumptions about your PHP
environment as possible while trying to make some more advanced PHP
features (i.e. APC cacheing using asbolute path URLs) as simple as
possible.
To use Thrift in your PHP codebase, take the following steps:
-#1) Copy all of thrift/lib/php/lib into your PHP codebase
-#2) Configure Symfony Autoloader (or whatever you usually use)
+1. Copy all of thrift/lib/php/lib into your PHP codebase
+2. Configure Symfony Autoloader (or whatever you usually use)
After that, you have to manually include the Thrift package
created by the compiler:
+```
require_once 'packages/Service/Service.php';
require_once 'packages/Service/Types.php';
+```
-Dependencies
-
+# Dependencies
PHP_INT_SIZE
- This built-in signals whether your architecture is 32 or 64 bit and is
- used by the TBinaryProtocol to properly use pack() and unpack() to
- serialize data.
+This built-in signals whether your architecture is 32 or 64 bit and is
+used by the TBinaryProtocol to properly use pack() and unpack() to
+serialize data.
apc_fetch(), apc_store()
- APC cache is used by the TSocketPool class. If you do not have APC
installed,
- Thrift will fill in null stub function definitions.
+APC cache is used by the TSocketPool class. If you do not have APC
installed,
+Thrift will fill in null stub function definitions.
+
+# Breaking Changes
+
+## 0.12.0
+
+1. For thrift compiler, `psr4` flag is opened default, generated code
match psr4 code struct.
+
+If you want old-style directory struct, use `classmap` option. (i.e.
`-gen php:classmap`)
+
+2. For ThriftClassloader, if use psr4 code struct, must use
`$thriftClassLoader->registerNamespace('namespace', '')` to register
autoload.
--- End diff --
Better phrasing:
If using PSR4, use `$thriftClassLoader->registerNamespace('namespace',
'')` instead of `$thriftClassLoader->registerDefinition('namespace',
'')`.
---
[GitHub] thrift issue #1479: THRIFT-4474: generate PHP code use PSR-4 style default
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 #1505: Fixing java thrift compiler to generate constants in sta...
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 issue #1459: THRIFT-4448: Golang: do something with context.Context
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
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 :/ ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
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
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
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). ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
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. ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
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. ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
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 committer, like [these](https://github.com/apache/thrift/commits/master). ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
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...
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...
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 we use something like `Client_`
as the method name. It looks ugly, but there is precedent for it (conflicts
with keywords etc).
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
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...
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. That way, we would only have a `TClient` within the `UserServiceBase` and it can be reused by any extending service. Thoughts? ---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
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...
}
}
```
---
[GitHub] thrift issue #1459: THRIFT-4448: Golang: do something with context.Context
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). ---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: Fix panic on p.c.Call when using de...
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
}
```
@johnboiles Are you getting this second "Base" struct with 0.11 compiler?
Because I can't reproduce.
> Also should I remove the // Deprecated comments if these are not actually
deprecated?
Yes please.
---
[GitHub] thrift issue #1461: THRIFT-4447: Golang: fix for (deprecated) New*ClientFact...
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 process. I should have removed the deprecated comments, especially since those constructors *are* compatible. The bugfix looks good. ---
[GitHub] thrift issue #1411: Fix remote client for HTTP transport
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
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 issue #1411: Fix remote client for HTTP transport
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 #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
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 pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
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 $THRIFT_ROOT/ WORKDIR $THRIFT_ROOT/src + --- End diff -- My bad, will revert this as well. ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
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) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport,
protocol, protocol)
+ client :=
errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
Good point :) I'll add both, but it'll have to wait till tomorrow as I've
already left the office.
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
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/* && \ rm -rf /var/tmp/* +# Ruby --- End diff -- Fixed. ---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
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) {
if !prepareClientCallReply(protocol, i, err) {
return
}
- client := errortest.NewErrorTestClientProtocol(transport,
protocol, protocol)
+ client :=
errortest.NewErrorTestClient(thrift.NewTStandardClient(protocol, protocol))
--- End diff --
No, it's backwards compatible now (see
[here](https://github.com/dcelasun/thrift/blob/555efe5aefe9619a900471e56e86906d40bc96b9/compiler/cpp/src/thrift/generate/t_go_generator.cc#L1889-L1930)),
the test simply uses the new method.
I've also tested it with the integration of tests of a real, production
app. It works as expected.
---
[GitHub] thrift pull request #1382: THRIFT-4285 Move TX/RX methods from gen. code to ...
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/* && \ rm -rf /var/tmp/* +# Ruby --- End diff -- It's because I messed up the rebase. Will fix. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
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 #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
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. I agree avoiding the BC break is worth it if possible. > I was away for a couple days on college tours with my son, sorry for the delay. No worries, I just wanted to ping you in case this slipped through the cracks. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
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
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. Though if you still think we shouldn't do it, I can change the tests to use older APIs. I didn't want to touch it since I didn't write the original patch. > It looks like this may not be backwards compatible with existing code - is there any way to put in an adapter that would allow existing code to continue working? I don't think so. Several interfaces had to change since what used to be returned from generated methods is now returned from the library. Good news is the changes are limited to client, server and protocol interfaces (they don't affect the signature for RPCs) so updating to 0.11 should be a few lines of change (initializing the server/client) for most people. Just to make sure, I'll update a real application to this patch and post my experiences here. ---
[GitHub] thrift issue #1382: THRIFT-4285 Move TX/RX methods from gen. code to library
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 ...
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 to the transport. This patch was originally written by [Chris Bannister](https://issues.apache.org/jira/browse/THRIFT-4285) but it seemed abandoned and no longer applied cleanly to master. I fixed it in order to get things moving again. I've also bumped `Dockerfile`s to Go 1.9 since `t.Run` in `testing/T` doesn't exist before that and we were already using 1.9 for the CentOS container. It would be great if this can be merged before 0.11 is tagged. You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift THRIFT-4285 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1382.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1382 ---
[GitHub] thrift pull request #1381: THRIFT-4285: Move TX/RX methods from gen. code to...
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...
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 to the transport. This patch was originally written by [Chris Bannister](https://issues.apache.org/jira/browse/THRIFT-4285) but it seemed abandoned and no longer applied cleanly to master. I fixed it in order to get things moving again. It would be great if we can get this in before 0.11 is tagged. Client: Go You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift THRIFT-4285 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1381.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1381 commit 0e03968520db377bce9f85caf55d7a218f9d59d5 Author: D. Can Celasun <c...@dcc.im> Date: 2017-09-21T13:21:00Z 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 to the transport. Client: Go ---
[GitHub] thrift issue #1375: THRIFT-4346: Allow ZlibTransportFactory to wrap other fa...
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 #1375: THRIFT-4346: Allow ZlibTransportFactory to wrap o...
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 of NewTZlibTransport
func (p *TZlibTransportFactory) GetTransport(trans TTransport)
(TTransport, error) {
+ if p.factory != nil {
+ // wrap other factory
+ var err error
+ trans, err = p.factory.GetTransport(trans)
+ if err != nil {
+ return nil, err
+ }
+ }
return NewTZlibTransport(trans, p.level)
}
// NewTZlibTransportFactory constructs a new instance of
NewTZlibTransportFactory
func NewTZlibTransportFactory(level int) *TZlibTransportFactory {
- return {level: level}
+ return {level: level, factory: nil}
+}
+
+// NewTZlibTransportFactory constructs a new instance of
NewTZlibTransportFactory
--- End diff --
The second `NewTZlibTransportFactory` should be `TZlibTransportFactory`
---
[GitHub] thrift issue #1341: THRIFT-4307: Make ssl-open timeout effective in golang c...
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 your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1312: THRIFT-4260: Add context as first arg for client method.
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 this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1312: Add context as first arg for client method.
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 this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1309: Use build tags to support context.
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 the container (and hence with Go 1.7+) you need to remove `legacy_context` and `thirdparty-dep` from the Makefile. @taozle Maybe there is a way to check the go version before adding the flag? --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1309: Use build tags to support context.
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 it somehow. The `check` target already includes `thirdparty-dep`, so it should work with `make -k check` when you are running Go<1.7. What were your exact steps that resulted in this error? What's your Go version? --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1309: Use build tags to support context.
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 a PR to update the Go tutorial with the new handler signature. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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, "Go", " Disable automatic spelling correction of initialisms (e.g. \"URL\")\n" \ "read_write_private\n" " Make read/write methods private, default is public Read/Write\n" \ - "use_context\n" - " Make service method receive a context as first argument.\n") + "legacy_context\n" + " Use lagacy x/net/context instead of context in go<1.7.\n") --- End diff -- Typo s/lagacy/legacy --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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, "Go", "read_write_private\n" " Make read/write methods private, default is public Read/Write\n" \ "use_context\n" - " Make service method receive a context as first argument.\n") + " Make service method receive a context as first argument, only go1.7+ is supported.\n") --- End diff -- Great, thanks for working on this! --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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, "Go", "read_write_private\n" " Make read/write methods private, default is public Read/Write\n" \ "use_context\n" - " Make service method receive a context as first argument.\n") + " Make service method receive a context as first argument, only go1.7+ is supported.\n") --- End diff -- > i think a control flag is still required to decided whether to use `context` or `x/net/context`. Hmm, I think you are right. Maybe something like `legacy_context`? --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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, "Go", "read_write_private\n" " Make read/write methods private, default is public Read/Write\n" \ "use_context\n" - " Make service method receive a context as first argument.\n") + " Make service method receive a context as first argument, only go1.7+ is supported.\n") --- End diff -- I don't think we should commit it. Right now, we don't have a `vendor` folder [in the library](https://github.com/apache/thrift/tree/master/lib/go). When people use `go get` (or other dependency tools) they'll get the `x/net/context` dependency automatically (if they are on Go<1.7). What do you think? --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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, "Go", "read_write_private\n" " Make read/write methods private, default is public Read/Write\n" \ "use_context\n" - " Make service method receive a context as first argument.\n") + " Make service method receive a context as first argument, only go1.7+ is supported.\n") --- End diff -- Breaking BC can be ok (happened before, see my commits) but if we remove `use_context` we can make sure all RPCs will have the same signature starting with `(ctx *Context, ...`. To be more clear: I think adding context support is *very* worthwhile and it's worth breaking BC for it (especially for a new release), but it would be better to support `context` for all Go versions, not just Go>1.7 --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-package thrift
-
-import (
- "context"
- "log"
- "runtime/debug"
- "sync"
-)
-
-/*
- * This is only a simple sample same as TSimpleServer but add context
- * usage support.
- */
-type TSimpleServer2 struct {
--- End diff --
Ok, but as I said in the other comment, if we use `x/net/context` for
Go<1.7, then we can have a single `TSimpleServer` with context support for all
Go versions.
---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
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, "Go", "read_write_private\n" " Make read/write methods private, default is public Read/Write\n" \ "use_context\n" - " Make service method receive a context as first argument.\n") + " Make service method receive a context as first argument, only go1.7+ is supported.\n") --- End diff -- You do have an explicit dependency to the new `context` package [here](https://github.com/taozle/thrift/blob/0df5965eb2a35793f19de1b2696e3c4afb6b5132/compiler/cpp/src/thrift/generate/t_go_generator.cc#L887). What I'm suggesting is to use `x/net/context` there if it's Go<1.7, so you don't need `use_context` at all. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 `multiplexed_protocol_go17.go` or something like that to be consistent with `TMultiplexedProcessor` in `multiplexed_protocol.go`? --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-package thrift
-
-import (
- "context"
- "log"
- "runtime/debug"
- "sync"
-)
-
-/*
- * This is only a simple sample same as TSimpleServer but add context
- * usage support.
- */
-type TSimpleServer2 struct {
--- End diff --
Why was this removed?
---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
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, "Go", "read_write_private\n" " Make read/write methods private, default is public Read/Write\n" \ "use_context\n" - " Make service method receive a context as first argument.\n") + " Make service method receive a context as first argument, only go1.7+ is supported.\n") --- End diff -- Can we support `x/net/context` for go<1.7? --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1287: THRIFT-4219 Refactor Go HTTP Client
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 not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1287: THRIFT-4219 Refactor Go HTTP Client
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, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1287: THRIFT-4219 Refactor Go HTTP Client
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 and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1287: THRIFT-4219 Refactor Go HTTP Client
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 languages. Constructor functions with "Post" in their names are all deprecated using the proper format [0] as all constructors now have the POST behaviour. Fixes THRIFT-4219. [0] https://github.com/golang/go/issues/10909#issuecomment-136492606 You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift THRIFT-4219 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1287.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1287 commit fdbf25f5f802fc9fde926b61c0fd31f95e39d000 Author: D. Can Celasun <dcela...@gmail.com> Date: 2017-06-10T14:37:47Z 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 languages. Constructor functions with "Post" in their names are all deprecated using the proper format [0] as all constructors now have the POST behaviour. Fixes THRIFT-4219. [0] https://github.com/golang/go/issues/10909#issuecomment-136492606 --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1285: THRIFT-4215/4216 Make transport factories return errors
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` is released. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1286: THRIFT-4215 Make transport factories return error...
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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1286: THRIFT-4215 Make transport factories return error...
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 ``` to ```go GetTransport(trans TTransport) (TTransport, error) ``` so the factory can pass any underlying error to the caller (previously such errors were ignored). This is a backwards incompatible change for anyone implementing custom transports, but it shouldn't effect anyone using the ones in this library. Fixes [THRIFT-4215](https://issues.apache.org/jira/browse/THRIFT-4215). Fixes [THRIFT-4216](https://issues.apache.org/jira/browse/THRIFT-4216). P.S: Some files have formatting-only changes due to `gofmt`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift THRIFT-4215 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1286.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1286 commit e66dbded3a57df8be17c678956abc305345305fc Author: D. Can Celasun <c...@dcc.im> Date: 2017-06-02T12:33:32Z THRIFT-4215 Make transport factories return errors This commit changes the signature of TTransportFactory.GetTransport from GetTransport(trans TTransport) TTransport to GetTransport(trans TTransport) (TTransport, error) so the factory can pass any underlying error to the caller (previously such errors were ignored). This is a backwards incompatible change for anyone implementing custom transports, but it shouldn't effect anyone using the ones in this library. Fixes THRIFT-4215. Fixes THRIFT-4216. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1285: THRIFT-4215/4216 Make transport factories return errors
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 as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1285: THRIFT-4215/4216 Make transport factories return ...
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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1285: THRIFT-4215/4216 Make transport factories return ...
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 ``` to ```go GetTransport(trans TTransport) (TTransport, error) ``` so the factory can pass any underlying error to the caller (previously such errors were ignored). This is a backwards incompatible change for anyone implementing custom transports, but it shouldn't effect anyone using the ones in this library. Fixes [THRIFT-4215](https://issues.apache.org/jira/browse/THRIFT-4215). Fixes [THRIFT-4216](https://issues.apache.org/jira/browse/THRIFT-4216). You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift THRIFT-4215 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1285.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1285 commit 312a4aadbb2106c9fb21360b87e0c2d5fea8a78e Author: D. Can Celasun <c...@dcc.im> Date: 2017-06-02T12:33:32Z THRIFT-4215 Make transport factories return errors This commit changes the signature of TTransportFactory.GetTransport from GetTransport(trans TTransport) TTransport to GetTransport(trans TTransport) (TTransport, error) so the factory can pass any underlying error to the caller (previously such errors were ignored). This is a backwards incompatible change for anyone implementing custom transports, but it shouldn't effect anyone using the ones in this library. Fixes THRIFT-4215. Fixes THRIFT-4216. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1281: THRIFT-3703 Unions Field Count Does Not Consider Map/Set...
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. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1281: THRIFT-3703 Unions Field Count Does Not Consider ...
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 for map/set/list
types fixes the generated Count* methods.
Sample IDL:
```thrift
union Foo{
1: map<bool,bool> u1
2: set u2
3: list u3
4: bool u4
}
```
Generated code before fix:
```go
func (p *Foo) CountSetFieldsFoo() int {
count := 0
if p.IsSetU4() {
count++
}
return count
}
```
After fix:
```go
func (p *Foo) CountSetFieldsFoo() int {
count := 0
if p.IsSetU1() {
count++
}
if p.IsSetU2() {
count++
}
if p.IsSetU3() {
count++
}
if p.IsSetU4() {
count++
}
return count
}
```
Fixes THRIFT-3703.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dcelasun/thrift THRIFT-3703
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1281.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1281
commit cacfd6fd041eba27540f61a897cc5d5826126197
Author: D. Can Celasun <c...@dcc.im>
Date: 2017-05-30T10:44:56Z
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 for map/set/list
types fixes the generated Count* methods.
Fixes THRIFT-3703.
---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1266: THRIFT-4197 [Go] Transparent gzip support for HTTP trans...
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 and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1266: THRIFT-4197 [Go] Transparent gzip support for HTTP trans...
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 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1266: THRIFT-4197 [Go] Transparent gzip support for HTT...
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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1266: THRIFT-4197 [Go] Transparent gzip support for HTT...
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 merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift go-gzip-support Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1266.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1266 commit b434d2d59b98f457600d0928e519b400462dee4a Author: D. Can Celasun <c...@dcc.im> Date: 2017-05-11T10:04:01Z [Go] Transparent gzip support for HTTP transport Check if the client supports gzip compressed responses and compress the response if it does. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1266: THRIFT-4197 [Go] Transparent gzip support for HTTP trans...
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/hs/CMakeFiles/haskell_library.dir/build.make:88: recipe for target 'lib/hs/thrift_cabal.stamp' failed make[2]: *** [lib/hs/thrift_cabal.stamp] Error 1 CMakeFiles/Makefile2:3926: recipe for target 'lib/hs/CMakeFiles/haskell_library.dir/all' failed make[1]: *** [lib/hs/CMakeFiles/haskell_library.dir/all] Error 2 ``` --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1266: THRIFT-4197 [Go] Transparent gzip support for HTT...
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 into a Git repository by running: $ git pull https://github.com/dcelasun/thrift go-gzip-support Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1266.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1266 commit b148a865bc2c2ade26dcffc0a7b18c85dc332464 Author: D. Can Celasun <c...@dcc.im> Date: 2017-05-11T10:04:01Z [Go] Transparent gzip support for HTTP transport Check if the client supports gzip compressed responses and compress the response if it does. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1207: THRIFT-4031: Fix invalid Go code generation for l...
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. See THRIFT-4031 for details. You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1207.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1207 commit c5609762b717db771985c8d3f74befda3764d180 Author: D. Can Celasun <c...@dcc.im> Date: 2017-03-03T11:03:24Z THRIFT-4031: Fix invalid code generation for list of typedef'ed built-in types This commit reverts 12d430e723b020f7a8ce42a40c19edf88f948367 which caused invalid code to be generated for certain types. See THRIFT-4031 for details. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
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 this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
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 not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
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 this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
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 failed")
}
- setTestInput := map[int32]struct{}{1: {}, 2: {}, 3: {}}
+ setTestInput := []int32{1, 2, 3}
--- End diff --
Yeah, it's also finally serializable to JSON for non-primitive types.
---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
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, so this is the only possible implementation that supports `set` for any `T`. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
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 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
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
return thing, nil
}
-func (p *ThriftTestHandler) TestSet(thing map[int32]struct{}) (r
map[int32]struct{}, err error) {
+func (p *ThriftTestHandler) TestSet(thing []int32) (r []int32, err error) {
--- End diff --
Yes, this is a breaking change which was originally discussed in
THRIFT-4011; I only started working on this PR after I was given the go ahead.
> How will this be communicated?
I would imagine posts to the mailing lists and an announcement on the
website, several weeks in advance of a new release?
> What happens to existing handlers that implement the older method?
Compiling IDLs with this patch will change the signature of any RPC or
struct in the generated Go code, so it's very easy to catch at compile time and
make the changes.
---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
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 here. That leaves us with silent deduplication (during serialization) in lib/go/thrift. Are you OK with that? --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
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 set, Java
HashSet, set in Python, etc. Note: PHP does not support sets, so it is treated
similar to a List
Similar to PHP, Go does not have a native type for sets, so the best thing
to do is to treat it similar to a list.
> Given that, I would say it could be one option to error, when the user
inserts a duplicate.
This is not possible, because the caller doesn't "insert" anything, they
simply return a slice. Consider the following:
```thrift
service Foo {
set bar() throws (1: Something error)
}
```
This generates an interface called `Foo` with the following method:
```go
type Foo interface {
Bar() ([]string, error)
}
```
So the user simply returns a string slice, the Thrift library has no
control over it. Once `Foo` returns, it's too late for Thrift itself to return
an error, only panic. Speaking of which, panicking in case of ***programmer
error*** is very common and idiomatic in Go. The standard library is full of
such panics (e.g search for "misuse"
[here](https://golang.org/src/sync/waitgroup.go)). I would consider returning a
non-unique slice for a Thrift set a programming error (and hence deserving a
panic), but if you disagree, I can update the PR with deduplication in the
library.
---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
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 for Thrift sets. I've specifically didn't touch the Go library since there was no agreement on panicking for duplicates. We have three options: 1. Leave it as is and add documentation stating deduplication is the caller's responsibility. 2. Silently deduplicate before serialization. 3. panic on duplicates. 2 and 3 probably requires [`reflect.DeepEqual`](https://golang.org/pkg/reflect/#DeepEqual), which is not ideal. @Jens-G Thoughts? You can merge this pull request into a Git repository by running: $ git pull https://github.com/dcelasun/thrift master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1156.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1156 commit 0e2e8c0b041300dafff641e19848a1e46df32bc6 Author: D. Can Celasun <dcela...@gmail.com> Date: 2017-01-15T09:53:19Z THRIFT-4011 Use slices for Thrift sets This commit changes the Go generator to use slices, instead of maps for Thrift sets. --- 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 is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
