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
