Hi John,
One question: Why must the field name be involved in the naming? It
somehow contradicts encapsulation. Field name `includeStaleStores` (or
`staleStoresEnabled`) sounds perfectly fine to me. IMO, we should
decouple the parameter name from the actual field name.
Bruno
On Thu, Jan 23, 2020 at 3:02 PM John Roesler <vvcep...@apache.org> wrote:
>
> Hi all,
>
> Thanks for the discussion!
>
> The basic idea I used in the original draft of the grammar was to avoid
> "fancy code" and just write "normal java". That's why I favored java bean
> spec over Kafka code traditions.
>
> According to the bean spec, setters always start with "set" and getters
> always start with "get" (or "is" for booleans). This often yields absurd
> or awkward readability. On the other hand, the "kafka" idioms
> seems to descend from Scala idiom of naming getters and setters
> exactly the same as the field they get and set. This plays to a language
> feature of Scala (getter referential transparency) that is not present
> in Java. My feeling is that we probably keep this idiom around
> precisely to avoid the absurd phrasing that the bean spec leads to.
>
> On the other hand, adopting the Kafka/Scala idiom brings in an
> additional burden I was trying to avoid: you have to pick a good
> name. Basically I was trying to avoid exactly this conversation ;)
> i.e., "X sounds weird, how about Y", "well, Y also sounds weird,
> what about Z", "Z sounds good, but then the setter sounds weird",
> etc.
>
> Maybe avoiding discussion was too ambitious, and I can't deny that
> bean spec names probably result in no one being happy, so I'm on
> board with the current proposal:
>
> setters:
> set{FieldName}(value)
> {enable/disable}{FieldName}()
>
> getters:
> {fieldName}()
> {fieldName}{Enabled/Disabled}()
>
> Probably, we'll find cases that are silly under that formula too,
> but we'll cross that bridge when we get to it.
>
> I'll update the grammar when I get the chance.
>
> Thanks!
> -John
>
> On Thu, Jan 23, 2020, at 12:37, Navinder Brar wrote:
> > Thanks Bruno, for the comments.
> > 1) Fixed.
> >
> > 2) I would be okay to call the variable staleStores. Since anyways we
> > are not using constructor, so the only way the variable is exposed
> > outside is the getter and the optional builder method. With this
> > variable name, we can name the builder method as "enableStaleStores"
> > and I feel staleStoresEnabled() is more readable for getter function.
> > So, we can also change the grammar for getters for boolean variables to
> > {FlagName}enabled / {FlagName}disabled. WDYT @John.
> >
> > Thanks,
> > Navinder On Thursday, 23 January, 2020, 11:43:38 pm IST, Bruno
> > Cadonna <br...@confluent.io> wrote:
> >
> > Hi Navinder,
> >
> > Thank you for the KIP!
> >
> > It looks good to me. Here my comments:
> >
> > 1) I agree with John and Matthias that you should remove the
> > implementation of the methods in the KIP. Just the method signatures
> > suffice and make the reading easier.
> >
> > 2) According to the grammar `withIncludeStaleStores()` should be
> > `enableIncludeStaleStores()` but since that reads a bit clumsy I
> > propose to call the method `enableStaleStores()`.
> >
> > 3) The getter `includeStaleStores()` does not sound right to me. It
> > does not include stale stores but rather checks if stale stores should
> > be queried. Thus, I would call it `staleStoresEnabled()` (or
> > `staleStoresIncluded` but that does not align nicely with
> > `enableStaleStores()`). No need to change the field name, though.
> > Maybe, we could also add this special rule for getters of boolean
> > values to the grammar. WDYT?
> >
> > I have a final remark about the `StoreQueryParams`. I think it should
> > be immutable. That is more an implementation detail and we should
> > discuss it on the PR. Just wanted to mention it in advance. Probably
> > we should add also a rule for immutability to the grammar.
> >
> > Best,
> > Bruno
> >
> > On Wed, Jan 22, 2020 at 7:38 PM Navinder Brar
> > <navinder_b...@yahoo.com.invalid> wrote:
> > >
> > > +1 on changing to storeName() and includeStateStores(). We can add this
> > > to grammar wiki as Matthias suggested.
> > >
> > > I have edited the KIP to remove "Deprecating" in favor of "Changing" and
> > > I agree we can deprecate store(final String storeName, final
> > > QueryableStoreType<T> queryableStoreType).
> > >
> > > Thanks
> > > Navinder
> > > On Thursday, 23 January, 2020, 07:28:38 am IST, Matthias J. Sax
> > > <matth...@confluent.io> wrote:
> > >
> > > 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
> > > >>>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > >
