Thanks, Bruno!
I meant to do it, but got side-tracked.
-John
On Tue, Feb 11, 2020, at 03:55, Bruno Cadonna wrote:
> Hi all,
>
> I am fine with StoreQueryParameters, but then we should also change
> the DSL grammar accordingly. Since the PR was merged, I suppose
> everybody agrees on the new name and I changed the DSL grammar.
>
> Best,
> Bruno
>
> On Thu, Feb 6, 2020 at 1:07 PM Navinder Brar
> <navinder_b...@yahoo.com.invalid> wrote:
> >
> > Hi,
> >
> > While implementing 562, we have decided to rename StoreQueryParams ->
> > StoreQueryParameters. I have updated the PR and confluence. Please share if
> > anyone has feedback on it.
> >
> > Thanks & Regards,
> > Navinder Pal Singh Brar
> >
> > On Friday, 24 January, 2020, 08:45:15 am IST, Navinder Brar
> > <navinder_b...@yahoo.com.invalid> wrote:
> >
> > Hi John,
> >
> > Thanks for the responses. I will make the below changes as I had suggested
> > earlier, and then close the vote in a few hours.
> >
> > includeStaleStores -> staleStores
> > withIncludeStaleStores() > enableStaleStores()
> > includeStaleStores() -> staleStoresEnabled()
> >
> > Thanks,
> > Navinder
> >
> > Sent from Yahoo Mail for iPhone
> >
> >
> > On Friday, January 24, 2020, 5:36 AM, John Roesler <vvcep...@apache.org>
> > wrote:
> >
> > Hi Bruno,
> >
> > Thanks for your question; it's a very reasonable response to
> > what I said before.
> >
> > I didn't mean "field" as in an instance variable, just as in a specific
> > property or attribute. It's hard to talk about because all the words
> > for this abstract concept are also words that people commonly use
> > for instance variables.
> >
> > The principle is that these classes should be simple data containers.
> > It's not so much that the methods match the field name, or that the
> > field name matches the methods, but that all three bear a simple
> > and direct relationship to each other. Or maybe I should say that
> > the getters, setters, and fields are all directly named after a property.
> >
> > The point is that we should make sure we're not "playing games" in
> > these classes but simply setting a property and offering a transparent
> > way to get exactly what you just set.
> >
> > I actually do think that the instance variable itself should have the
> > same name as the "field" or "property" that the getters and setters
> > are named for. This is not a violation of encapsulation because those
> > instance variables are required to be private.
> >
> > I guess you can think of this rule as more of a style guide than a
> > grammar, but whatever. As a maintainer, I think we should discourage
> > these particular classes to have different instance variables than
> > method names. Otherwise, it's just silly. Either "includeStaleStores"
> > or "staleStoresEnabled" is a fine name, but not both. There's no
> > reason at all to name all the accessors one of them and the instance
> > variable they access the other name.
> >
> > Thanks,
> > -John
> >
> >
> > On Thu, Jan 23, 2020, at 17:27, Bruno Cadonna wrote:
> > > 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
> > > > > > >>>>>>>>>>
> > > > > > >>>>>>>>>
> > > > > > >>>>>>>>
> > > > > > >>>>>>>
> > > > > > >>>>>>>
> > > > > > >>>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>
> > > > > >
> > >
> >
> >
>
