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<Integer> 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
<[email protected]> wrote:
Thanks, Navinder!
I took a look at the KIP.
We tend to use static factory methods instead of public constructors, and also
builders for optional parameters.
Given that, I think it would be more typical to have a factory method:
storeQueryParams()
and also builders for setting the optional parameters, like:
withPartitions(List<Integer> partitions)
withStaleStoresEnabled()
withStaleStoresDisabled()
I was also thinking this over today, and it really seems like there are two
main cases for specifying partitions,
1. you know exactly what partition you want. In this case, you'll only pass in
a single number.
2. you want to get a handle on all the stores for this instance (the current
behavior). In this case, it's not clear how to use withPartitions to achieve
the goal, unless you want to apply a-priori knowledge of the number of
partitions in the store. We could consider an empty list, or a null, to
indicate "all", but that seems a little complicated.
Thus, maybe it would actually be better to eschew withPartitions for now and
instead just offer:
withPartition(int partition)
withAllLocalPartitions()
and the getters:
Integer getPartition() // would be null if unset or if "all partitions"
boolean getAllLocalPartitions() // true/false if "all partitions" requested
Sorry, I know I'm stirring the pot, but what do you think about this?
Oh, also, the KIP is missing the method signature for the new
KafkaStreams#store overload.
Thanks!
-John
On Fri, Jan 17, 2020, at 08:07, Navinder Brar wrote:
> Hi all,
> I have created a new
> KIP:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-562%3A+Allow+fetching+a+key+from+a+single+partition+rather+than+iterating+over+all+the+stores+on+an+instance
> Please take a look if you get a chance.
> ~Navinder
