All, the discussion list for this proposed change has been quiet for a few days.
If there are no changes or other proposals, I'll start a voting thread later today. Thanks, Bill On Wed, Jun 13, 2018 at 12:31 PM Guozhang Wang <wangg...@gmail.com> wrote: > 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 >
