Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Thanks for the feedback. I updated the interface names to `TimestampedXxxStore`. About `RecordConverter` I would like to keep the name, because it's a generic concept and we will have a special implementation that does the timestamp conversion -- however, this should be reflected in the

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Matthias, Thanks for the updated wiki page. I made another pass and here are some minor comments I have (mostly about API names and writing itself): 1. The scope of the RecordConverter may not be very clear from the wiki page reads. Here's my understanding: 1.a) For build-in implementations

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

I also want to point out, that Ryanne Dolan commented on the WIP PR (https://github.com/apache/kafka/pull/6044) about the naming. I asked him to reply to this thread, but this did no happen yet, thus I want to point it out myself, because it seems to important. WindowWithTimestampStore.java >

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Bill, I left the question about legacy column family out, because as a matter of fact, we use the default column family atm that cannot be deleted. Thus, this old column family will always be there. Nevertheless, as an implementation detail, it might make sense to avoid accessing both column

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi Matthias, Thanks for the KIP, it goes into good detail and is well done. Overall I'm a +1 on the KIP and have one minor question. Regarding the upgrade path, we'll use two column families to do a lazy conversion which makes sense to me. What is the plan to get rid of the "legacy" column

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi Matthias, Thanks for the updates to the KIP. I've just read it over, and am personally quite happy with it. Thanks for tackling this dicey issue and putting in a huge amount of design work to produce a smooth upgrade path for DSL users. Thanks, -John On Mon, Dec 17, 2018 at 10:35 AM

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Dear all, I finally managed to update the KIP. To address the concerns about the complex upgrade path, I simplified the design. We don't need any configs and the upgrade can be done with the simple single rolling bounce pattern. The suggestion is to exploit RocksDB column families to isolate

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Thanks for the information Matthias. I will await your completion of this ticket then since it underpins the essential parts of a RocksDB TTL aligned with the changelog topic. I am eager to work on that ticket myself, so if I can help on this one in any way please let me know. Thanks Adam On

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

It's an interesting idea to use second store, to maintain the timestamps. However, each RocksDB instance implies some overhead. In fact, we are looking into ColumnFamilies atm to see if we can use those and merge multiple RocksDBs into a single one to reduce this overhead. -Matthias On 11/20/18

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi Adam Sounds great, I was already planning to ask around if anyone had tackled this. We have a use case very similar to what you described in KAFKA-4212, only with Global State Stores. I have tried a few things with the normal DSL but was not really successful. Schedule/Punctuate is not

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

I did not think about how TTL should be integrated exactly. However, it seems reasonable, to provide TTL not based on RocksDB built-in TTL, because we want to synchronize deletes in RocksDB with deletes in the changelog topic (to avoid the filtering you implemented). Thus, on each put() we could

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi Matthias Thanks - I figured that it was probably a case of just too much to do and not enough time. I know how that can go. I am asking about this one in relation to https://issues.apache.org/jira/browse/KAFKA-4212, adding a TTL to RocksDB. I have outlined a bit about my use-case within 4212,

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Adam, I am still working on it. Was pulled into a lot of other tasks lately so this was delayed. Also had some discussions about simplifying the upgrade path with some colleagues and I am prototyping this atm. Hope to update the KIP accordingly soon. -Matthias On 11/10/18 7:41 AM, Adam

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hello Matthias I am curious as to the status of this KIP. TTL and expiry of records will be extremely useful for several of our business use-cases, as well as another KIP I had been working on. Thanks On Mon, Aug 13, 2018 at 10:29 AM Eno Thereska wrote: > Hi Matthias, > > Good stuff. Could

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi Matthias, Good stuff. Could you comment a bit on how future-proof is this change? For example, if we want to store both event timestamp "and" processing time in RocksDB will we then need another interface (e.g. called KeyValueWithTwoTimestampsStore)? Thanks Eno On Thu, Aug 9, 2018 at 2:30

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Thanks for your input Guozhang and John. I see your point, that the upgrade API is not simple. If you don't thinks it's valuable to make generic store upgrades possible (atm), we can make the API internal, too. The impact is, that we only support a predefined set up upgrades (ie, KV to KVwithTs,

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi Matthias, I think this KIP is looking really good. I have a few thoughts to add to the others: 1. You mentioned at one point users needing to configure `upgrade.mode="null"`. I think this was a typo and you meant to say they should remove the config. If they really have to set it to a string

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hello Matthias, Thanks for the updated KIP. Some more comments: 1. The current set of proposed API is a bit too complicated, which makes the upgrade flow from user's perspective also a bit complex. I'd like to check different APIs and discuss about their needs separately: 1.a. StoreProxy:

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Thanks for the feedback Bill. I just update the KIP with some of your points. >> Regarding step 3C of the in-place upgrade (users needing to watch the >> restore process), I'm wondering if we want to provide a type of >> StateRestoreListener that could signal when the new stores have reached >>

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi Matthias, Thanks for the update and the working prototype, it helps with understanding the KIP. I took an initial pass over this PR, and overall I find the interfaces and approach to be reasonable. Regarding step 3C of the in-place upgrade (users needing to watch the restore process), I'm

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hi, KIP-268 (rebalance meatadata) is finished and included in AK 2.0 release. Thus, I want to pick up this KIP again to get the RocksDB upgrade done for 2.1. I updated the KIP accordingly and also have a "prove of concept" PR ready (for "in place" upgrade only):

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

After some more thoughts, I want to follow John's suggestion and split upgrading the rebalance metadata from the store upgrade. I extracted the metadata upgrade into it's own KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-268%3A+Simplify+Kafka+Streams+Rebalance+Metadata+Upgrade I'll

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

@John: yes, we would throw if configs are missing (it's an implementation details IMHO and thus I did not include it in the KIP) @Guozhang: 1) I understand know what you mean. We can certainly, allow all values "0.10.0.x", "0.10.1.x", "0.10.2.x", ... "1.1.x" for `upgrade.from` parameter. I had a

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hello Matthias, thanks for your replies. 1) About the config names: actually I was trying to not expose implementation details :) My main concern was that in your proposal the values need to cover the span of all the versions that are actually using the same version, i.e. "0.10.1.x-1.1.x". So if

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

This all sounds awesome to me. One (very minor) thought about the config parameters. You might consider throwing an exception if "upgrade.from" is set but "upgrade.mode" is not. This would eliminate the risk that folks forget to unset "upgrade.from" and just leave it in their configs

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

@John, Guozhang, thanks a lot for your comments. Very long reply... About upgrading the rebalance metadata: Another possibility to do this, would be to register multiple assignment strategies for the 1.2 applications. For this case, new instances would be configured to support both and the

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

@John: For the protocol version upgrade, it is only for the encoded metadata bytes protocol, which are just bytes-in bytes-out from Consumer's pov, so I think this change should be in the Streams layer as well. @Matthias: for 2), I agree that adding a "newest supported version" besides the

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Ted, I still consider changing the KIP to include it right away -- if not, I'll create a JIRA. Need to think it through in more detail first. (Same for other open questions like interface names -- I collect feedback and update the KIP after we reach consensus :)) -Matthias On 3/9/18 3:35 PM,

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hey James and Matthias, It seems clear (to me) that there's no way to avoid a double bounce for this release. But I do think we should figure out whether there's a feature we can put in right now to allow future releases to be single-bounce. I'm just thinking that this double bounce thing is the

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Thanks for the details, Matthias. bq. change the metadata protocol only if a future release, encoding both used and supported version might be an advantage Looks like encoding both versions wouldn't be implemented in this KIP. Please consider logging a JIRA with the encoding proposal. Cheers

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

@Bill: I think a filter predicate should be part of user code. And even if we want to add something like this, I would prefer to do it in a separate KIP. @James: I would love to avoid a second rolling bounce. But from my understanding it would not be possible. The purpose of the second rolling

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Matthias, For all the upgrade paths, is it possible to get rid of the 2nd rolling bounce? For the in-place upgrade, it seems like primary difference between the 1st rolling bounce and the 2nd rolling bounce is to decide whether to send Subscription Version 2 or Subscription Version 3.

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Matthias, Thanks for the KIP, it's a +1 from me. I do have one question regarding the retrieval methods on the new interfaces. Would want to consider adding one method with a Predicate that would allow for filtering records by the timestamp stored with the record? Or is this better left for

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Matthias: For my point #1, I don't have preference as to which separator is chosen. Given the background you mentioned, current choice is good. For #2, I think my proposal is better since it is closer to English grammar. Would be good to listen to what other people think. On Thu, Mar 8, 2018 at

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Thanks for the comments! @Guozhang: So far, there is one PR for the rebalance metadata upgrade fix (addressing the mentioned https://issues.apache.org/jira/browse/KAFKA-6054) It give a first impression how the metadata upgrade works including a system test:

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hey Matthias, The KIP looks good to me. I had several questions queued up, but they were all in the "rejected alternatives" section... oh, well. One very minor thought re changing the state directory from "//< application.id>//rocksdb/storeName/" to "//< application.id>//rocksdb-v2/storeName/":

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Matthias: Nicely written KIP. "in_place" : can this be "in-place" ? Underscore may sometimes be miss typed (as '-'). I think using '-' is more friendly to user. public interface ReadOnlyKeyValueTimestampStore { Is ReadOnlyKeyValueStoreWithTimestamp better name for the class ? Thanks On

Re: [DISCUSS] KIP-258: Allow to Store Record Timestamps in RocksDB

Hello Matthias, thanks for the KIP. I've read through the upgrade patch section and it looks good to me, if you already have a WIP PR for it could you also share it here so that people can take a look? I'm +1 on the KIP itself. But large KIPs like this there are always some devil hidden in the