Yes I was indeed thinking about now allowing `getStateStore()` to with the name specified in Materialized. I understand it is a bit too restrictive, but I cannot think of a elegant way to work around the following:
1) to programmatically enforce that the restore store is read-only. 2) With Materialized, the store name may not be specified by the user and hence it will be created internally; what would happen if users call `getStateStore()` with the correct internal store name? If not the semantics is a bit complex, if yes we are breaking the protocol to not expose internal store. Guozhang On Fri, May 11, 2018 at 10:42 AM, Andy Coates <a...@confluent.io> wrote: > Just a thought - but on the subject of disallowing access to the > materialized state store from within the transformer's init method... might > this not be overly restrictive? Could there be valid uses where read-only > access would be useful / valid. > > On 11 May 2018 at 10:35, Andy Coates <a...@confluent.io> wrote: > > > OK, KIP updated: > > - added overloads taking `Materialized` > > - dropped overloads taking `ValueTransformerSupplier` in favour of the > > `withKey` variants. > > - added more info around the limitations of the ProcessorContext passed > > in to the transformer's init calls, i.e. no forward calls allowed or > calls > > to getStateStore where the store name matches the materialized result of > > the call. > > > > I'll sort out the PR next. > > > > On 11 May 2018 at 10:26, Damian Guy <damian....@gmail.com> wrote: > > > >> I'm a +1 for Guozhang's suggestion > >> > >> On Fri, 11 May 2018 at 10:20 Andy Coates <a...@confluent.io> wrote: > >> > >> > Makes sense to me. What do others think? > >> > > >> > On 11 May 2018 at 10:13, Guozhang Wang <wangg...@gmail.com> wrote: > >> > > >> > > Hi folks, > >> > > > >> > > While looking into the overloaded functions, I'm wondering if we can > >> save > >> > > the transformers without key, i.e. only add two overloaded > functions: > >> > > > >> > > <VR> KTable<K, VR> transformValues(final > >> > ValueTransformerWithKeySupplier<? > >> > > super K, ? super V, ? extends VR> valueTransformerSupplier, > >> > > final String... stateStoreNames); > >> > > > >> > > <VR> KTable<K, VR> transformValues(final > >> > ValueTransformerWithKeySupplier<? > >> > > super K, ? super V, ? extends VR> valueTransformerSupplier, > >> > > final Materialized<K, VR, > >> > > KeyValueStore<Bytes, byte[]>> materialized, > >> > > final String... stateStoreNames); > >> > > > >> > > The reason is that, in KIP-149 we've added the overloaded functions > >> > > `withKey`, which should be covering the case without key already > >> because > >> > if > >> > > users do not really need the key, they can just take it as a dummy > >> > > parameter. We did not deprecate the old ones since some of them have > >> just > >> > > been added one version back. But if we agree that by the end of the > >> day > >> > we > >> > > would only maintain the overloaded value functions "with key" only, > >> then > >> > we > >> > > should not add the ones without keys any more in new KIPs. > >> > > > >> > > WDYT? > >> > > > >> > > > >> > > Guozhang > >> > > > >> > > > >> > > On Fri, May 11, 2018 at 9:42 AM, Andy Coates <a...@confluent.io> > >> wrote: > >> > > > >> > > > Sorry for my lack of response - I've been out of action with a bad > >> back > >> > > for > >> > > > a few days! > >> > > > > >> > > > I originally had the `Materialized` overloads added to the API. > I'll > >> > > update > >> > > > the KIP / PR with these again. In terms of semantics, as Matthias > >> > > suggests, > >> > > > these should be consistent with filter() and mapValues(), etc. > >> > > > > >> > > > On 8 May 2018 at 17:59, Guozhang Wang <wangg...@gmail.com> wrote: > >> > > > > >> > > > > To follow on Matthias and Damian's comments here: > >> > > > > > >> > > > > If we are going to add the overload functions as > >> > > > > > >> > > > > ``` > >> > > > > <VR> KTable<K, VR> transformValues(final > >> ValueTransformerSupplier<? > >> > > super > >> > > > > V, > >> > > > > ? extends VR> valueTransformerSupplier, > >> > > > > final String... > >> stateStoreNames, > >> > > > > final Materialized<K, > >> > > > > VR, KeyValueStore<Bytes, byte[]> materialized); > >> > > > > > >> > > > > <VR> KTable<K, VR> transformValues(final > >> > ValueTransformerWithKeySupplie > >> > > > r<? > >> > > > > super K, ? super V, ? extends VR> valueTransformerSupplier, > >> > > > > final String... > >> stateStoreNames, > >> > > > > final Materialized<K, > >> > > > > VR, KeyValueStore<Bytes, byte[]> materialized); > >> > > > > ``` > >> > > > > > >> > > > > Then are we going to still only allow the > valueTransofmer.init() / > >> > > > > process() to be able to access N stores, with N stores specified > >> with > >> > > the > >> > > > > stateStoreNames, but not the one specified in materialized.name > >> ()? > >> > > > > Personally I think it should be the case as the materialized > store > >> > > should > >> > > > > be managed by the Streams library itself, but we should probably > >> help > >> > > > users > >> > > > > to understand if they have some stores used for the same purpose > >> > > (storing > >> > > > > the value that are going to be sent to the downstream changelog > >> > stream > >> > > of > >> > > > > KTable), they should save that store and not creating by > >> themselves > >> > as > >> > > it > >> > > > > will be auto created by the Streams library. > >> > > > > > >> > > > > > >> > > > > Guozhang > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > On Tue, May 8, 2018 at 7:45 AM, Damian Guy < > damian....@gmail.com> > >> > > wrote: > >> > > > > > >> > > > > > Initially i thought materializing a store would be overkill, > but > >> > > from a > >> > > > > > consistency point of view it makes sense to add an overload > that > >> > > takes > >> > > > a > >> > > > > > `Materialized` and only create the store if that is supplied. > >> > > > > > > >> > > > > > On Sun, 6 May 2018 at 17:52 Matthias J. Sax < > >> matth...@confluent.io > >> > > > >> > > > > wrote: > >> > > > > > > >> > > > > > > Andy, > >> > > > > > > > >> > > > > > > thanks for the KIP. I don't have any further comments. > >> > > > > > > > >> > > > > > > My 2cents about Guozhang's questions: as I like consistent > >> > > behavior, > >> > > > I > >> > > > > > > think transfromValues() should behave the same way as > filter() > >> > and > >> > > > > > > mapValues(). > >> > > > > > > > >> > > > > > > > >> > > > > > > -Matthias > >> > > > > > > > >> > > > > > > On 5/2/18 2:24 PM, Guozhang Wang wrote: > >> > > > > > > > Hello Andy, > >> > > > > > > > > >> > > > > > > > Thanks for the KIP. The motivation and the general > proposal > >> > looks > >> > > > > good > >> > > > > > to > >> > > > > > > > me. I think in KTable it is indeed valuable to add the > >> > functions > >> > > > that > >> > > > > > > does > >> > > > > > > > not change key, such as mapValues, transformValues, and > >> filter. > >> > > > > > > > > >> > > > > > > > There are a few meta comments I have about the semantics > of > >> the > >> > > > newly > >> > > > > > > added > >> > > > > > > > functions: > >> > > > > > > > > >> > > > > > > > 1) For the resulted KTable, how should its > >> > "queryableStoreName()" > >> > > > be > >> > > > > > > > returning? > >> > > > > > > > > >> > > > > > > > 2) More specifically, how do we decide if the resulted > >> KTable > >> > is > >> > > to > >> > > > > be > >> > > > > > > > materialized or not? E.g. if there is no store names > >> provided > >> > > then > >> > > > it > >> > > > > > is > >> > > > > > > > likely that the resulted KTable is not materialized, or at > >> > least > >> > > > not > >> > > > > > > > logically materialized and not be queryable. What if there > >> is > >> > at > >> > > > > least > >> > > > > > > one > >> > > > > > > > state store provided? Will any of them be provided as the > >> > > > > materialized > >> > > > > > > > store, or should we still add a Materialized parameter for > >> this > >> > > > > > purpose? > >> > > > > > > > > >> > > > > > > > 3) For its internal implementations, how should the > >> key/value > >> > > > serde, > >> > > > > > > > sendOldValues flag etc be inherited from its parent > >> processor > >> > > node? > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > Guozhang > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > On Wed, May 2, 2018 at 12:43 PM, Andy Coates < > >> > a...@confluent.io> > >> > > > > > wrote: > >> > > > > > > > > >> > > > > > > >> Hi everyone, > >> > > > > > > >> > >> > > > > > > >> I would like to start a discussion for KIP 292. I would > >> > > appreciate > >> > > > > it > >> > > > > > if > >> > > > > > > >> you could review and provide feedback. > >> > > > > > > >> > >> > > > > > > >> KIP: KIP-292: Add transformValues() method to KTable > >> > > > > > > >> <https://cwiki.apache.org/confluence/display/KAFKA/KIP- > >> > > > > > > >> 292%3A+Add+transformValues%28%29+method+to+KTable> > >> > > > > > > >> Jira: KAFKA-6849 <https://issues.apache.org/ > >> > > > jira/browse/KAFKA-6849> > >> > > > > > > >> > >> > > > > > > >> PR: #4959 <https://github.com/apache/kafka/pull/4959> > >> > > > > > > >> > >> > > > > > > >> > >> > > > > > > >> > >> > > > > > > >> Thanks, > >> > > > > > > >> > >> > > > > > > >> Andy > >> > > > > > > >> > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > -- > >> > > > > -- Guozhang > >> > > > > > >> > > > > >> > > > >> > > > >> > > > >> > > -- > >> > > -- Guozhang > >> > > > >> > > >> > > > > > -- -- Guozhang
