Thanks for the explanation Bill. Makes sense to me. On Tue, Jun 12, 2018 at 9:21 AM, Bill Bejeck <bbej...@gmail.com> wrote:
> > Since there're only two values for the optional optimization config > > introduced by KAFKA-6935, I wonder the overloaded build method (with > Properties > > instance) would make the config unnecessary. > > Hi Ted, thanks for commenting. You raise a good point. Buy IMHO, yes we > still need the config as we want to give users the ability to turn > optimizations off/on explicitly and we haven't finalized anything > concerning how we'll pass in the parameters. Additionally, as we release > new versions, the config will give users the ability to choose to apply all > of the latest optimizations or stick with the previous version. > > Guozhang, > > > if we can hide this from the public API, to, e.g. add an additional > function > > in InternalTopologyBuilder of InternalStreamsBuilder (since in your > current > > working PR we're reusing InternalStreamsBuilder for the logical plan > > generation) which can then be called inside KafkaStreams constructors? > > I like the idea, but as I looked into it, there is an issue concerning the > fact that users can call Topology.describe() at any point. So with this > approach, we could end up where the first call to Topology.describe() > errors out or returns an invalid description, then the second call is > successful. So I don't think we'll be able to pursue this approach. > > > John, > > I initially liked your suggestion, but I also agree with Matthias as to why > we should not use that approach either. > > Thanks to all for the comments. > > Bill > > > On Mon, Jun 11, 2018 at 4:13 PM John Roesler <j...@confluent.io> wrote: > > > Thanks Matthias, > > > > I buy this reasoning. > > > > -John > > > > On Mon, Jun 11, 2018 at 12:48 PM, Matthias J. Sax <matth...@confluent.io > > > > wrote: > > > > > @John: I don't think this is a good idea. `KafkaStreams` executes a > > > `Topology` and should be agnostic if the topology was build manually or > > > via `StreamsBuilder` (at least from my point of view). > > > > > > -Matthias > > > > > > On 6/11/18 9:53 AM, Guozhang Wang wrote: > > > > Another implementation detail that we can consider: currently the > > > > InternalTopologyBuilder#setApplicationId() is used because we do not > > > have > > > > such a mechanism to pass in configs to the topology building process. > > > Once > > > > we add such mechanism we should consider removing this function as > > well. > > > > > > > > > > > > Guozhang > > > > > > > > On Mon, Jun 11, 2018 at 9:51 AM, Guozhang Wang <wangg...@gmail.com> > > > wrote: > > > > > > > >> Hello Bill, > > > >> > > > >> While working on https://github.com/apache/kafka/pull/5163 I am > > > wondering > > > >> if we can hide this from the public API, to e.g. add an additional > > > function > > > >> in InternalTopologyBuilder of InternalStreamsBuilder (since in your > > > current > > > >> working PR we're reusing InternalStreamsBuilder for the logical plan > > > >> generation) which can then be called inside KafkaStreams > constructors? > > > >> > > > >> > > > >> Guozhang > > > >> > > > >> > > > >> On Mon, Jun 11, 2018 at 9:41 AM, John Roesler <j...@confluent.io> > > > wrote: > > > >> > > > >>> Hi Bill, > > > >>> > > > >>> Thanks for the KIP. > > > >>> > > > >>> Just a small thought. This new API will result in calls that look > > like > > > >>> this: > > > >>> new KafkaStreams(builder.build(props), props); > > > >>> > > > >>> Do you think that's a significant enough eyesore to warrant adding > a > > > new > > > >>> KafkaStreams constructor taking a KStreamsBuilder like this: > > > >>> new KafkaStreams(builder, props); > > > >>> > > > >>> such that it would internally call builder.build(props) ? > > > >>> > > > >>> Thanks, > > > >>> -John > > > >>> > > > >>> > > > >>> > > > >>> On Fri, Jun 8, 2018 at 7:16 PM, Ted Yu <yuzhih...@gmail.com> > wrote: > > > >>> > > > >>>> Since there're only two values for the optional optimization > config > > > >>>> introduced by KAFKA-6935, I wonder the overloaded build method > (with > > > >>>> Properties > > > >>>> instance) would make the config unnecessary. > > > >>>> > > > >>>> nit: > > > >>>> * @return @return the {@link Topology} that represents the > specified > > > >>>> processing logic > > > >>>> > > > >>>> Double @return above. > > > >>>> > > > >>>> Cheers > > > >>>> > > > >>>> On Fri, Jun 8, 2018 at 3:20 PM, Bill Bejeck <b...@confluent.io> > > > wrote: > > > >>>> > > > >>>>> All, > > > >>>>> > > > >>>>> I'd like to start the discussion for adding an overloaded method > to > > > >>>>> StreamsBuilder taking a java.util.Properties instance. > > > >>>>> > > > >>>>> The KIP is located here : > > > >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > >>>>> 312%3A+Add+Overloaded+StreamsBuilder+Build+Method+ > > > >>>>> to+Accept+java.util.Properties > > > >>>>> > > > >>>>> I look forward to your comments. > > > >>>>> > > > >>>>> Thanks, > > > >>>>> Bill > > > >>>>> > > > >>>> > > > >>> > > > >> > > > >> > > > >> > > > >> -- > > > >> -- Guozhang > > > >> > > > > > > > > > > > > > > > > > > > > > -- -- Guozhang
