Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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
> 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
> > 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
> > 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 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}
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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
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
> 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
> 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 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.
> > > >
> >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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
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 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 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. WD
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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 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
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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()`
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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
> > 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 queryableStoreType).
> > >
> > > Thanks
> > > Navinder
> > >On Thursday, 23 January, 2020, 07:28:38 am IST, Matthias J. Sax
> > > 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` ->
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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
> 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 queryableStoreType).
> >
> > Thanks
> > Navinder
> > On Thursday, 23 January, 2020, 07:28:38 am IST, Matthias J. Sax
> > 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 queryableStoreType, final boolean
> > > includeStaleStor
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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
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 queryableStoreType).
>
> Thanks
> Navinder
> On Thursday, 23 January, 2020, 07:28:38 am IST, Matthias J. Sax
> 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 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
> > 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 StoreQueryParams fromNameAndType(
> >> final String storeName,
> >> final QueryableStoreType queryableStoreType
> >> )
> >>
> >>
> >> Thanks,
> >> Navinder
> >>
> >>On Wednesday, 22 January, 2020, 09:32:07 pm IST, John Roesler
> >> 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
> >>> 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 re
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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
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
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 queryableStoreType).
>
> Thanks
> Navinder
> On Thursday, 23 January, 2020, 07:28:38 am IST, Matthias J. Sax
> 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 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
> > 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 StoreQueryParams fromNameAndType(
> >> final String storeName,
> >> final QueryableStoreType queryableStoreType
> >> )
> >>
> >>
> >> Thanks,
> >> Navinder
> >>
> >> On Wednesday, 22 January, 2020, 09:32:07 pm IST, John Roesler
> >> 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 th
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
+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 queryableStoreType).
Thanks
Navinder
On Thursday, 23 January, 2020, 07:28:38 am IST, Matthias J. Sax
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 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 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 StoreQueryParams fromNameAndType(
>> final String storeName,
>> final QueryableStoreType queryableStoreType
>> )
>>
>>
>> Thanks,
>> Navinder
>>
>> On Wednesday, 22 January, 2020, 09:32:07 pm IST, John Roesler
>> 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
>>> 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 StoreQueryParams fromNameAndType(
>>> final String storeName,
>>> final QueryableStoreType queryableStoreType
>>> )
>
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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 StoreQueryParams fromNameAndType(
>> final String storeName,
>> final QueryableStoreType queryableStoreType
>> )
>>
>>
>> Thanks,
>> Navinder
>>
>> On Wednesday, 22 January, 2020, 09:32:07 pm IST, John Roesler
>> 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
>>> 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 StoreQueryParams fromNameAndType(
>>> final String storeName,
>>> final QueryableStoreType 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
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 StoreQueryParams fromNameAndType( > final String storeName, > final QueryableStoreType queryableStoreType > ) > > > Thanks, > Navinder > > On Wednesday, 22 January, 2020, 09:32:07 pm IST, John Roesler > 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 > > 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 StoreQueryParams fromNameAndType( > > final String storeName, > > final QueryableStoreType 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 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
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 StoreQueryParams fromNameAndType( final String storeName, final QueryableStoreType queryableStoreType ) Thanks, Navinder On Wednesday, 22 January, 2020, 09:32:07 pm IST, John Roesler 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 > 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 StoreQueryParams fromNameAndType( > final String storeName, > final QueryableStoreType 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 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. > >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 > 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 StoreQueryParams fromNameAndType( > final String storeName, > final QueryableStoreType 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 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.
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 StoreQueryParams fromNameAndType( final String storeName, final QueryableStoreType 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 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 base
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 StoreQueryParams fromNameAndType( final String storeName, final QueryableStoreType 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. > > ~NavinderOn Wednesday, 22 January, 2020, 12:49:51 pm IST, Matthias > J. Sax 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 > > 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 > >> wrote: > >> > >> Thanks, Navinder, > >>
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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. ~NavinderOn Wednesday, 22 January, 2020, 12:49:51 pm IST, Matthias J. Sax 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 > 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 >> 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 >>> 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 >>> 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 >> 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
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 > 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 >> 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 >>> 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 >>> 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 >> 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 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 thi
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 > 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 > > 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 > > 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 > 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 > > > 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 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() /
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 > 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 > 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 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 > > 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 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/KI
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 > 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 > 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 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 > > 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 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 > > > > > > > > >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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 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 > 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 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 > > > > > >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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 > 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 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 > > > > > >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 > 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 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 > > > > > >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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 > > >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
Hi Navinder, Thanks for the explanation. Your thinking makes sense, but those classes are actually only constructed internally by Streams, never by users of the API. I believe all the public config objects are constructed with factory methods. What you say for the partition selection sounds perfect! Thanks, John On Sat, Jan 18, 2020, at 01:52, Navinder Brar wrote: > Hi John, > Thanks for looking into it. > On using constructors rather than static factory methods I was coming > from the convention on the classes currently available to users such as > LagInfo and KeyQueryMetadata. Let me know if it's still favorable to > change StoreQueryParams into static factory method, I will update the > KIP. > On List partitions vs Integer partition, I agree sending empty > list to return all is cumbersome so will change the class to have a > single partition in case users want to fetch store for a partition and > if it's unset we will return all partitions available. > On KafkaStreams#store overload - noted. I will update the KIP. > Thanks,Navinder > > On Saturday, 18 January, 2020, 11:26:30 am IST, John Roesler > 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 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
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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 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 >
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
Hi John, Thanks for looking into it. On using constructors rather than static factory methods I was coming from the convention on the classes currently available to users such as LagInfo and KeyQueryMetadata. Let me know if it's still favorable to change StoreQueryParams into static factory method, I will update the KIP. On List partitions vs Integer partition, I agree sending empty list to return all is cumbersome so will change the class to have a single partition in case users want to fetch store for a partition and if it's unset we will return all partitions available. On KafkaStreams#store overload - noted. I will update the KIP. Thanks,Navinder On Saturday, 18 January, 2020, 11:26:30 am IST, John Roesler 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 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
Re: [DISCUSS] : KIP-562: Allow fetching a key from a single partition rather than iterating over all the stores on an instance
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 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
