KAFKA-3522: Add RocksDBTimestampedStore by mjsax · Pull Request #6149 · apache/kafka

mjsax · 2019-01-15T22:55:48Z

Part of KIP-258. adds only internal classes (we can merge right away).

mjsax · 2019-01-15T22:56:07Z

Call for review @guozhangwang @bbejeck @vvcephei

mjsax · 2019-01-15T23:21:16Z

The purpose of this test is, to catch interface changes if we upgrade RocksDB. Using reflections, we make sure the RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsFacade maps all methods from DBOptions and ColumnFamilyOptions to/from Options correctly.

very nice!

Actually, can you add that statement as a class javadoc, for posterity?

Was it possible for you to test the test? I.e., how do you know the test does what it's supposed to do?

I tested it "manually" by removing one method in RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsFacade -- this lets the test fail. Is this sufficient for you?

\cc @vvcephei

Yep! Sorry, I lost your reply in the shuffle.

That's sufficient. It looked right to me, just wanted to make sure we've seen it fail at least once.

mjsax · 2019-01-15T23:25:01Z

In contrast to RocksDBStore, we use column families. Thus, we cannot pass in Options object that is used by users to specify custom RocksDB options via StreamsConfig; instead we need to use DBOptions and ColumnFamilyOptions -- thus, we need to translated from Options into those two classed via this helper class.

mjsax · 2019-01-15T23:26:17Z

We try to open the DB with both column families -- this might fail if only one exist. For this case, we create the CF and retry to open the DB afterwards

mjsax · 2019-01-15T23:27:18Z

All operations are defined over both CF; ie, we do dual put/get operation to migrate data from default CF (that does not store any timestamps) to the new CF

mjsax · 2019-01-15T23:29:28Z

This class will get more helper method later...

mjsax · 2019-01-17T22:17:14Z

This and other stuff in this class is only small code cleanups and reordering of method. There is no actual change.

👍 Thanks for doing it.

bbejeck

Thanks for the hard work @mjsax. I made an initial pass and left some comments.

bbejeck · 2019-01-17T21:26:32Z

nit: I personally don't like the double brace initialization for Java collections, but since this subjective feel free to ignore this comment.

bbejeck · 2019-01-17T21:31:47Z

return an empty Array instead?

null indicates not found and should be correct. Let me know if you disagree.

Yeah I agree, forgot about returning null from store to indicate not found

bbejeck · 2019-01-17T21:37:44Z

why two calls here to db.compactRange? Do we need to make a call for each column family?

On oversight from my side. I think we can just call it once (will remove the duplicate line).

The docs are not very specific if this triggers compaction for all CF, but I believe yes. There is also an API to compact one CF, but I don't see any reason to use it. Thoughts?

Thinking about this one more, I actually believe that this would only compact default CF -- I'll add this to the RocksDBAccessor interface and pass in CF handlers.

bbejeck · 2019-01-17T21:55:44Z

For the operations involving the noTimestampColumnFamily maybe we could put a guard condition around each operation if(noTimesampColumnFamilyNotEmpty) {..} and the boolean flag is set when opening the db and check the size of the default column family with something like approximateNumEntries but only checking the noTimestampColumnFamily.

Addressed this with the new RocksDBAccessor interface.

bbejeck · 2019-01-17T22:00:08Z

Will this work with callers? Will we inspect each value and insert an unknown timestamp for records from default column family?

EDIT: NM I didn't see the RocksDbIterator class below when writing this comment.

mjsax · 2019-01-17T22:18:45Z

Ok. This is an actual change. Need to make this class static to be reused in new RocksDBTimestampedStore class. For this reason, we need to pass in openIterators in the constructor now.

Makes sense. If it's static, then we could also extract it to a separate file, which might be better than having one of the siblings define a class that both siblings use.

Also, it would reduce the LOC in this class, which is nice for readability.

mjsax · 2019-01-17T22:22:13Z

Updated this with RocksDBAccessor interface, that allows us to implement single/dual column-family access, depending if there is data in the old column family or not.

bbejeck

Thanks for the updates @mjsax. I've made another pass, and I like the approach of determining when to stop using both column families.

Regarding a test for RocksDBTimestampedStore I have a proposal,

Open a plain RocksDB give it the two expected column families and populate the RocksDB.DEFAULT_COLUMN_FAMILY with a known number of records, then close the plain RocksDB.
Open an instance of RocksDBTimestampedStore then do a series of gets on the known keys and assert all returned with the unknown timestamp, then close.
Open the plain RocksDB again and assert that RocksDB.DEFAULT_COLUMN_FAMILY` is empty.

WDYT?

mjsax · 2019-01-18T23:53:14Z

@bbejeck Added a test as requested :)

mjsax · 2019-01-20T07:10:35Z

restoreAllInternal() is only called here, so I think it's better to inline it.

Now I see why we need to make name package-private now; think it is okay.

mjsax · 2019-01-20T07:12:52Z

This test let checkstyle fail because method was too long. Extracted some part into prepareOldStore() to fix checkstyle.

mjsax · 2019-01-20T07:13:18Z

Similar as above: extracted into own method to avoid checkstyle issue

bbejeck

thanks for the updates and test @mjsax, overall looks good, I'd like to make one more pass

vvcephei

Thanks for the PR @mjsax .

Overall, it looks good. I left a few comments...

vvcephei · 2019-01-22T17:13:23Z

Neat!

Although, it seems like more of an adapter than a facade ;)

Ack. Renaming.

vvcephei · 2019-01-22T17:16:12Z

I couldn't find the reference that makes it necessary to make this public...

Ack. Reverting.

vvcephei · 2019-01-22T17:16:37Z

👍 Thanks for doing it.

vvcephei · 2019-01-22T17:21:01Z

Makes sense. If it's static, then we could also extract it to a separate file, which might be better than having one of the siblings define a class that both siblings use.

Also, it would reduce the LOC in this class, which is nice for readability.

vvcephei · 2019-01-22T17:31:00Z

This comment seems misplaced. IIUC, the class already restricts the key type to Bytes.

c&p issue (from RocksDBStore). At some point, the class had generics and it seems while refactoring this we missed to update this JavaDoc. Fixing in both classes.

vvcephei · 2019-01-22T17:52:58Z

I don't think it matters. This implementation is pretty straightforward; it doesn't seem like the bytebuffer really has any advantage.

If the others agree, we should get rid of the todo.

It the same question as #6151 (comment)

I would like to get a uniform agreement/policy what to use and will update the code accordingly after we made a decision. \cc @guozhangwang

I'd say just remove the TODO; using either is fine here.

As discussed in person, I update to use ByteBuffer what should be our default API to use to modify byte arrays.

vvcephei · 2019-01-22T17:54:50Z

very nice!

Actually, can you add that statement as a class javadoc, for posterity?

vvcephei · 2019-01-22T17:56:57Z

Was it possible for you to test the test? I.e., how do you know the test does what it's supposed to do?

mjsax · 2019-01-23T00:42:23Z

Updated this.

guozhangwang

Made a pass on non-testing code.

guozhangwang · 2019-01-23T20:04:41Z

Out of scope of this: we should consider have a KIP in the next release to refactor APIs of config setters as:

public interface RocksDBConfigSetter { /** * Set the rocks db options for the provided storeName. * * @param storeName the name of the store being configured * @param options the Rocks DB options * @param configs the configuration supplied to {@link org.apache.kafka.streams.StreamsConfig} */ void setConfig(final String storeName, final DBOptions dbOptions, final ColumnFamilyOptions cfOptions, final Map<String, Object> configs); }

And then we can choose to call the other constructor of

public Options(final DBOptions dbOptions, final ColumnFamilyOptions columnFamilyOptions) {..}

Sure. Can you please create a Jira for tracking? Otherwise it might get dropped.

https://issues.apache.org/jira/browse/KAFKA-7869

guozhangwang · 2019-01-23T20:11:12Z

Looked at https://github.com/facebook/rocksdb/blob/master/options/options.cc#L367 there seems no code for setting memtable config here.

I'd suggest we just mimic the code here instead of following the FAQ since that page may well obsoleted.

Ack. Will update the comment accordingly.

guozhangwang · 2019-01-23T20:13:58Z

guozhangwang · 2019-01-23T20:15:02Z

Meta: this and the existing class still share a lot of common code; I'm wondering if it is possible to just add a flag to the existing class, based on which we can decide whether use Options v.s. DBOptions / CFOptions, and use RocksDBAcccessor.

Yes. That works. We can share more code.

guozhangwang · 2019-01-23T22:32:48Z

Just following on my other idea about collapsing into a single class here: maybe instead of naming it as keyValueWithTimestamp, we just name it as:

"default" -> version 2.1-

"2.2" -> version 2.2 to now.

And the flag can just be indicating if it is 1) or 2) above; in the future if we need to do this again we can then have:

"default" -> version 2.1-

"2.2" -> version 2.2 - 2.5 (just made that up).

"3.0" -> version 3.0 - now.

etc.

We cannot rename default column family (and we cannot delete it either) -- it's there all the time. I can still rename the new CF to 2.2 if you want. Let me know.

guozhangwang · 2019-01-23T22:55:56Z

I'd suggest try-catch each line separately since the underlying RocksDBException would not tell you which line actually went wrong, and this piece of info would be very useful for trouble shooting; ditto below.

guozhangwang · 2019-01-23T22:56:57Z

I think we can still use write(batch); here for efficiency?

write(batch) should be done by the caller -- that we call it in the other accessor is a bug -- will rename the method to prepareBatch().

Not sure I can follow this: could you elaborate a bit more? I was thinking to use the RocksDB's

void write(final WriteOptions writeOpts, final WriteBatch updates)

API for the putAll call, is it possible?

That is exactly what is use. Compare RocksDB#putAll():

@Override public void putAll(final List<KeyValue<Bytes, byte[]>> entries) { try (final WriteBatch batch = new WriteBatch()) { dbAccessor.prepareBatch(entries, batch); write(batch); } catch (final RocksDBException e) { throw new ProcessorStateException("Error while batch writing to store " + name, e); } }

It calls dbAccessor.prepareBatch(entries, batch); (already renamed from putAll -> prepareBatch) that is this method. We do the call "outside" because only who the batch is prepared differs.

Does this make sense

guozhangwang · 2019-01-23T22:59:37Z

I'd say just remove the TODO; using either is fine here.

guozhangwang · 2019-01-23T23:01:36Z

Which do you think is more accurate, this or db.getLongProperty("rocksdb.estimate-num-keys"); directly?

If CF is not specified, not all CF all accessed by default CF only -- thus, we need to add both.

I extended RocksDBTimestampedStoreTest accordingly to test approximateNumEntries(), too.

guozhangwang · 2019-01-23T23:02:05Z

We can probably save this class if we consolidate on one RocksDBStore.

UPDATE: can we just modify RocksDbKeyValueBytesStoreSupplier to allow passing in a flag, based on which we will construct RocksDB v.s. TRocksDB?

mjsax · 2019-01-25T07:10:04Z

Updated this. Reworked RocksDB and RocksDBTimestampStore to share more code. Both use RocksDBAccessor now. Also consolidate the two corresponding tests.

Rebased to get BloomFilter changes that got merged recently.

guozhangwang

Made another pass over the PR.

guozhangwang · 2019-01-28T03:33:24Z

+    }
+
+    @Override
+    public synchronized KeyValue<Bytes, byte[]> next() {


Though this is not introduced in this PR: this function seems not needed?

Yes. Maybe we should add the check if the iterator is open? Similar to hasNext() ?

Never mind. super.next() calls hasNext() anyway. This part is covered. Removing this method.

guozhangwang · 2019-01-28T03:39:24Z

Now I see why we need to make name package-private now; think it is okay.

guozhangwang · 2019-01-28T03:42:08Z

+    final String name;
    private final String parentDir;
-    private final Set<KeyValueIterator> openIterators = Collections.synchronizedSet(new HashSet<>());
+    final Set<KeyValueIterator<Bytes, byte[]>> openIterators = Collections.synchronizedSet(new HashSet<>());


Why openIterators needs to be package-private now?

guozhangwang · 2019-01-28T03:49:09Z

+    public synchronized byte[] get(final Bytes key) {
+        validateStoreOpen();
+        try {
+            return dbAccessor.get(key.get());


For put / putAll etc the exception is captured inside the dbAccessor, while for get it is captured in the caller, is it intentional?

If not, we should move the exception capturing logic inside the dbAccessor as well.

Fair question. If we move all try-catch into the dbAccessor, we need to duplicate more code. For example, db.flush() could throw and catching the exception in RocksDB#flush() is a single place -- if we move it into the dbAccessor, we need to to try-catch in both implementations.

Thoughts?

put is different, because you requested to catch put() and get() within DualAccessor separately -- thus, I moved the code inside.

For putAll() does also throw (main pattern is that the caller catches).

guozhangwang · 2019-01-28T03:49:54Z

-        final byte[] value = get(key);
+        final byte[] oldValue;
+        try {
+            oldValue = dbAccessor.getOnly(key.get());


Ditto here.

guozhangwang · 2019-01-28T04:04:17Z

+        }
+    }
+
+    private class DualColumnFamilyAccessor implements RocksDBAccessor {


nit: maybe we can make it just a general accessor that takes two parameters: oldCF and newCF? Or we can do this generalizing in the future if you'd like to hard-code for now.

If we make one, how does it now that it need to upgrade stuff or not? Seems to imply a null check for each operation if both or only one CF should be accessed? That would imply runtime overhead. Also, I think the code would be a little bit harder to read. Thoughts?

We will create the DualColumnFamilyAccessor only when at the construction time if we found there are two CFs and the old CF is not empty right?

So that means we do not need to do extra check inside this accessor impl right?

+1 from me as well for putting both accessors into separate classes

I looked at it closer. I still think it's better to split them out, but I also don't think it's a correctness issue right now, so I'd be fine with merging what you have.

Let's to some refactoring as follow ups. It's internal anyway. Would like to keep the PRs focused to get stuff in and merged :)

Actually I was only suggesting to make the current Dual accessor to be more general
currently it assumes the old to be default, and new to be withTimestamp.
what I was suggesting is only to make these two parameterized; so that in the future we only have two accessor impls:

XX-CF only; which we already do in this PR.

XX-to-YY upgrade: old XX CF to YY CF upgrade accessor.

All that being said, I'm okay with such refactoring as follow-ups.

This refactoring is already done: DualAccessor has a constructor with two CF parameters.

There is one missing piece: the conversion function is hard coded. After TimestampedByteStore PR is merged, I will refactor this to pass in a RecordConverter that and will pass in TimestampedByteStore#convertValueToNewFormat()

guozhangwang · 2019-01-28T04:10:54Z

+                    nextNoTimestamp = null;
+                    iterNoTimestamp.next();
+                } else {
+                    if (comparator.compare(nextNoTimestamp.key.get(), nextWithTimestamp.key.get()) <= 0) {


if (comparator.compare(nextNoTimestamp.key.get(), nextWithTimestamp.key.get()) == 0) we need to advance on both ends while only returning the one from with-timestamp iterator, otherwise we may get duplicates returned.

This should never happen, because each key should be stored only once. Either in old or new CF. Or do I miss something?

I think it is still possible. Here's one scenario:

last checkpoint at offset 100; all writes goes to old CF.

continue writes to old CF til offset 110, but no checkpoint written yet.

non-graceful shutdown happens, and upon restarting new CF is used.

we start restoring from offset 100 to log-end-offset 110, to the new CF.

Now we ended with data of offsets 100-110 in both CFs.

In step (4), we also delete on old CF. Compare https://github.com/apache/kafka/pull/6149/files#diff-be43584f61033a47b5422775f5c5efdaR154

I see. Do we guarantee that concurrent IQ will not see duplicated results with the db-accessor updating logic as well? If yes, we can save this check.

That should be safe. Note, that IQ is "disabled" if we are not in RUNNING state.

guozhangwang · 2019-01-28T04:12:59Z

UPDATE: can we just modify RocksDbKeyValueBytesStoreSupplier to allow passing in a flag, based on which we will construct RocksDB v.s. TRocksDB?

guozhangwang · 2019-01-28T04:15:35Z

+        }
+
+        private KeyValue<Bytes, byte[]> getKeyValueNoTimestamp() {
+            return new KeyValue<>(new Bytes(iterNoTimestamp.key()), getValueWithUnknownTimestamp(iterNoTimestamp.value()));


I'd suggest one optimization here, in order to save array-copying every time while we scan over the old CF: we keep the original key and value bytes for no-timestamp iterator, and only when we've already decided to assign from this iterator to next then we will call getValueWithUnknownTimestamp to do the bytes array copying.

guozhangwang · 2019-01-28T04:25:22Z

+
+
+        // iterating should not migrate any data, but return all key over both CF (plus surrogate timestamps for old CF)
+        final KeyValueIterator<Bytes, byte[]> it = rocksDBStore.all();


Could we also test for range(from, to) queries?

vvcephei

Thanks, @mjsax !

I'm still looking over it, but I wanted to submit one batch of feedbacks for you.

-John

vvcephei · 2019-01-29T16:49:52Z

+    private ColumnFamilyHandle noTimestampColumnFamily;
+    private ColumnFamilyHandle withTimestampColumnFamily;
+
+    RocksDBTimestampedStore(final String name) {


👍 It looks like the two-arg constructor is unused.

vvcephei · 2019-01-29T16:52:49Z

+                     final ColumnFamilyOptions columnFamilyOptions) {
+        final List<ColumnFamilyDescriptor> columnFamilyDescriptors = asList(
+            new ColumnFamilyDescriptor(RocksDB.DEFAULT_COLUMN_FAMILY, columnFamilyOptions),
+            new ColumnFamilyDescriptor("keyValueWithTimestamp".getBytes(StandardCharsets.UTF_8), columnFamilyOptions));


Did @guozhangwang suggest to rename this DF to 2.2? I actually think the descriptive name might be better. It seems like it'll be less work in the long run to remember what exactly is different about the different CFs.

He did suggest but there was no final agreement. I personally don't care too much.

what about naming keyValueWithTimestamp_2.2?

I don't feel strong about my suggestion either :) Will leave it to anyone who has a strong feeling here.

vvcephei · 2019-01-29T23:16:46Z

+        }
+    }
+
+    private class DualColumnFamilyAccessor implements RocksDBAccessor {


Personally, I like the existing design. The DualColumnFamilyAccessor has to do a lot of extra checking that isn't necessary if there's just one cf to deal with. If we collapse them into one class with a flag, we pay for it with a lot of branching.

One thing I did find confusing was reasoning about the fact that the Dual accessor is embedded in this (Timestamped) class, and the Single accessor is embedded in the parent (non-timestamped) class. But, we're using it as an accessor for this (the child) class. This seems unnecessarily convoluted, and it's a little hard to see if it's actually ok, or just coincidentally ok, since the parent and child APIs are only semantically, rather than actually, different.

It seems simpler to understand if we pull both accessors out into separate classes that take db, name, options, etc as constructor arguments, rather than closing over protected state.

bbejeck

Thanks for the update, Overall looks good to me, I want to take another pass over the unit test later this evening.

bbejeck · 2019-01-29T23:44:20Z

+                     final ColumnFamilyOptions columnFamilyOptions) {
+        final List<ColumnFamilyDescriptor> columnFamilyDescriptors = asList(
+            new ColumnFamilyDescriptor(RocksDB.DEFAULT_COLUMN_FAMILY, columnFamilyOptions),
+            new ColumnFamilyDescriptor("keyValueWithTimestamp".getBytes(StandardCharsets.UTF_8), columnFamilyOptions));


what about naming keyValueWithTimestamp_2.2?

bbejeck · 2019-01-29T23:47:34Z

+        }
+    }
+
+    private class DualColumnFamilyAccessor implements RocksDBAccessor {


+1 from me as well for putting both accessors into separate classes

bbejeck · 2019-01-30T01:30:44Z

+public class RocksDBTimestampedStoreTest extends RocksDBStoreTest {
+    final String unknownTimestampString = new String(new LongSerializer().serialize(null, ConsumerRecord.NO_TIMESTAMP));
+
+    String getTimestampPrefix() {


nit: this is unused

bbejeck

One minor nit otherwise LGTM.

bbejeck · 2019-01-30T06:36:51Z

+        // approx: 4 entries on old CF, 1 in new CF
+        assertThat(rocksDBStore.approximateNumEntries(), is(5L));
+
+        // should add new key10 to new CF


nit: comment states key10 added, but actual key put is key8

vvcephei · 2019-01-30T06:40:37Z

+            }
+
+            if (nextNoTimestamp == null) {
+                if (nextWithTimestamp == null) {


should both iterators also be reporting !isValid here as well? I'm finding he rocksdb iterator api a little confusing...

I guess if we never allow a null key into the store, then this is an effective way to check for the end of the iteration.

null keys are not allowed -- that is the assumption in this check. We can add the !isValid as safe guard if you want.

I don't feel strongly about it. If we enforce the "no null keys" invariant, then they are equivalent.

It seems mildly confusing that we essentially have two different methods of determining when the iterator has run out of data. I leave it up to you.

\cc @guozhangwang WDYT?

Ack. Updating this.

vvcephei

Hey @mjsax ,

There are a few minor discussions, but overall, it looks good to me.

Thanks!
-John

guozhangwang · 2019-01-30T17:33:39Z

LGTM. @mjsax please feel free to merge after @vvcephei 's comments.

mjsax · 2019-01-30T17:47:15Z

Updated this. Will merge when Jenkins is green.

mjsax · 2019-01-30T20:41:45Z

https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/19162/

java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.TimeoutException: Aborted due to timeout.
	at org.apache.kafka.common.internals.KafkaFutureImpl.wrapAndThrow(KafkaFutureImpl.java:45)
	at org.apache.kafka.common.internals.KafkaFutureImpl.access$000(KafkaFutureImpl.java:32)
	at org.apache.kafka.common.internals.KafkaFutureImpl$SingleWaiter.await(KafkaFutureImpl.java:89)
	at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:260)
	at kafka.api.AdminClientIntegrationTest.testElectPreferredLeaders(AdminClientIntegrationTest.scala:1273)

Retest this please.

* AK/trunk: fix typo (apache#5150) MINOR: Reduce replica.fetch.backoff.ms in ReassignPartitionsClusterTest (apache#5887) KAFKA-7766: Fail fast PR builds (apache#6059) KAFKA-7798: Expose embedded clientIds (apache#6107) KAFKA-7641; Introduce "group.max.size" config to limit group sizes (apache#6163) KAFKA-7433; Introduce broker options in TopicCommand to use AdminClient (KIP-377) MINOR: Fix some field definitions for ListOffsetReponse (apache#6214) KAFKA-7873; Always seek to beginning in KafkaBasedLog (apache#6203) KAFKA-7719: Improve fairness in SocketServer processors (KIP-402) (apache#6022) MINOR: fix checkstyle suppressions for generated RPC code to work on Windows KAFKA-7859: Use automatic RPC generation in LeaveGroups (apache#6188) KAFKA-7652: Part II; Add single-point query for SessionStore and use for flushing / getter (apache#6161) KAFKA-3522: Add RocksDBTimestampedStore (apache#6149) KAFKA-3522: Replace RecordConverter with TimestampedBytesStore (apache#6204)

Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Guozhang Wang <guozhang@confluent.io>

mjsax added the streams label Jan 15, 2019

mjsax commented Jan 15, 2019

View reviewed changes

mjsax commented Jan 17, 2019

View reviewed changes

bbejeck reviewed Jan 17, 2019

View reviewed changes

mjsax commented Jan 17, 2019

View reviewed changes

mjsax force-pushed the kafka-3522-rocksdb-format-rocksdbwithtimestampstore branch from 2102fef to 391caa9 Compare January 17, 2019 22:21

bbejeck reviewed Jan 17, 2019

View reviewed changes

mjsax changed the title ~~KAFKA-3522: Add RocksDBWithTimestampsStore~~ KAFKA-3522: Add RocksDBTimestampedStore Jan 18, 2019

mjsax commented Jan 20, 2019

View reviewed changes

bbejeck reviewed Jan 22, 2019

View reviewed changes

vvcephei reviewed Jan 22, 2019

View reviewed changes

mjsax force-pushed the kafka-3522-rocksdb-format-rocksdbwithtimestampstore branch from f9a5a3b to 110cbf1 Compare January 23, 2019 00:42

mjsax mentioned this pull request Jan 23, 2019

KAFKA-3522: Add RocksDBTimestampedSegmentedBytesStore #6186

Merged

guozhangwang reviewed Jan 23, 2019

View reviewed changes

mjsax force-pushed the kafka-3522-rocksdb-format-rocksdbwithtimestampstore branch from 300dcc2 to 8d74b82 Compare January 25, 2019 07:08

KAFKA-3522: Add RocksDBWithTimestampsStore

c854ee8

mjsax force-pushed the kafka-3522-rocksdb-format-rocksdbwithtimestampstore branch from 8d74b82 to c854ee8 Compare January 25, 2019 07:16

guozhangwang reviewed Jan 28, 2019

View reviewed changes

Github comments

73253b8

Github comments

66eaa42

vvcephei reviewed Jan 29, 2019

View reviewed changes

Remove unused constructors

bf1d53d

bbejeck reviewed Jan 30, 2019

View reviewed changes

bbejeck approved these changes Jan 30, 2019

View reviewed changes

vvcephei reviewed Jan 30, 2019

View reviewed changes

vvcephei approved these changes Jan 30, 2019

View reviewed changes

Github comments

ff685da

Github comments

88e573c

mjsax merged commit 73565b7 into apache:trunk Jan 31, 2019

mjsax deleted the kafka-3522-rocksdb-format-rocksdbwithtimestampstore branch January 31, 2019 00:13

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019

KAFKA-3522: Add RocksDBTimestampedStore (apache#6149)

d9308b6

Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Guozhang Wang <guozhang@confluent.io>

mjsax added the kip Requires or implements a KIP label Jun 12, 2020



		// iterating should not migrate any data, but return all key over both CF (plus surrogate timestamps for old CF)
		final KeyValueIterator<Bytes, byte[]> it = rocksDBStore.all();

Conversation

mjsax commented Jan 15, 2019

Uh oh!

mjsax commented Jan 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 17, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment