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
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
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
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
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,
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
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
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
+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
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
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
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
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
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
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
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
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
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
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
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
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
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
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,
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
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
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
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
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
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
30 matches
Mail list logo
