Thanks for the KIP Bruno, +1 (binding) Sophie
On Mon, Jan 25, 2021 at 11:23 AM Guozhang Wang <wangg...@gmail.com> wrote: > Hey Bruno, > > Thanks for your response! > > 1) Yup I'm good with option a) as well. > 2) Thanks! > 3) Sounds good to me. I think it would not change any StreamThread > implementation regarding capturing exceptions from consumer.poll() since it > captures StreamsException as fatal. > > > Guozhang > > On Wed, Dec 16, 2020 at 4:43 AM Bruno Cadonna <br...@confluent.io> wrote: > > > Hi Guozhang, > > > > Thank for the feedback! > > > > Please find my answers inline. > > > > Best, > > Bruno > > > > > > On 14.12.20 23:33, Guozhang Wang wrote: > > > Hello Bruno, > > > > > > Just a few more questions about the KIP: > > > > > > 1) If the internal topics exist but the calculated num.partitions do > not > > > match the existing topics, what would Streams do; > > > > Good point! I missed to explicitly consider misconfigurations in the KIP. > > > > I propose to throw a fatal error in this case during manual and > > automatic initialization. For the fatal error, we have two options: > > a) introduce a second exception besides MissingInternalTopicException, > > e.g. MisconfiguredInternalTopicException > > b) rename MissingInternalTopicException to > > MissingOrMisconfiguredInternalTopicException and throw that in both > cases. > > > > Since the process to react on such an exception user-side should be > > similar, I am fine with option b). However, IMO option a) is a bit > > cleaner. WDYT? > > > > > 2) Since `init()` is a blocking call (we only return after all topics > are > > > confirmed to be created), should we have a timeout for this call as > well > > or > > > not; > > > > I will add an overload with a timeout to the KIP. > > > > > 3) If the configure is set to `MANUAL_SETUP`, then during rebalance do > we > > > still check if number of partitions of the existing topic match or not; > > if > > > not, do we throw the newly added exception or throw a fatal > > > StreamsException? Today we would throw the StreamsException from > assign() > > > which would be then thrown from consumer.poll() as a fatal error. > > > > > > > Yes, I think we should check if the number of partitions match. I > > propose to throw the newly added exception in the same way as we throw > > now the MissingSourceTopicException, i.e., throw it from > > consumer.poll(). WDYT? > > > > > Guozhang > > > > > > > > > On Mon, Dec 14, 2020 at 12:47 PM John Roesler <vvcep...@apache.org> > > wrote: > > > > > >> Thanks, Bruno! > > >> > > >> I'm +1 (binding) > > >> > > >> -John > > >> > > >> On Mon, 2020-12-14 at 09:57 -0600, Leah Thomas wrote: > > >>> Thanks for the KIP Bruno, LGTM. +1 (non-binding) > > >>> > > >>> Cheers, > > >>> Leah > > >>> > > >>> On Mon, Dec 14, 2020 at 4:29 AM Bruno Cadonna <br...@confluent.io> > > >> wrote: > > >>> > > >>>> Hi, > > >>>> > > >>>> I'd like to start the voting on KIP-698 that proposes an explicit > user > > >>>> initialization of broker-side state for Kafka Streams instead of > > >> letting > > >>>> Kafka Streams setting up the broker-side state automatically during > > >>>> rebalance. Such an explicit initialization avoids possible data loss > > >>>> issues due to automatic initialization. > > >>>> > > >>>> https://cwiki.apache.org/confluence/x/7CnZCQ > > >>>> > > >>>> Best, > > >>>> Bruno > > >>>> > > >> > > >> > > >> > > > > > > > > -- > -- Guozhang >
