Hi all, This reminds me of a previous issue I think that we were discussing. @John Roesler <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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
