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