You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by dcelasun <gi...@git.apache.org> on 2017/01/15 10:05:33 UTC
[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 <dc...@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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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<string> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1156
Oopsie. After I read the word "map" above I must have been mentally switched to maps somehow. My mistake.
Nevertheless what I said about panic and errors in general above still holds true. At this time we have exactly 0 (zero) panics in the code under /lib/go/thrift. So I would argue that introducing a panic at least breaks the usual patterns of that library.
> Speaking of which, panicking in case of programmer error is very common and idiomatic in Go. The standard library is full of such panics
Put four Go experts together and you get five opinions, all idiomatic. Especially about the high and lofty Go error handling design principles:
> In Go, error handling is important. **The language's design and conventions encourage you to explicitly check for errors** where they occur (as distinct from the convention in other languages of throwing exceptions and sometimes catching them).
Or mostly assigning them to `_` as many people do in Go. That's for the "encourage you to explicitly check for errors" part. In contrast, an exception must be caught, not only "sometimes", because there is no other error handling option.
> The decision to not include exceptions in Go is an example of its **simplicity** and orthogonality. Using multiple return values and a simple convention, **Go solves the problem** of letting programmers know when things have gone wrong and **reserves panic for the truly exceptional**.
I can't see any "simplicity" in replacing exceptions by panics, "encouraging" error handling by leading people into misusing the `_` operator and at the same time claiming "*this time, we get it right*". Go's error handling is FUBAR from the beginning to the end. Good intentions, absolutely, but the implementation .... All personal opinion, of course,
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1156#discussion_r97858435
--- 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 --
As long as it is a compile time failure if someone doesn't change it, that's good. In C++ (C++03, or C++11 without the "override" keyword being used) if you change the signature of a virtual method, anything that overrides it will silently define a new method instead of overriding what you want. It's pretty messy.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1156
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1156
Ok, all fine. Don't panic ;-)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1156#discussion_r97841167
--- 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 --
This looks like a breaking change; any generated go implementation's handler will need to change. How will this be communicated? What happens to existing handlers that implement the older method?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1156
Ok, thanks.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1156
```
src/common/clientserver_test.go:61: cannot use handler (type *MockThriftTest) as type thrifttest.ThriftTest in argument to GetServerParams:
*MockThriftTest does not implement thrifttest.ThriftTest (wrong type for TestSet method)
have TestSet(map[int32]struct {}) (map[int32]struct {}, error)
want TestSet([]int32) ([]int32, error)
src/common/clientserver_test.go:226: cannot use s (type map[int32]struct {}) as type []int32 in argument to client.TestSet
FAIL common [build failed]
make[2]: *** [check] Error 2
make[2]: Leaving directory `/thrift/src/test/go'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/thrift/src/test'
make: *** [check-recursive] Error 1
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1156
Hi @dcelasun,any news? In caseyou're donw with it, could you rebase the PR to trigger a new Travis build?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1156
+1
Sent from mobile device, please ignore spelling mistakes.
________________________________
Von: Duru Can Celasun
Gesendet: 16.01.2017 08:00
An: apache/thrift
Cc: Jens Geyer; Mention
Betreff: Re: [apache/thrift] THRIFT-4011 Use slices for Thrift sets (#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?
-
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/apache/thrift/pull/1156#issuecomment-272787325>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFkH7U-4jT-CNWuziuVaqCK6G_FWI8Koks5rSxWOgaJpZM4Lj4Hw>.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by dcelasun <gi...@git.apache.org>.
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<T>` 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1156: THRIFT-4011 Use slices for Thrift sets
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1156
http://thrift.apache.org/docs/types states that (as one would expect) a Thrift map is defined as "A map of **strictly unique keys** to values. Translates to an STL map, Java HashMap, PHP associative array, Python/Ruby dictionary, etc.".
Given that, I would say it could be one option to error, when the user inserts a duplicate. On the other hand, in that case the better option could be to simply replace the current value.
But **what should not happen is that serialized data come in with duplicated keys**. That would be clearly an error as it is a violation of the rule above. In that case it might be absolutely ok to return some kind of error.
Re `panic()`, from my understanding of Go that's not the idiomatic way to go, since everything form tha above is an easily recoverable error.
Further reading:
* https://blog.golang.org/error-handling-and-go
* https://golang.org/doc/effective_go.html#panic
* http://stackoverflow.com/questions/28472922/when-to-use-os-exit-and-panic-in-golang
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1156: THRIFT-4011 Use slices for Thrift sets
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1156#discussion_r100088653
--- 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 --
Syntax looks much better than before.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---