Thanks for the clarifications about the getters. I agree that it makes sense to move to the new pattern incrementally. Might be useful to create a Jira (or multiple?) to track this. It's an straight forward change.
A nit about the KIP: it should only list the signature but not the full
code of the implementation (ie, only package name and the class + method
names; we can omit toString(), equals(), and hashCode(), too -- alo, no
license header please ;))
nit: `isIncludeStaleStores` -> `includeStaleStores` (the "is"-prefix
reads clumsy and it's common in Kafka code base to omit the "get"-prefix
for getters -- we should adopt this)
@John: might be worth to include this in the Grammar wiki page?
nit (similar as above):
- `getStoreName` -> `storeName`
- `getQueryableStoreType` -> `queryableStoreType`
The KIP says
> Deprecating the KafkaStreams#store(final String storeName, final
> QueryableStoreType<T> queryableStoreType, final boolean includeStaleStores)
> in favour of the funtion mentioned below.
We don't need to deprecate this method but we can remove it directly,
because it was never release.
What is the plan for
> store(final String storeName, final QueryableStoreType<T> queryableStoreType)
> {
Given that the new `StoreQueryParams` allows to specify `storeName` and
`queryableStoreType`, should we deprecate this method in favor of the
new `store(StoreQueryParams)` overload?
-Matthias
On 1/22/20 10:06 AM, John Roesler wrote:
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
signature.asc
Description: OpenPGP digital signature
