Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

1. We don't need to worry about impl detail. But yes, we can remove the method from the interanl context that extends `ProcessorContext` already 2. Same here: we can discuss on the PR. Btw: it seems you got enough votes. Can you close the vote? Looking forward to your PR. -Matthias On

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

Hi, I would like to revive this KIP. 1. As per proposed solution, we want to add following method in ProcessorContext class /** * Returns current cached wall-clock system timestamp in milliseconds. * * @return the current cached wall-clock system timestamp in milliseconds */ long

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

Thanks for the reply, Matthias, I see what you mean. I suppose I was thinking that we would pass in the cached system time, which is also what we’re proposing to add to the ProcessorContext. If you think there’s something about the timestamp extractor in particular that would make people want

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

Interesting proposal. However, it raises the question how the runtime would pass in the `systemTime` parameter? To be accurate, we would need to call `Time.milliseconds()` each time before we call the timestamp extractor. This sound expensive and maybe the extractor does not even use this value.

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

Thanks Matthias, This is a really good point. It might be a bummer to have to actually change the topology between testing and production. Do you think we can rather evolve the TimestampExtractor interface to let Streams pass in the current system time, along with the current record and the

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

Hi, I just had one more thought about an additional improvement we might want to include in this KIP. Kafka Streams ships with a `WallclockTimestampExtractor` that just returns `System.currentTimeMillis()` and thus, cannot be mocked. And it seems that there is no way for a user to build a custom

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

Sure, I would appreciate help from Piotr creating an example. On Tue, Jul 7, 2020 at 12:03 PM Boyang Chen 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.

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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.

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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 wrote: > Thanks, Will, > > That looks good to me. I would only add "cached" or something > to indicate that it wouldn't just transparently look

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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 wrote: > Hi Will, > > This proposal looks good to me overall. Thanks for the contribution! > > Just a couple of

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

Re: [DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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

[DISCUSS] KIP-622 Add currentSystemTimeMs and currentStreamTimeMs to ProcessorContext

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