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