Hi, I just had one more thought about an additional improvement we might want to include in this KIP.
Kafka Streams ships with a `WallclockTimestampExtractor` that just returns `System.currentTimeMillis()` and thus, cannot be mocked. And it seems that there is no way for a user to build a custom timestamps extractor that returns the TTD's mocked system time. Thus, it might be nice, to add a `MockTimeExtractor` (only in the test-util package) that users could set and this `MockTimeExtractor` returns the mocked system time. Thoughts? -Matthias On 7/7/20 11:11 PM, Matthias J. Sax wrote: > I think, we don't need a default implementation for the new methods. > > What would be the use-case to implement the `ProcessorContext` > interface? In contract to, for example, `KeyValueStore`, > `ProcessorContext` is a use-only interface because it's never passed > into Kafka Streams, but only handed out to the user. > > > -Matthias > > > On 7/7/20 1:28 PM, William Bottrell wrote: >> Sure, I would appreciate help from Piotr creating an example. >> >> On Tue, Jul 7, 2020 at 12:03 PM Boyang Chen <reluctanthero...@gmail.com> >> wrote: >> >>> Hey John, >>> >>> since ProcessorContext is a public API, I couldn't be sure that people >>> won't try to extend it. Without a default implementation, user code >>> compilation will break. >>> >>> William and Piotr, it seems that we haven't added any example usage of the >>> new API, could we try to address that? It should help with the motivation >>> and follow-up meta comments as John proposed. >>> >>> Boyang >>> >>> On Mon, Jul 6, 2020 at 12:04 PM Matthias J. Sax <mj...@apache.org> wrote: >>> >>>> William, >>>> >>>> thanks for the KIP. LGMT. Feel free to start a vote. >>>> >>>> >>>> -Matthias >>>> >>>> >>>> On 7/4/20 10:14 AM, John Roesler wrote: >>>>> Hi Richard, >>>>> >>>>> It’s good to hear from you! >>>>> >>>>> Thanks for bringing up the wall-clock suppression feature. IIRC, >>> someone >>>> actually started a KIP discussion for it already, but I don’t think it >>> went >>>> to a vote. I don’t recall any technical impediment, just the lack of >>>> availability to finish it up. Although there is some association, it >>> would >>>> be good to keep the KIPs separate. >>>>> >>>>> Thanks, >>>>> John >>>>> >>>>> On Sat, Jul 4, 2020, at 10:05, Richard Yu wrote: >>>>>> Hi all, >>>>>> >>>>>> This reminds me of a previous issue I think that we were discussing. >>>>>> @John Roesler <mailto:vvcep...@apache.org> I think you should >>> remember >>>> this one. >>>>>> >>>>>> A while back, we were talking about having suppress operator emit >>>>>> records by wall-clock time instead of stream time. >>>>>> If we are adding this, wouldn't that make it more feasible for us to >>>>>> implement that feature for suppression? >>>>>> >>>>>> If I recall correctly, there actually had been quite a bit of user >>>>>> demand for such a feature. >>>>>> Might be good to include it in this KIP (or maybe use one of the prior >>>>>> KIPs for this feature). >>>>>> >>>>>> Best, >>>>>> Richard >>>>>> >>>>>> On Sat, Jul 4, 2020 at 6:58 AM John Roesler <vvcep...@apache.org> >>>> wrote: >>>>>>> Hi all, >>>>>>> >>>>>>> 1. Thanks, Boyang, it is nice to see usage examples in KIPs like >>>> this. It helps during the discussion, and it’s also good documentation >>>> later on. >>>>>>> >>>>>>> 2. Yeah, this is a subtle point. The motivation mentions being able >>>> to control the time during tests, but to be able to make it work, the >>>> processor implementation needs a public method on ProcessorContext to get >>>> the time. Otherwise, processors would have to check the type of the >>> context >>>> and cast, depending on whether they’re running inside a test or not. In >>>> retrospect, if we’d had a usage example, this probably would have been >>>> clear. >>>>>>> >>>>>>> 3. I don’t think we expect people to have their own implementations >>>> of ProcessorContext. Since all implementations are internal, it’s really >>> an >>>> implementation detail whether we use a default method, abstract methods, >>> or >>>> concrete methods. I can’t think of an implementation that really wants to >>>> just look up the system time. In the production code path, we cache the >>>> time for performance, and in testing, we use a mock time. >>>>>>> >>>>>>> Thanks, >>>>>>> John >>>>>>> >>>>>>> >>>>>>> On Fri, Jul 3, 2020, at 06:41, Piotr Smoliński wrote: >>>>>>> > 1. Makes sense; let me propose something >>>>>>> > >>>>>>> > 2. That's not testing-only. The goal is to use the same API to >>>> access >>>>>>> > the time >>>>>>> > in deployment and testing environments. The major driver is >>>>>>> > System.currentTimeMillis(), >>>>>>> > which a) cannot be used in tests b) could go in specific cases >>> back >>>> c) >>>>>>> > is not compatible >>>>>>> > with punctuator call. The idea is that we could access clock using >>>>>>> > uniform API. >>>>>>> > For completness we should have same API for system and stream >>> time. >>>>>>> > >>>>>>> > 3. There aren't that many subclasses. Two important ones are >>>>>>> > ProcessorContextImpl and >>>>>>> > MockProcessorContext (and third one: >>>>>>> > ForwardingDisableProcessorContext). If given >>>>>>> > implementation does not support schedule() call, there is no >>> reason >>>> to >>>>>>> > support clock access. >>>>>>> > The default implementation should just throw >>>>>>> > UnsupportedOperationException just to prevent >>>>>>> > from compilation errors in possible subclasses. >>>>>>> > >>>>>>> > On 2020/07/01 02:24:43, Boyang Chen <reluctanthero...@gmail.com> >>>> wrote: >>>>>>> > > Thanks Will for the KIP. A couple questions and suggestions: >>>>>>> > > >>>>>>> > > 1. I think for new APIs to make most sense, we should add a >>>> minimal example >>>>>>> > > demonstrating how it could be useful to structure unit tests w/o >>>> the new >>>>>>> > > APIs. >>>>>>> > > 2. If this is a testing-only feature, could we only add it >>>>>>> > > to MockProcessorContext? >>>>>>> > > 3. Regarding the API, since this will be added to the >>>> ProcessorContext with >>>>>>> > > many subclasses, does it make sense to provide default >>>> implementations as >>>>>>> > > well? >>>>>>> > > >>>>>>> > > Boyang >>>>>>> > > >>>>>>> > > On Tue, Jun 30, 2020 at 6:56 PM William Bottrell < >>>> bottre...@gmail.com> >>>>>>> > > wrote: >>>>>>> > > >>>>>>> > > > Thanks, John! I made the change. How much longer should I let >>>> there be >>>>>>> > > > discussion before starting a VOTE? >>>>>>> > > > >>>>>>> > > > On Sat, Jun 27, 2020 at 6:50 AM John Roesler < >>>> vvcep...@apache.org> wrote: >>>>>>> > > > >>>>>>> > > > > Thanks, Will, >>>>>>> > > > > >>>>>>> > > > > That looks good to me. I would only add "cached" or >>> something >>>>>>> > > > > to indicate that it wouldn't just transparently look up the >>>> current >>>>>>> > > > > System.currentTimeMillis every time. >>>>>>> > > > > >>>>>>> > > > > For example: >>>>>>> > > > > /** >>>>>>> > > > > * Returns current cached wall-clock system timestamp in >>>> milliseconds. >>>>>>> > > > > * >>>>>>> > > > > * @return the current cached wall-clock system timestamp in >>>> milliseconds >>>>>>> > > > > */ >>>>>>> > > > > long currentSystemTimeMs(); >>>>>>> > > > > >>>>>>> > > > > I don't want to give specific information about _when_ >>>> exactly the >>>>>>> > > > > timestamp cache will be updated, so that we can adjust it in >>>> the >>>>>>> > > > > future, but it does seem important to make people aware that >>>> they >>>>>>> > > > > won't see the timestamp advance during the execution of >>>>>>> > > > > Processor.process(), for example. >>>>>>> > > > > >>>>>>> > > > > With that modification, I'll be +1 on this proposal. >>>>>>> > > > > >>>>>>> > > > > Thanks again for the KIP! >>>>>>> > > > > -John >>>>>>> > > > > >>>>>>> > > > > On Thu, Jun 25, 2020, at 02:32, William Bottrell wrote: >>>>>>> > > > > > Thanks, John! I appreciate you adjusting my lingo. I made >>>> the change to >>>>>>> > > > > the >>>>>>> > > > > > KIP. I will add the note about system time to the javadoc. >>>>>>> > > > > > >>>>>>> > > > > > On Wed, Jun 24, 2020 at 6:52 PM John Roesler < >>>> vvcep...@apache.org> >>>>>>> > > > > wrote: >>>>>>> > > > > > >>>>>>> > > > > > > Hi Will, >>>>>>> > > > > > > >>>>>>> > > > > > > This proposal looks good to me overall. Thanks for the >>>> contribution! >>>>>>> > > > > > > >>>>>>> > > > > > > Just a couple of minor notes: >>>>>>> > > > > > > >>>>>>> > > > > > > The system time method would return a cached timestamp >>>> that Streams >>>>>>> > > > > looks >>>>>>> > > > > > > up once when it starts processing a record. This may be >>>> confusing, so >>>>>>> > > > > it >>>>>>> > > > > > > might be good to state it in the javadoc. >>>>>>> > > > > > > >>>>>>> > > > > > > I thought the javadoc for the stream time might be a bit >>>> confusing. >>>>>>> > > > We >>>>>>> > > > > > > normally talk about “Tasks” not “partition groups” in >>> the >>>> public api. >>>>>>> > > > > Maybe >>>>>>> > > > > > > just saying that it’s “the maximum timestamp of any >>>> record yet >>>>>>> > > > > processed by >>>>>>> > > > > > > the task” would be both high level and accurate. >>>>>>> > > > > > > >>>>>>> > > > > > > Thanks again! >>>>>>> > > > > > > -John >>>>>>> > > > > > > >>>>>>> > > > > > > On Mon, Jun 22, 2020, at 02:10, William Bottrell wrote: >>>>>>> > > > > > > > Thanks, Bruno. I updated the KIP, so hopefully it >>> makes >>>> more sense. >>>>>>> > > > > > > Thanks >>>>>>> > > > > > > > to Matthias J. Sax and Piotr Smolinski for helping >>> with >>>> details. >>>>>>> > > > > > > > >>>>>>> > > > > > > > I welcome more feedback. Let me know if something >>>> doesn't make >>>>>>> > > > sense >>>>>>> > > > > or I >>>>>>> > > > > > > > need to provide more detail. Also, feel free to >>>> enlighten me. >>>>>>> > > > Thanks! >>>>>>> > > > > > > > >>>>>>> > > > > > > > On Thu, Jun 11, 2020 at 1:11 PM Bruno Cadonna < >>>> br...@confluent.io> >>>>>>> > > > > > > wrote: >>>>>>> > > > > > > > >>>>>>> > > > > > > > > Hi Will, >>>>>>> > > > > > > > > >>>>>>> > > > > > > > > Thank you for the KIP. >>>>>>> > > > > > > > > >>>>>>> > > > > > > > > 1. Could you elaborate a bit more on the motivation >>>> in the KIP? >>>>>>> > > > An >>>>>>> > > > > > > > > example would make the motivation clearer. >>>>>>> > > > > > > > > >>>>>>> > > > > > > > > 2. In section "Proposed Changes" you do not need to >>>> show the >>>>>>> > > > > > > > > implementation and describe internals. A description >>>> of the >>>>>>> > > > > expected >>>>>>> > > > > > > > > behavior of the newly added methods should suffice. >>>>>>> > > > > > > > > >>>>>>> > > > > > > > > 3. In "Compatibility, Deprecation, and Migration >>>> Plan" you should >>>>>>> > > > > > > > > state that the change is backward compatible because >>>> the two >>>>>>> > > > > methods >>>>>>> > > > > > > > > will be added and no other method will be changed or >>>> removed. >>>>>>> > > > > > > > > >>>>>>> > > > > > > > > Best, >>>>>>> > > > > > > > > Bruno >>>>>>> > > > > > > > > >>>>>>> > > > > > > > > On Wed, Jun 10, 2020 at 10:06 AM William Bottrell < >>>>>>> > > > > bottre...@gmail.com >>>>>>> > > > > > > > >>>>>>> > > > > > > > > wrote: >>>>>>> > > > > > > > > > >>>>>>> > > > > > > > > > Add currentSystemTimeMs and currentStreamTimeMs to >>>>>>> > > > > ProcessorContext >>>>>>> > > > > > > > > > < >>>>>>> > > > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > >>>>>>> > > > >>>> >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-622%3A+Add+currentSystemTimeMs+and+currentStreamTimeMs+to+ProcessorContext >>>>>>> > > > > > > > > > >>>>>>> > > > > > > > > > >>>>>>> > > > > > > > > > I am extremely new to Kafka, but thank you to John >>>> Roesler and >>>>>>> > > > > > > Matthias >>>>>>> > > > > > > > > J. >>>>>>> > > > > > > > > > Sax for pointing me in the right direction. I >>>> accept any and >>>>>>> > > > all >>>>>>> > > > > > > > > feedback. >>>>>>> > > > > > > > > > >>>>>>> > > > > > > > > > Thanks, >>>>>>> > > > > > > > > > Will >>>>>>> > > > > > > > > >>>>>>> > > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > >>>>>>> > > > > >>>>>>> > > > >>>>>>> > > >>>>>>> > >>>> >>>> >>> >> >
signature.asc
Description: OpenPGP digital signature
