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 >>>>> >>>> >>> >> > >
signature.asc
Description: OpenPGP digital signature
