+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.
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 >
