Thanks Navinder! I've also updated the motivation. Thanks, -John
On Wed, Jan 22, 2020, at 11:12, Navinder Brar wrote: > 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 > > > >>>>>> > > > >>>>> > > > >>>> > > > >>> > > > >>> > > > >>> > > > >> > > > >> > > > >> > > > > > > >
