On Thu, Jan 28, 2016 at 11:15 AM, John Sirois <[email protected]> wrote:
> > > On Thu, Jan 28, 2016 at 11:04 AM, BCG <[email protected]> wrote: > >> On 01/28/2016 12:06 PM, John Sirois wrote: >> >>> I've filed an issue that proposes either adding an option to the `java` >>> gen >>> lang or else adding a new gen kang (ala `javame`) that effects generation >>> of immutable thrift structs and unions for java: >>> https://issues.apache.org/jira/browse/THRIFT-3583 >>> >>> I have positive feedback from the Apache Aurora project where I >>> conducted a >>> full working experiment with the idea ( >>> http://markmail.org/message/a6sdqcelgokw6mwz) and some initial positive >>> feedback on the thrift ticket. Before proceeding to draw up patches, I'd >>> like to vet a few possible paths here and generally gauge the community >>> stance. >>> >>> From what I can tell, there are 3 primary challenges implementing an >>> option >>> to generate immutable entities - in order from most daunting to least: >>> >>> 1. The thrift java support library is fundamentally mutable. >>> It starts in TBase, where you need an instance before you can `read` it >>> from the wire, you can `reset` it and you can `deepCopy`. The latter is >>> not problematic, but the former are. It seems like, assuming the >>> immutable >>> option were to leverage the existing thrift java lib, the TBase interface >>> would need to be decomposed into a read interface and a write interface >>> and >>> then TBase would need to be built back up on top of those to maintain >>> backaward compatibility but open a seam to introduce the new object >>> model. >>> >>> I think this is a sensible approach. > > Noting on the basis of BCG's feedback (thanks!) and in the interest of landing this change in self-contained, backward-compatible increments, I'm proceeding along this path. Please yell if you have objections! >> 2. The java standard collection library is fundamentally mutable. >>> Its true, you can just use Collections.immutable{List,Map,Set} but it >>> would >>> be nice to communicate immutability in signatures - notably using guava's >>> Immutable{List,Map,Set} - as well as leverage the performance >>> optimizations >>> they've invested so much time in. Using guava though would present a >>> jar-hell situtation for thrift's consumers. We could roll our own >>> immutable collections - or interfaces - but this presents an impedance >>> mismatch problem for those using guava. >>> >> A possible solution to this would be to have an optional compiler flag >> for using Guava collections in the generated code. There are not huge >> differences in the APIs (from a code generation standpoint). This same >> approach is used for Sorted* collections in the Thrift compiler already. >> Users can then be responsible for adding their preferred version of Guava >> to their own application themselves (i.e., don't add it as a compile >> dependency to libthrift's pom). I'm not familiar with how stable the Guava >> API is, but I presume it is fairly easy to generate code that would be >> backwards compatible with many older versions of that library. > > > I think you're right here about guava stability. The APIs my experiment > changes linked below use have all been stable as long as I can remember - > I think all the way back to before the google-collections -> guava rename. > So forcing users to supply the guava dep would probably work just fine. > > >> >>> 3. The java standard library for older versions of java (<=1.7) is >>> fundamentally nullable. >>> It would be wonderful to present the option of representing nullable >>> fields >>> as Optional. Unfortunately the java stdlib doesn't carry this until >>> present - 1.8. Again there is the option of using guava's Optional with >>> the associated jar-hell problem. And also the roll our own option with >>> the >>> same impedance mismatch issues. >>> >> I have the same feeling as above... I think that this should be up to the >> user and controllable via a compiler flag... for instance maybe we could >> generate 1.7 compatible code by default, and have flags to select using >> Java 8's Optional or Guava's Optional (as long as references to Optional >> don't leak into the libthrift JAR, which is current built to run on Java 5: >> https://github.com/bgould/thrift/blob/master/lib/java/build.xml#L97 ). >> I'd also be in favor of making Java 8 for generated code and having a >> "java7" flag similar to the "java5" flag that already exists. Also maybe >> I'm missing the point - but is it not possible to have immutable structs >> that have nullable fields, or is using Optional required for immutability? >> > > You're correct. We can have immutable structs that have @Nullable fields, > I'd just like to modernize the API as a whole is undertaking this > endeavor. Although my demonstration series of changes [1][2][3] for Aurora > don't use Optional (Aurora is java 1.8 so I could), I think Aurora would > likely love to have this - its just a big mechanical change to adapt > today. Optional pairs particularly well with java 8 and the with'er > UnaryOperator mutator style implemented in the experiment changes below. > > [1] https://reviews.apache.org/r/42748/ > [2] https://reviews.apache.org/r/42749/ > [3] https://reviews.apache.org/r/42756/ > > >> >> Whatever the solution, I'm happy to help contribute and test this feature >> as work progresses. >> >> $.02 >> >> Ben >> >> >
