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