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 > > >>>>>> > > >>>>> > > >>>> > > >>> > > >>> > > >>> > > >> > > >> > > >> > > > > >
