On Wed, Feb 10, 2016 at 3:31 AM, Aki Sukegawa <[email protected]> wrote:
> +1 > > The current signature is really awkward and the new design makes sense to > me. > It wouldn't be pleasant for every existing user but this clutter is worth > removing IMO. > The patch looks great too, BTW. > Thanks for the feedback Aki. Since the patch is now green under the Jenkins Thrift-precommit job andyou have offered positive feedback, I'll proceed with the patch and request final review and merging by the committers. > On Tue, Feb 9, 2016 at 5:10 AM John Sirois <[email protected]> wrote: > > > On Mon, Feb 8, 2016 at 12:40 PM, John Sirois <[email protected]> wrote: > > > > > I'd like to propose a breaking change in both the java generated > service > > > code and in supporting library code. This change would change > > > parametrization and use of clients to be symmetric with servers in the > > java > > > async stack. > > > > > > Today the situation is as described in comments in > > > https://issues.apache.org/jira/browse/THRIFT-3112. > > > Although you might expect an async client call that returns an int to > > look > > > like: > > > `client.add(int first, int second, new AsyncMethodCallback<Integer>() > > > {...});` > > > > > > It in fact looks like: > > > `client.add(int first, int second, new > AsyncMethodCallback<add_call>() > > > {...});` > > > > > > Where `add_call` is a type with an `int getResult() throws ...` method > > the > > > client must call in the `AsyncMethodCallback.onComplete` implementation > > to > > > retrieve the result of the async call, or else its remote error. > > > > > > This has been the case since initial commit in 2010, but there are > > several > > > shortcomings, chief among these being: > > > 1. The parametrization is highly un-expected. Its neither the expected > > > return type (like the server parametrization) nor a type at the same > > > abstraction level as the decalring AsyncIface - its a type encapsulated > > by > > > the AsyncClient _implementation_. > > > 2. Because of the coupling to implementation in 1, we are not as free > to > > > modify the library implementation or generated code as we might be in a > > > world where the AsyncIface was composed purely of standard value types > > > (structs, ints, etc). > > > > > > It seems to me it would be wonderful to take our pre 1.0.0 status to > > > correct this confusing and leaky API. > > > > > > I'm interested in what folks think. > > > > > > > To help make this proposal more concrete, I have a pull request with the > > fully functioning diff with new tests here: > > https://github.com/apache/thrift/pull/840 > > >
