Hi Yishun, Thank you for continuing with this KIP. IMO, this KIP is very important to develop robust code.
I think, a good approach is to do some research on mock development on the internet and in the literatures and then try to prototype the mocks. These activities should yield you a list of pros and cons that you can add to the KIP. With this information it is simpler for everybody to discuss this KIP. Does this make sense to you? Best, Bruno On Mon, Apr 29, 2019 at 7:11 PM Yishun Guan <gyis...@gmail.com> wrote: > Hi, > > Sorry for the late reply, I have read through all your valuable > comments. The KIP still needs work at this point. > > I think at this point, one question comes up is that, how should we > implement the mock stores - as Sophie suggested, should we open to all > Store backend and just wrap around the Store class type which the user > will be providing - or, as Bruno suggested, we shouldn't have a > production backend store to be wrapped around in a mock store, just > keep track of the state of each method calls, even EasyMock could be > one of the option too. > > Personally, EasyMock will makes the implementation easier but building > from scratch provides extra functionality and provides expandability > (But I am not sure what kind of extra functionality we want in the > future). > > What do you guys think? > > Best, > Yishun > > On Fri, Apr 26, 2019 at 2:03 AM Matthias J. Sax <matth...@confluent.io> > wrote: > > > > What is the status of this KIP? > > > > > > Btw: there is also KIP-456. I was wondering if it might be required or > > helpful to align the design of both with each other. Thoughts? > > > > > > > > -Matthias > > > > > > On 4/11/19 12:17 AM, Matthias J. Sax wrote: > > > Thanks for the KIP. Only one initial comment (Sophie mentioned this > > > already but I want to emphasize on it). > > > > > > You state that > > > > > >> These will be internal classes, so no public API/interface. > > > > > > If this is the case, we don't need a KIP. However, the idea of the > > > original Jira is to actually make those classes public, as part of the > > > `streams-test-utils` package. If it's not public, developers should not > > > use them, because they don't have any backward compatibility > guarantees. > > > > > > Hence, I would suggest that the corresponding classes go into a new > > > package `org.apache.kafka.streams.state`. > > > > > > > > > -Matthias > > > > > > > > > On 4/9/19 8:58 PM, Bruno Cadonna wrote: > > >> Hi Yishun, > > >> > > >> Thank you for the KIP. > > >> > > >> I have a couple of comments: > > >> > > >> 1. Could you please add an example to the KIP that demonstrates how > the > > >> mocks should be used in a test? > > >> > > >> 2. I am wondering, whether the MockKeyValueStore needs to be backed > by an > > >> actual KeyValueStore (in your KIP InMemoryKeyValueStore). Would it not > > >> suffice to provide the mock with the entries that it has to check in > case > > >> of input operation like put() and with the entries it has to return > in case > > >> of an output operation like get()? In my opinion, a mock should have > as > > >> little and as simple code as possible. A unit test should depend as > little > > >> as possible from productive code that it does not explicitly test. > > >> > > >> 3. I would be interested in the arguments against using a > well-established > > >> and well-tested mock framework like EasyMock. If there are good > arguments, > > >> they should be listed under 'Rejected Alternatives'. > > >> > > >> 3. What is the purpose of the parameter 'time' in MockStoreFactory? > > >> > > >> Best, > > >> Bruno > > >> > > >> On Tue, Apr 9, 2019 at 11:29 AM Sophie Blee-Goldman < > sop...@confluent.io> > > >> wrote: > > >> > > >>> Hi Yishun, thanks for the KIP! I have a few initial > questions/comments: > > >>> > > >>> 1) It may be useful to capture the iterator results as well (eg with > a > > >>> MockIterator that wraps the underlying iterator and records the same > way > > >>> the MockStore wraps/records the underlying store) > > >>> > > >>> 2) a. Where is the "persistent" variable coming from or being used? > It > > >>> seems the MockKeyValueStore accepts it in the constructor, but only > the > > >>> name parameter is passed when constructing a new MockKeyValueStore in > > >>> build() ... also, if we extend InMemoryXXXStore shouldn't this > always be > > >>> false? > > >>> b. Is the idea to wrap an in-memory store for each type > (key-value, > > >>> session, etc)? We don't (yet) offer an in-memory version of the > session > > >>> store although it is in the works, so this will be possible -- I am > more > > >>> wondering if it makes sense to decide this for the user or to allow > them to > > >>> choose between in-memory or rocksDB by setting "persistent" > > >>> > > >>> 3) I'm wondering if users might want to be able to plug in their own > custom > > >>> stores as the underlying backend...should we support this as well? > WDYT? > > >>> > > >>> 4) We probably want to make these stores available through the public > > >>> test-utils package (maybe not the stores themselves which should be > > >>> internal, but should there be some kind of public API that gives > access to > > >>> them?) > > >>> > > >>> Cheers, > > >>> Sophie > > >>> > > >>> On Tue, Apr 9, 2019 at 9:19 AM Yishun Guan <gyis...@gmail.com> > wrote: > > >>> > > >>>> Bumping this up again, thanks! > > >>>> > > >>>> On Fri, Apr 5, 2019, 14:36 Yishun Guan <gyis...@gmail.com> wrote: > > >>>> > > >>>>> Hi, bumping this up again. Thanks! > > >>>>> > > >>>>> On Tue, Apr 2, 2019, 13:07 Yishun Guan <gyis...@gmail.com> wrote: > > >>>>> > > >>>>>> Hi All, > > >>>>>> > > >>>>>> I like to start a discussion on KIP-448 > > >>>>>> (https://cwiki.apache.org/confluence/x/SAeZBg). It is about > adding > > >>>>>> Mock state stores and relevant components for testing purposes. > > >>>>>> > > >>>>>> Here is the JIRA: > https://issues.apache.org/jira/browse/KAFKA-6460 > > >>>>>> > > >>>>>> This is a rough KIP draft, review and comment are appreciated. It > > >>>>>> seems to be tricky and some requirements and details are still > needed > > >>>>>> to be discussed. > > >>>>>> > > >>>>>> Thanks, > > >>>>>> Yishun > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > >> > > > > > >
