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