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 <vvcep...@apache.org> 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 <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 > > > > > >
