I'll agree with the "Cons" by referencing back to this thread: https://lists.apache.org/thread.html/caa8k_flvcmx+tyksxdmcxxe9y_zyohe4ovht9f2jb1wckob...@mail.gmail.com
On Tue, Feb 7, 2017 at 5:15 PM, Eugene Kirpichov < kirpic...@google.com.invalid> wrote: > Hello, > > I was auditing Beam for violations of PTransform style guide > https://beam.apache.org/contribute/ptransform-style-guide/ and came across > another style point that deserves discussion. > > Look at Count transform: > > public static <T> Combine.Globally<T, Long> globally() { > return Combine.globally(new CountFn<T>()); > } > > public static <K, V> Combine.PerKey<K, V, Long> perKey() { > return Combine.<K, V, Long>perKey(new CountFn<V>()); > } > > public static <T> PerElement<T> perElement() { > return new PerElement<>(); > } > > I asked myself: should globally() and perKey() also define wrapper classes > - e.g. should it be "public static <T> Globally<T, Long> globally()" where > "Globally" is a new inner class of Count? > > I argue that the answer is yes, but it's not clear-cut. > Cons: > - If we return a Combine.Globally, the user can use the features provided > by Combine.Globally - e.g. .withDefaults(), .withFanout(), > .asSingletonView(). > Pros: > - Style consistency with other transforms. Almost all transforms have their > own class, and their factory functions return that class. > - Implementation can evolve. However, in case of Count, that is unlikely. > - ...Or is it? If the transform has a concrete class, then the runner can > intercept that class and e.g. provide an (even) more efficient > implementation of Count.Globally. This gets much more awkward if the runner > has to intercept every Combine and check whether it's combining using a > CountFn. > > So, I propose to add this as a style guide rule, and solve the problem in > "Cons" by saying "Yeah, if you want the extra features of the transform > you're expanding into, you have to propagate them through the API of your > transform and delegate to the underlying transform manually". > > Thoughts? >
