Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2021-01-17 Thread Dongjin Lee
Hi John, After long thoughts, I successfully made TimeOrderedKeyValueBuffer extend ReadOnlyKeyValueStore (with supporting all, range iterations) by separating its serde/metric operations into independent class, like the other StateStore implementations (like InMemoryKeyValueStore,

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-10-06 Thread John Roesler
Hi Dongjin, Yes, those were the APIs I was thinking of. I honestly didn’t think of it until now. Sorry about that. I agree, a POC implementation would help us to see if this is a good choice for the kip. Thanks! John On Tue, Oct 6, 2020, at 10:21, Dongjin Lee wrote: > You mean, the

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-10-06 Thread Dongjin Lee
You mean, the performance issue related to `#all` or `#range` query. Right? I reviewed the second approach (i.e., extending `ValueGetter`), and this approach is worth trying. Since KIP-508 was dropped from 2.7.0 release, we have enough time now. Let me have a try. I think we can have a rough one

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-30 Thread John Roesler
Thanks Dongjin, It typically is nicer to be able to see usage examples, so I'd certainly be in favor if you're willing to add it to the KIP. I'm wondering if it's possible to implement the whole ReadOnlyKeyValueStore interface as proposed, if we really go ahead and just internally query into the

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-30 Thread Dongjin Lee
> It seems like it must be a ReadOnlyKeyValueStore. Does that sound right? Yes, it is. Would it be better to add a detailed description of how this feature effects interactive query, with examples? Best, Dongjin On Tue, Sep 29, 2020 at 10:31 AM John Roesler wrote: > Hi Dongjin, > > Thanks!

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-28 Thread John Roesler
Hi Dongjin, Thanks! Sorry, I missed your prior message. The proposed API looks good to me. I’m wondering if we should specify what kind of store view would be returned when querying the operation result. It seems like it must be a ReadOnlyKeyValueStore. Does that sound right? Thanks! John

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-28 Thread Dongjin Lee
Hi John, I updated the KIP with the discussion above. The 'Public Interfaces' section describes the new API, and the 'Rejected Alternatives' section describes the reasoning about why we selected this API design and rejected the other alternatives. Please have a look when you are free. And please

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-21 Thread Dongjin Lee
Hi John, I updated the PR applying the API changes we discussed above. I am now updating the KIP document. Thanks, Dongjin On Sat, Sep 19, 2020 at 10:42 AM John Roesler wrote: > Hi Dongjin, > > Yes, that’s right. My the time of KIP-307, we had no choice but to add a > second name. But we do

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-18 Thread John Roesler
Hi Dongjin, Yes, that’s right. My the time of KIP-307, we had no choice but to add a second name. But we do have a choice with Suppress. Thanks! -John On Thu, Sep 17, 2020, at 13:14, Dongjin Lee wrote: > Hi John, > > I just reviewed KIP-307. As far as I understood, ... > > 1. There was

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-17 Thread Dongjin Lee
Hi John, I just reviewed KIP-307. As far as I understood, ... 1. There was Materialized name initially. 2. With KIP-307, Named Operations were added. 3. Now we have two options for materializing suppression. If we take Materialized name here, we have two names for the same operation, which is

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-16 Thread John Roesler
Hi Dongjin, Thanks for the reply. Yes, that’s correct, we added that method to name the operation. But the operation seems synonymous with the view produced the operation, right? During KIP-307, I remember thinking that it’s unfortunate the we had to have two different “name” concepts for the

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-16 Thread Dongjin Lee
Hi John, It seems like the available alternatives in this point is clear: 1. Pass queriable name as a separate parameter (i.e., `KTable#suppress(Suppressed, String)`) 2. Make use of the Suppression processor name as a queryable name by adding `enableQuery` optional flag to `Suppressed`.

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-16 Thread John Roesler
Hi Dongjin, Yes, that's where I was leaning. Although, I'd prefer adding the option to Suppressed instead of adding a new argument to the method call. What do you think about: class Suppressed { + public Suppressed enableQuery(); } Since Suppressed already has `withName(String)`, it seems

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-16 Thread Dongjin Lee
Hi John, > Although it's not great to have "special snowflakes" in the API, Choice B does seem safer in the short term. We would basically be proposing a temporary API to make the suppressed view queriable without a Materialized argument. Then, it seems like you prefer

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-15 Thread John Roesler
Hi Dongjin, Thanks for presenting these options. The concern that Matthias brought up is a very deep problem that afflicts all operations downstream of windowing operations. It's the same thing that derailed KIP-300. For the larger context, I have developed a couple of approaches to resolve this

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-15 Thread Dongjin Lee
Hi Matthias, Thank you very much for the detailed feedback. Here are my opinions: > Because there is no final result for non-windowed KTables, it seems that this new feature only make sense for the windowed-aggregation case? I think a little bit different. Of course, for windowed KTable, this

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-11 Thread Matthias J. Sax
Thanks for updating the KIP. I think there is still one open question. `suppress()` can be used on non-windowed KTable for rate control, as well as on a windowed-KTable (also for rate control, but actually mainly) for only emitting the final result of a windowed aggregation. For the non-windowed

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-11 Thread Dongjin Lee
Hi All, Here is the voting thread: https://lists.apache.org/thread.html/r5653bf2dafbb27b247bf20dbe6f070c151b3823d96c9c9ca94183e20%40%3Cdev.kafka.apache.org%3E Thanks, Dongjin On Fri, Sep 11, 2020 at 4:23 PM Dongjin Lee wrote: > Hi John, > > Thanks for the feedback. I will open the Vote thread

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-11 Thread Dongjin Lee
Hi John, Thanks for the feedback. I will open the Vote thread now. Best, Dongjin On Fri, Sep 11, 2020 at 2:00 AM John Roesler wrote: > Hi Dongjin, > > Sorry for the delay. I'm glad you're still pushing this > forward. It would be nice to get this in to the 2.7 release. > > I just took another

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-09-10 Thread John Roesler
Hi Dongjin, Sorry for the delay. I'm glad you're still pushing this forward. It would be nice to get this in to the 2.7 release. I just took another look at the KIP, and it looks good to me! I think this is ready for a vote. Thanks, -John On Wed, 2020-08-05 at 22:04 +0900, Dongjin Lee wrote:

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-08-05 Thread Dongjin Lee
Hi All, I updated the KIP and the implementation, following the discussion here. You must be working hard preparing the release of 2.6.0, so please have a look after your work is done. Thanks,

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-03-07 Thread John Roesler
Thanks Matthias, Good idea. I've changed the ticket name and added a note clarifying that this ticket is not the same as https://issues.apache.org/jira/browse/KAFKA-7224 Incidentally, I learned that I never documented my reasons for abandoning my work on KAFKA-7224 ! I've now updated that

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-03-07 Thread Matthias J. Sax
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Thanks for clarification. Can you maybe update the Jira ticket? Do we have a ticket for spill-to-disk? Maybe link to it and explain that it's two different things? Maybe even rename the ticket to something more clear, ie, "make suppress result

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-03-07 Thread John Roesler
Hey Matthias, I’m sorry if the ticket was poorly stated. The ticket is to add a DSL overload to pass a Materialized argument to suppress. As a result, the result of the suppression would be queriable. This is unrelated to “persistent buffer” aka “spill-to-disk”. There was some confusion

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-03-07 Thread Matthias J. Sax
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Thanks for the KIP Dongjin, I am still not sure if I can follow, what might also be caused by the backing JIRA ticket (maybe John can clarify the intent of the ticket as he created it): Currently, suppress() only uses an in-memory buffer and my

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-27 Thread John Roesler
Hi Dongjin, No problem; glad we got it sorted out. Thanks again for picking this up! -John On Wed, Feb 26, 2020, at 09:24, Dongjin Lee wrote: > > I was under the impression that you wanted to expand the scope of the KIP > to additionally allow querying the internal buffer, not just the result.

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-26 Thread Dongjin Lee
> I was under the impression that you wanted to expand the scope of the KIP to additionally allow querying the internal buffer, not just the result. Can you clarify whether you are proposing to allow querying the state of the internal buffer, the result, or both? Sorry for the confusion. As we

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-24 Thread John Roesler
Hi Dongjin, Ah, I think I may have been confused. I 100% agree that we need a materialized variant for suppress(). Then, you could do: ...suppress(..., Materialized.as(“final-count”)) If that’s your proposal, then we are on the same page. I was under the impression that you wanted to expand

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-20 Thread Dongjin Lee
Hi John, Thanks for your kind explanation with an example. > But it feels like you're saying you're trying to do something different than just query the windowed key and get back the current count? Yes, for example, what if we need to retrieve the (all or range) keys with a closed window? In

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-19 Thread John Roesler
Thanks for the response, Dongjin, I'm sorry, but I'm still not following. It seems like the view you would get on the "current state of the buffer" would always be equivalent to the view of the upstream table. Let me try an example, and maybe you can point out the flaw in my reasoning. Let's

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-19 Thread Dongjin Lee
Hi John, 'The intermediate state of the suppression' in KIP does not mean the state of upstream KTable - sure, the state of the upstream KTable can be queried by materializing the operator immediately before the suppress as you shown. What I meant in KIP was the final state of the buffer, which

Re: [DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-14 Thread John Roesler
Hi Dongjin, Thanks for the KIP! Can you explain more about why the internal data structures of suppression should be queriable? The motivation just says that users might want to do it, which seems like it could justify literally anything :) One design point of Suppression is that if you

[DISCUSS] KIP-508: Make Suppression State Queriable - rebooted.

2020-02-14 Thread Dongjin Lee
Hi devs, I'd like to reboot the discussion on KIP-508, which aims to support a Materialized variant of KTable#suppress. It was initially submitted several months ago but closed by the inactivity. - KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-508%3A+Make+Suppression+State+Queriable

Re: [DISCUSS] KIP-508: Make Suppression State Queriable

2019-11-24 Thread Matthias J. Sax
Moved this KIP into status "inactive". Feel free to resume and any time. -Matthias On 8/16/19 7:08 AM, Dongjin Lee wrote: > Hi all, > > I would like to start a discussion of KIP-508, making suppression state > queriable. Please give it a read when you are free and give some feedbacks. > >

[DISCUSS] KIP-508: Make Suppression State Queriable

2019-08-16 Thread Dongjin Lee
Hi all, I would like to start a discussion of KIP-508, making suppression state queriable. Please give it a read when you are free and give some feedbacks. https://cwiki.apache.org/confluence/display/KAFKA/KIP-508%3A+Make+Suppression+State+Queriable Thanks, Dongjin -- *Dongjin Lee* *A