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
