I went through the grammar wiki page and since it is already agreed in
principle I will change from constructor to below method and add the getters
back.
public static <T> StoreQueryParams<T> fromNameAndType(
final String storeName,
final QueryableStoreType<T> queryableStoreType
)
Thanks,
Navinder
On Wednesday, 22 January, 2020, 09:32:07 pm IST, John Roesler
<vvcep...@apache.org> wrote:
22) I'm specifically proposing to establish a new convention.
The existing convention is fundamentally broken and has
been costly both for users and maintainers. That is the purpose
of the grammar I proposed. The plan is to implement new APIs
following the grammar and gradually to port old APIs to it.
The grammar wiki page has plenty of justification, so I won't
recapitulate it here.
Thanks,
-John
On Wed, Jan 22, 2020, at 09:39, Navinder Brar wrote:
> 10) Sure John, please go ahead.
>
> 21) I have no strong opinion on constructor vs static factory. If
> everyone's okay with it, I can make the change.
>
> 22) I looked at classes suggested by Matthias and I see there are no
> getters there. We are ok with breaking the convention?
>
> Thanks,Navinder Pal Singh Brar
>
>
>
> On Wednesday, 22 January, 2020, 08:40:27 pm IST, John Roesler
> <vvcep...@apache.org> wrote:
>
> Hi all,
>
> 10) For the motivation, I have some thoughts for why this KIP is
> absolutely essential as designed. If it's ok with you, Navinder,
> I'd just edit the motivation section of the wiki? If you're unhappy
> with my wording, you're of course welcome to revert or revise it;
> it just seems more efficient than discussing it over email.
>
> 20) The getters were my fault :)
> I proposed to design this KIP following the grammar proposal:
> https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Streams+DSL+Grammar
> At the risk of delaying the vote on this KIP, I'd humbly suggest we
> keep the getters,
> for all the reasons laid out on that grammar.
>
> I realize this introduces an inconsistency, but my hope is that we
> would close that
> gap soon. I can even create tickets for migrating each API, if that
> helps make
> this idea more palatable. IMO, this proposed API is likely to be a bit
> "out of
> the way", in that it's not likely to be heavily used by a broad
> audience in 2.5,
> so the API inconsistency wouldn't be too apparent. Plus, it will save
> us from
> implementing a config object in the current style, along with an
> "internal"
> counterpart, which we would immediately have plans to deprecate.
>
> Just to clarify (I know this has been a bit thrashy):
> 21. there should be no public constructor, instead (since there are
> required arguments),
> there should be just one factory method:
> public static <T> StoreQueryParams<T> fromNameAndType(
> final String storeName,
> final QueryableStoreType<T> queryableStoreType
> )
>
> 22. there should be getters corresponding to each argument (required
> and optional):
> Integer getPartition()
> boolean getIncludeStaleStores()
>
> Instead of adding the extra getAllPartitions() pseudo-getter, let's
> follow Ted's advice and
> just document that getPartition() would return `null`, and that it
> means that a
> specific partition hasn't been requested, so the store would wrap all
> local partitions.
>
> With those two changes, this proposal would be 100% in line with the grammar,
> and IMO ready to go.
>
> Thanks,
> -John
>
> Thanks,
> -John
>
> On Wed, Jan 22, 2020, at 03:56, Navinder Brar wrote:
> > Thanks Matthias for the feedback.
> >
> > 10) As Guozhang suggested above, we thought of adding storeName and
> > queryableStoreType as well in the StoreQueryParams, which is another
> > motivation for this KIP as it overloads KafkaStreams#store(). I have
> > updated the motivation in the KIP as well.
> >
> > 20) I agree we can probably remove getPartition() and
> > getIncludeStaleStores() but we would definitely need getStoreName and
> > getQueryableStoreType() as they would be used in internal classes
> > QueryableStoreProvider.java and StreamThreadStateStoreProvider.java.
> >
> > 30) I have edited the KIP to include only the changed KafkaStreams#store().
> >
> > 40) Removed the internal classes from the KIP.
> >
> > I have incorporated feedback from Guozhang as well in the KIP. If
> > nothing else is pending, vote is ongoing.
> >
> > ~Navinder On Wednesday, 22 January, 2020, 12:49:51 pm IST, Matthias
> > J. Sax <matth...@confluent.io> wrote:
> >
> > Thanks for the KIP. Overall it makes sense.
> >
> > Couple of minor comments/questions:
> >
> > 10) To me, it was initially quite unclear why we need this KIP. The
> > motivation section does only talk about some performance issues (that
> > are motivated by single key look-ups) -- however, all issues mentioned
> > in the KIP could be fixed without any public API change. The important
> > cases, why the public API changes (and thus this KIP) is useful are
> > actually missing in the motivation section. I would be helpful to add
> > more details.
> >
> > 20) `StoreQueryParams` has a lot of getter methods that we usually don't
> > have for config objects (compare `Consumed`, `Produced`, `Materialized`,
> > etc). Is there any reason why we need to add those getters to the public
> > API?
> >
> > 30) The change to remove `KafkaStreams#store(...)` as introduced in
> > KIP-535 should be listed in sections Public API changes. Also, existing
> > methods should not be listed -- only changes. Hence, in
> > `KafkaStreams.java` only one new method and the `store()` method as
> > added via KIP-535 should be listed.
> >
> > 40) `QueryableStoreProvider` and `StreamThreadStateStoreProvider` are
> > internal classes and thus we can remove all changes to it from the KIP.
> >
> >
> > Thanks!
> >
> >
> > -Matthias
> >
> >
> >
> > On 1/21/20 11:46 AM, Vinoth Chandar wrote:
> > > Chiming in a bit late here..
> > >
> > > +1 This is a very valid improvement. Avoiding doing gets on irrelevant
> > > partitions will improve performance and efficiency for IQs.
> > >
> > > As an incremental improvement to the current APIs, adding an option to
> > > filter out based on partitions makes sense
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > On Mon, Jan 20, 2020 at 3:13 AM Navinder Brar
> > > <navinder_b...@yahoo.com.invalid> wrote:
> > >
> > >> Thanks John. If there are no other comments to be addressed, I will start
> > >> a vote today so that we are on track for this release.~Navinder
> > >>
> > >>
> > >> On Monday, January 20, 2020, 8:32 AM, John Roesler <vvcep...@apache.org>
> > >> wrote:
> > >>
> > >> Thanks, Navinder,
> > >>
> > >> The Param object looks a bit different than I would have done, but it
> > >> certainly is explicit. We might have to deprecate those particular
> > >> factory
> > >> methods and move to a builder pattern if we need to add any more options
> > >> in
> > >> the future, but I’m fine with that possibility.
> > >>
> > >> The KIP also discusses some implementation details that aren’t necessary
> > >> here. We really only need to see the public interfaces. We can discuss
> > >> the
> > >> implementation in the PR.
> > >>
> > >> That said, the public API part of the current proposal looks good to me!
> > >> I
> > >> would be a +1 if you called for a vote.
> > >>
> > >> Thanks,
> > >> John
> > >>
> > >> On Sun, Jan 19, 2020, at 20:50, Navinder Brar wrote:
> > >>> I have made some edits in the KIP, please take another look. It would
> > >>> be great if we can push it in 2.5.0.
> > >>> ~Navinder
> > >>>
> > >>>
> > >>> On Sunday, January 19, 2020, 12:59 AM, Navinder Brar
> > >>> <navinder_b...@yahoo.com.INVALID> wrote:
> > >>>
> > >>> Sure John, I will update the StoreQueryParams with static factory
> > >>> methods.
> > >>> @Ted, we would need to create taskId only in case a user provides one
> > >>> single partition. In case user wants to query all partitions of an
> > >>> instance the current code is good enough where we iterate over all
> > >>> stream threads and go over all taskIds to match the store. But in case
> > >>> a user requests for a single partition-based store, we need to create a
> > >>> taskId out of that partition and store name(using
> > >>> internalTopologyBuilder class) and match with the taskIds belonging to
> > >>> that instance. I will add the code in the KIP.
> > >>>
> > >>> On Sunday, 19 January, 2020, 12:47:08 am IST, Ted Yu
> > >>> <yuzhih...@gmail.com> wrote:
> > >>>
> > >>> Looking at the current KIP-562:
> > >>>
> > >>> bq. Create a taskId from the combination of store name and partition
> > >>> provided by the user
> > >>>
> > >>> I wonder if a single taskId would be used for the “all partitions” case.
> > >>> If so, we need to choose a numerical value for the partition portion of
> > >> the
> > >>> taskId.
> > >>>
> > >>> On Sat, Jan 18, 2020 at 10:27 AM John Roesler <vvcep...@apache.org>
> > >> wrote:
> > >>>
> > >>>> Thanks, Ted!
> > >>>>
> > >>>> This makes sense, but it seems like we should lean towards explicit
> > >>>> semantics in the public API. ‘-1’ meaning “all partitions” is
> > >> reasonable,
> > >>>> but not explicit. That’s why I suggested the Boolean for “all
> > >> partitions”.
> > >>>> I guess this also means getPartition() should either throw an
> > >> exception or
> > >>>> return null if the partition is unspecified.
> > >>>>
> > >>>> Thanks,
> > >>>> John
> > >>>>
> > >>>> On Sat, Jan 18, 2020, at 08:43, Ted Yu wrote:
> > >>>>> I wonder if the following two methods can be combined:
> > >>>>>
> > >>>>> Integer getPartition() // would be null if unset or if "all
> > >> partitions"
> > >>>>> boolean getAllLocalPartitions() // true/false if "all partitions"
> > >>>> requested
> > >>>>>
> > >>>>> into:
> > >>>>>
> > >>>>> Integer getPartition() // would be null if unset or -1 if "all
> > >>>> partitions"
> > >>>>>
> > >>>>> Cheers
> > >>>>>
> > >>>>> On Fri, Jan 17, 2020 at 9:56 PM John Roesler <vvcep...@apache.org>
> > >>>> wrote:
> > >>>>>
> > >>>>>> Thanks, Navinder!
> > >>>>>>
> > >>>>>> I took a look at the KIP.
> > >>>>>>
> > >>>>>> We tend to use static factory methods instead of public
> > >> constructors,
> > >>>> and
> > >>>>>> also builders for optional parameters.
> > >>>>>>
> > >>>>>> Given that, I think it would be more typical to have a factory
> > >> method:
> > >>>>>> storeQueryParams()
> > >>>>>>
> > >>>>>> and also builders for setting the optional parameters, like:
> > >>>>>> withPartitions(List<Integer> partitions)
> > >>>>>> withStaleStoresEnabled()
> > >>>>>> withStaleStoresDisabled()
> > >>>>>>
> > >>>>>>
> > >>>>>> I was also thinking this over today, and it really seems like
> > >> there are
> > >>>>>> two main cases for specifying partitions,
> > >>>>>> 1. you know exactly what partition you want. In this case, you'll
> > >> only
> > >>>>>> pass in a single number.
> > >>>>>> 2. you want to get a handle on all the stores for this instance
> > >> (the
> > >>>>>> current behavior). In this case, it's not clear how to use
> > >>>> withPartitions
> > >>>>>> to achieve the goal, unless you want to apply a-priori knowledge
> > >> of the
> > >>>>>> number of partitions in the store. We could consider an empty
> > >> list, or
> > >>>> a
> > >>>>>> null, to indicate "all", but that seems a little complicated.
> > >>>>>>
> > >>>>>> Thus, maybe it would actually be better to eschew withPartitions
> > >> for
> > >>>> now
> > >>>>>> and instead just offer:
> > >>>>>> withPartition(int partition)
> > >>>>>> withAllLocalPartitions()
> > >>>>>>
> > >>>>>> and the getters:
> > >>>>>> Integer getPartition() // would be null if unset or if "all
> > >> partitions"
> > >>>>>> boolean getAllLocalPartitions() // true/false if "all partitions"
> > >>>> requested
> > >>>>>>
> > >>>>>> Sorry, I know I'm stirring the pot, but what do you think about
> > >> this?
> > >>>>>>
> > >>>>>> Oh, also, the KIP is missing the method signature for the new
> > >>>>>> KafkaStreams#store overload.
> > >>>>>>
> > >>>>>> Thanks!
> > >>>>>> -John
> > >>>>>>
> > >>>>>> On Fri, Jan 17, 2020, at 08:07, Navinder Brar wrote:
> > >>>>>>> Hi all,
> > >>>>>>> I have created a new
> > >>>>>>> KIP:
> > >>>>>>
> > >>>>
> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-562%3A+Allow+fetching+a+key+from+a+single+partition+rather+than+iterating+over+all+the+stores+on+an+instance
> > >>>>>>> Please take a look if you get a chance.
> > >>>>>>> ~Navinder
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> > >>
> > >
> >
