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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
