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: https://github.com/apache/kafka/pull/4636 I can share other PRs as soon as they are ready. I agree that the KIP is complex am I ok with putting out more code to give better discussion context. @Ted: I picked `_` instead of `-` to align with the `processing.guarantee` parameter that accepts `at_least_one` and `exactly_once` as values. Personally, I don't care about underscore vs dash but I prefer consistency. If you feel strong about it, we can also change it to `-`. About the interface name: I am fine either way -- I stripped the `With` to keep the name a little shorter. Would be good to get feedback from others and pick the name the majority prefers. @John: We can certainly change it. I agree that it would not make a difference. I'll dig into the code to see if any of the two version might introduce undesired complexity and update the KIP if I don't hit an issue with putting the `-v2` to the store directory instead of `rocksdb-v2` -Matthias On 3/8/18 2:44 PM, John Roesler wrote: > 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 "/<state.dir>/< > application.id>/<task.id>/rocksdb/storeName/" to "/<state.dir>/< > application.id>/<task.id>/rocksdb-v2/storeName/": if you put the "v2" > marker on the storeName part of the path (i.e., "/<state.dir>/< > application.id>/<task.id>/rocksdb/storeName-v2/"), then you get the same > benefits without altering the high-level directory structure. > > It may not matter, but I could imagine people running scripts to monitor > rocksdb disk usage for each task, or other such use cases. > > Thanks, > -John > > On Thu, Mar 8, 2018 at 2:02 PM, Ted Yu <yuzhih...@gmail.com> wrote: > >> 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<K, V> { >> >> Is ReadOnlyKeyValueStoreWithTimestamp better name for the class ? >> >> Thanks >> >> On Thu, Mar 8, 2018 at 1:29 PM, Guozhang Wang <wangg...@gmail.com> wrote: >> >>> 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 details, so I think it is better to have the >>> implementation in parallel along with the design discussion :) >>> >>> >>> Guozhang >>> >>> >>> On Wed, Mar 7, 2018 at 2:12 PM, Matthias J. Sax <matth...@confluent.io> >>> wrote: >>> >>>> Hi, >>>> >>>> I want to propose KIP-258 for the Streams API to allow storing >>>> timestamps in RocksDB. This feature is the basis to resolve multiple >>>> tickets (issues and feature requests). >>>> >>>> Looking forward to your comments about this! >>>> >>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- >>>> 258%3A+Allow+to+Store+Record+Timestamps+in+RocksDB >>>> >>>> >>>> -Matthias >>>> >>>> >>>> >>> >>> >>> -- >>> -- Guozhang >>> >> >
signature.asc
Description: OpenPGP digital signature
