You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/08/24 04:26:10 UTC
[GitHub] [kafka] guozhangwang opened a new pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
guozhangwang opened a new pull request #11252:
URL: https://github.com/apache/kafka/pull/11252
This is an alternative approach in parallel to #11235. After several unsuccessful trials to improve its efficiency i've come up with a "slightly" larger approach, which is to use a kv-store instead as the shared store, which would store the value as list<v>. The benefits of this approach are:
1) Only serde once that compose <timestamp, byte, key>, at the outer metered stores, with less byte array copies.
2) Deletes are straight-forward with no scan reads, just a single call to delete all duplicated <timestamp, byte, key> values.
3) Using a KV store has less space amplification than a segmented window store.
The cons though:
1) Each put call would be a get-then-write to append to the list; also we would spend a few more bytes to store the list (most likely a singleton list, and hence just 4 more bytes).
2) It's more complicated definitely.. :)
The main idea is that since the shared store is actively GC'ed by the expiration logic, not based on time retention, and since that the key format is in <timestamp, byte, key>, the range expiration query is quite efficient as well.
This is not a final PR as you can see I had many quick-hacks on serdes etc, but just to illustrate the idea. I plan to run the benchmarks to see how it behaves compare with the other and if people agree with this approach, I will refine it to be cleaner.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r708758100
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +210,39 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
- // Delete the key from the outer window store now it is emitted
- store.put(record.key.key(), null, record.key.window().start());
+ // blind-delete the key from the outer window store now it is emitted;
+ // we may delete some values of the same key which has not been iterated yet,
+ // but since the iterator would still return that key this is fine.
+ // we do not use the delete() call since that would incur an extra get
+ store.put(timestampedKeyAndJoinSide, null);
Review comment:
If we do the blind delete of the full list, is there not potential data loss scenario? Assume you have a list if 2+ left-join candidates in the list. We process the first one, and do the delete, and we crash before processing the others in the list. After restart, we might never emit the left-join result for those record?
Thus, would we need to do the delete only after we have exhausted the list, ie, move from key-timestamp to a different key or timestamp ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#issuecomment-911108274
Benchmark results:
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1secouterjoin-3-5-3-1-0-GUOZHANG-SSJOIN-guozhang-ssjoin-kv/
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1secleftjoin-3-5-3-1-0-GUOZHANG-SSJOIN-guozhang-ssjoin-kv/
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1minouterjoin-3-5-3-1-0-GUOZHANG-SSJOIN-guozhang-ssjoin-kv/
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1minleftjoin-3-5-3-1-0-GUOZHANG-SSJOIN-guozhang-ssjoin-kv/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710444534
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +211,51 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
+ TimestampedKeyAndJoinSide<K> prevKey = null;
+
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
+
+ if (prevKey != null && !prevKey.equals(timestampedKeyAndJoinSide)) {
Review comment:
Could we simply this to:
```
if (!timestampedKeyAndJoinSide.equals(prevKey)) {
```
Flipping the order should avoid a NPE because `timestampedKeyAndJoinSide != null` and `equals()` should handle `null` as input gracefully?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] spena commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
spena commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r703675892
##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedTimestampedKeyAndJoinSideSerializerTest.java
##########
@@ -24,49 +24,49 @@
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.Assert.assertThrows;
-public class KeyAndJoinSideSerializerTest {
+public class TimestampedTimestampedKeyAndJoinSideSerializerTest {
Review comment:
The class starts with repeated timestamped -> `TimestampedTimestamped...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#issuecomment-904313218
ping @mjsax @spena
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r701252846
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowStore.java
##########
@@ -33,172 +39,112 @@
* For key range queries, like fetch(key, fromTime, toTime), use the {@link RocksDBWindowStore}
* which uses the {@link WindowKeySchema} to serialize the record bytes for efficient key queries.
*/
+@SuppressWarnings("unchecked")
public class RocksDBTimeOrderedWindowStore
Review comment:
Sounds good, will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r707812546
##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/ListValueStoreTest.java
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.metrics.Metrics;
+import org.apache.kafka.common.serialization.Serde;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.utils.LogContext;
+import org.apache.kafka.common.utils.Time;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.processor.StateStoreContext;
+import org.apache.kafka.streams.processor.internals.MockStreamsMetrics;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.test.InternalMockProcessorContext;
+import org.apache.kafka.test.MockRecordCollector;
+import org.apache.kafka.test.TestUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+
+import static java.util.Arrays.asList;
+import static org.apache.kafka.test.StreamsTestUtils.toList;
+import static org.junit.Assert.assertEquals;
+
+public class ListValueStoreTest {
+ private static final String STORE_NAME = "rocksDB list value store";
+
+ MockRecordCollector recordCollector;
+ KeyValueStore<Integer, String> listStore;
+ InternalMockProcessorContext<Integer, String> context;
+
+ final File baseDir = TestUtils.tempDirectory("test");
+
+ @Before
+ public void setup() {
+ listStore = buildStore(Serdes.Integer(), Serdes.String());
+
+ recordCollector = new MockRecordCollector();
+ context = new InternalMockProcessorContext<>(
+ baseDir,
+ Serdes.String(),
+ Serdes.Integer(),
+ recordCollector,
+ new ThreadCache(
+ new LogContext("testCache"),
+ 0,
+ new MockStreamsMetrics(new Metrics())));
+ context.setTime(1L);
+
+ listStore.init((StateStoreContext) context, listStore);
+ }
+
+ @After
+ public void after() {
+ listStore.close();
+ }
+
+ <K, V> KeyValueStore<K, V> buildStore(final Serde<K> keySerde,
+ final Serde<V> valueSerde) {
+ return new ListValueStoreBuilder<>(
+ new RocksDbKeyValueBytesStoreSupplier(STORE_NAME, false),
+ keySerde,
+ valueSerde,
+ Time.SYSTEM)
+ .build();
+ }
+
+ @Test
+ public void shouldGetAll() {
+ listStore.put(0, "zero");
+ // should retain duplicates
+ listStore.put(0, "zero again");
+ listStore.put(1, "one");
+ listStore.put(2, "two");
+
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> zeroAgain = KeyValue.pair(0, "zero again");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+ final KeyValue<Integer, String> two = KeyValue.pair(2, "two");
+
+ assertEquals(
+ asList(zero, zeroAgain, one, two),
+ toList(listStore.all())
+ );
+ }
+
+ @Test
+ public void shouldGetAllNonDeletedRecords() {
+ // Add some records
+ listStore.put(0, "zero");
+ listStore.put(1, "one");
+ listStore.put(1, "one again");
+ listStore.put(2, "two");
+ listStore.put(3, "three");
+ listStore.put(4, "four");
+
+ // Delete some records
+ listStore.put(1, null);
+ listStore.put(3, null);
+
+ // Only non-deleted records should appear in the all() iterator
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> two = KeyValue.pair(2, "two");
+ final KeyValue<Integer, String> four = KeyValue.pair(4, "four");
+
+ assertEquals(
+ asList(zero, two, four),
+ toList(listStore.all())
+ );
+ }
+
+ @Test
+ public void shouldGetAllReturnTimestampOrderedRecords() {
+ // Add some records in different order
+ listStore.put(4, "four");
+ listStore.put(0, "zero");
+ listStore.put(2, "two1");
+ listStore.put(3, "three");
+ listStore.put(1, "one");
+
+ // Add duplicates
+ listStore.put(2, "two2");
+
+ // Only non-deleted records should appear in the all() iterator
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+ final KeyValue<Integer, String> two1 = KeyValue.pair(2, "two1");
+ final KeyValue<Integer, String> two2 = KeyValue.pair(2, "two2");
+ final KeyValue<Integer, String> three = KeyValue.pair(3, "three");
+ final KeyValue<Integer, String> four = KeyValue.pair(4, "four");
+
+ assertEquals(
+ asList(zero, one, two1, two2, three, four),
+ toList(listStore.all())
+ );
+ }
+
+ @Test
+ public void shouldEarlyClosedIteratorStillGetAllRecords() {
+ listStore.put(0, "zero");
+ listStore.put(1, "one");
+
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+
+ final KeyValueIterator<Integer, String> it = listStore.all();
+ assertEquals(zero, it.next());
+ it.close();
+
+ // A new all() iterator after a previous all() iterator was closed should return all elements.
Review comment:
Not sure if I understand here... in the caller processor, we do not rely on the range to automatically delete values while iterating, but still explicitly call `put(k, null)` to do the delete, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r701251294
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/KeyAndJoinSideDeserializer.java
##########
@@ -48,15 +50,22 @@ public void configure(final Map<String, ?> configs, final boolean isKey) {
@Override
public KeyAndJoinSide<K> deserialize(final String topic, final byte[] data) {
- final boolean bool = data[0] == 1;
+ final boolean bool = data[8] == 1;
Review comment:
Ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710482150
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +211,51 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
+ TimestampedKeyAndJoinSide<K> prevKey = null;
+
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
+
+ if (prevKey != null && !prevKey.equals(timestampedKeyAndJoinSide)) {
Review comment:
if `prevKey == null` then the above would actually pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710457259
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
Review comment:
Ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r701251517
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/KeyAndJoinSideSerializer.java
##########
@@ -55,9 +57,11 @@ public void configure(final Map<String, ?> configs, final boolean isKey) {
public byte[] serialize(final String topic, final KeyAndJoinSide<K> data) {
final byte boolByte = (byte) (data.isLeftSide() ? 1 : 0);
final byte[] keyBytes = keySerializer.serialize(topic, data.getKey());
+ final byte[] timestampBytes = timestampSerializer.serialize(topic, data.getTimestamp());
return ByteBuffer
- .allocate(keyBytes.length + 1)
+ .allocate(8 + keyBytes.length + 1)
Review comment:
Ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710448218
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
+ }
+
+ @Override
+ public byte[] get(final Bytes key) {
+ return wrapped().get(key);
Review comment:
Ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r707817999
##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/ListValueStoreTest.java
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.metrics.Metrics;
+import org.apache.kafka.common.serialization.Serde;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.utils.LogContext;
+import org.apache.kafka.common.utils.Time;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.processor.StateStoreContext;
+import org.apache.kafka.streams.processor.internals.MockStreamsMetrics;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.test.InternalMockProcessorContext;
+import org.apache.kafka.test.MockRecordCollector;
+import org.apache.kafka.test.TestUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+
+import static java.util.Arrays.asList;
+import static org.apache.kafka.test.StreamsTestUtils.toList;
+import static org.junit.Assert.assertEquals;
+
+public class ListValueStoreTest {
+ private static final String STORE_NAME = "rocksDB list value store";
+
+ MockRecordCollector recordCollector;
+ KeyValueStore<Integer, String> listStore;
+ InternalMockProcessorContext<Integer, String> context;
+
+ final File baseDir = TestUtils.tempDirectory("test");
+
+ @Before
+ public void setup() {
+ listStore = buildStore(Serdes.Integer(), Serdes.String());
+
+ recordCollector = new MockRecordCollector();
+ context = new InternalMockProcessorContext<>(
+ baseDir,
+ Serdes.String(),
+ Serdes.Integer(),
+ recordCollector,
+ new ThreadCache(
+ new LogContext("testCache"),
+ 0,
+ new MockStreamsMetrics(new Metrics())));
+ context.setTime(1L);
+
+ listStore.init((StateStoreContext) context, listStore);
+ }
+
+ @After
+ public void after() {
+ listStore.close();
+ }
+
+ <K, V> KeyValueStore<K, V> buildStore(final Serde<K> keySerde,
+ final Serde<V> valueSerde) {
+ return new ListValueStoreBuilder<>(
+ new RocksDbKeyValueBytesStoreSupplier(STORE_NAME, false),
+ keySerde,
+ valueSerde,
+ Time.SYSTEM)
+ .build();
+ }
+
+ @Test
+ public void shouldGetAll() {
+ listStore.put(0, "zero");
+ // should retain duplicates
+ listStore.put(0, "zero again");
+ listStore.put(1, "one");
+ listStore.put(2, "two");
+
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> zeroAgain = KeyValue.pair(0, "zero again");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+ final KeyValue<Integer, String> two = KeyValue.pair(2, "two");
+
+ assertEquals(
+ asList(zero, zeroAgain, one, two),
+ toList(listStore.all())
Review comment:
Potential memory leak? `toList` does not close the iterator -- should we update `toList` ? Or make sure we close the iterator in the using try-with-resources? (similar below -- not sure about existing tests using `toList`? -- might be worth to double check)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang merged pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang merged pull request #11252:
URL: https://github.com/apache/kafka/pull/11252
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r705883780
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImplJoin.java
##########
@@ -280,85 +279,51 @@ private void assertUniqueStoreNames(final WindowBytesStoreSupplier supplier,
}
}
- @SuppressWarnings("unchecked")
- private <K, V1, V2> StoreBuilder<WindowStore<KeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>> sharedOuterJoinWindowStoreBuilder(final JoinWindows windows,
- final StreamJoinedInternal<K, V1, V2> streamJoinedInternal,
- final String joinThisGeneratedName) {
+ private <K, V1, V2> StoreBuilder<KeyValueStore<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>> sharedOuterJoinWindowStoreBuilder(final JoinWindows windows,
+ final StreamJoinedInternal<K, V1, V2> streamJoinedInternal,
+ final String joinThisGeneratedName) {
final boolean persistent = streamJoinedInternal.thisStoreSupplier() == null || streamJoinedInternal.thisStoreSupplier().get().persistent();
- final String storeName = buildOuterJoinWindowStoreName(streamJoinedInternal, joinThisGeneratedName);
-
- final KeyAndJoinSideSerde keyAndJoinSideSerde = new KeyAndJoinSideSerde<>(streamJoinedInternal.keySerde());
- final LeftOrRightValueSerde leftOrRightValueSerde = new LeftOrRightValueSerde(streamJoinedInternal.valueSerde(), streamJoinedInternal.otherValueSerde());
-
- final StoreBuilder<WindowStore<KeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>> builder;
- if (persistent) {
- builder = new TimeOrderedWindowStoreBuilder<KeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>(
- persistentTimeOrderedWindowStore(
- storeName + "-store",
- Duration.ofMillis(windows.size() + windows.gracePeriodMs()),
- Duration.ofMillis(windows.size())
- ),
- keyAndJoinSideSerde,
- leftOrRightValueSerde,
- Time.SYSTEM
- );
- } else {
- builder = Stores.windowStoreBuilder(
- Stores.inMemoryWindowStore(
- storeName + "-store",
- Duration.ofMillis(windows.size() + windows.gracePeriodMs()),
- Duration.ofMillis(windows.size()),
- false
- ),
- keyAndJoinSideSerde,
- leftOrRightValueSerde
- );
- }
-
- if (streamJoinedInternal.loggingEnabled()) {
- builder.withLoggingEnabled(streamJoinedInternal.logConfig());
- } else {
- builder.withLoggingDisabled();
- }
-
- return builder;
- }
+ final String storeName = buildOuterJoinWindowStoreName(streamJoinedInternal, joinThisGeneratedName) + "-store";
Review comment:
Did we miss to add the `"-store"` suffix originally?
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimeOrderedKeyValueBytesStore.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.state.KeyValueStore;
+
+public class ChangeLoggingTimeOrderedKeyValueBytesStore extends ChangeLoggingKeyValueBytesStore {
Review comment:
Should this class be called `ChangeLoggingListValueByteStore` ?
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +208,37 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
Review comment:
Why do we not need to cast here any longer, but move the cast to the `else` branch ?
##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/ListValueStoreTest.java
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.metrics.Metrics;
+import org.apache.kafka.common.serialization.Serde;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.utils.LogContext;
+import org.apache.kafka.common.utils.Time;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.processor.StateStoreContext;
+import org.apache.kafka.streams.processor.internals.MockStreamsMetrics;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.test.InternalMockProcessorContext;
+import org.apache.kafka.test.MockRecordCollector;
+import org.apache.kafka.test.TestUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+
+import static java.util.Arrays.asList;
+import static org.apache.kafka.test.StreamsTestUtils.toList;
+import static org.junit.Assert.assertEquals;
+
+public class ListValueStoreTest {
+ private static final String STORE_NAME = "rocksDB list value store";
+
+ MockRecordCollector recordCollector;
+ KeyValueStore<Integer, String> listStore;
+ InternalMockProcessorContext<Integer, String> context;
+
+ final File baseDir = TestUtils.tempDirectory("test");
+
+ @Before
+ public void setup() {
+ listStore = buildStore(Serdes.Integer(), Serdes.String());
+
+ recordCollector = new MockRecordCollector();
+ context = new InternalMockProcessorContext<>(
+ baseDir,
+ Serdes.String(),
+ Serdes.Integer(),
+ recordCollector,
+ new ThreadCache(
+ new LogContext("testCache"),
+ 0,
+ new MockStreamsMetrics(new Metrics())));
+ context.setTime(1L);
+
+ listStore.init((StateStoreContext) context, listStore);
+ }
+
+ @After
+ public void after() {
+ listStore.close();
+ }
+
+ <K, V> KeyValueStore<K, V> buildStore(final Serde<K> keySerde,
+ final Serde<V> valueSerde) {
+ return new ListValueStoreBuilder<>(
+ new RocksDbKeyValueBytesStoreSupplier(STORE_NAME, false),
+ keySerde,
+ valueSerde,
+ Time.SYSTEM)
+ .build();
+ }
+
+ @Test
+ public void shouldGetAll() {
+ listStore.put(0, "zero");
+ // should retain duplicates
+ listStore.put(0, "zero again");
+ listStore.put(1, "one");
+ listStore.put(2, "two");
+
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> zeroAgain = KeyValue.pair(0, "zero again");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+ final KeyValue<Integer, String> two = KeyValue.pair(2, "two");
+
+ assertEquals(
+ asList(zero, zeroAgain, one, two),
+ toList(listStore.all())
+ );
+ }
+
+ @Test
+ public void shouldGetAllNonDeletedRecords() {
+ // Add some records
+ listStore.put(0, "zero");
+ listStore.put(1, "one");
+ listStore.put(1, "one again");
+ listStore.put(2, "two");
+ listStore.put(3, "three");
+ listStore.put(4, "four");
+
+ // Delete some records
+ listStore.put(1, null);
+ listStore.put(3, null);
+
+ // Only non-deleted records should appear in the all() iterator
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> two = KeyValue.pair(2, "two");
+ final KeyValue<Integer, String> four = KeyValue.pair(4, "four");
+
+ assertEquals(
+ asList(zero, two, four),
+ toList(listStore.all())
+ );
+ }
+
+ @Test
+ public void shouldGetAllReturnTimestampOrderedRecords() {
+ // Add some records in different order
+ listStore.put(4, "four");
+ listStore.put(0, "zero");
+ listStore.put(2, "two1");
+ listStore.put(3, "three");
+ listStore.put(1, "one");
+
+ // Add duplicates
+ listStore.put(2, "two2");
+
+ // Only non-deleted records should appear in the all() iterator
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+ final KeyValue<Integer, String> two1 = KeyValue.pair(2, "two1");
+ final KeyValue<Integer, String> two2 = KeyValue.pair(2, "two2");
+ final KeyValue<Integer, String> three = KeyValue.pair(3, "three");
+ final KeyValue<Integer, String> four = KeyValue.pair(4, "four");
+
+ assertEquals(
+ asList(zero, one, two1, two2, three, four),
+ toList(listStore.all())
+ );
+ }
+
+ @Test
+ public void shouldEarlyClosedIteratorStillGetAllRecords() {
+ listStore.put(0, "zero");
+ listStore.put(1, "one");
+
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+
+ final KeyValueIterator<Integer, String> it = listStore.all();
+ assertEquals(zero, it.next());
+ it.close();
+
+ // A new all() iterator after a previous all() iterator was closed should return all elements.
Review comment:
Why is this a requirement?
Should we instead on call `valueList.removeAll()` on `iter.close()`? -- In this test, if there would be 3 keys, the third key won't be returned either after `close()` to why allow to keep iteration on the current key ?
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
Review comment:
Can we add: `A put(k,null) will still delete the key, ie, the full list of all values of this key.`
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] value) {
+ // if the value is null we can skip the get and blind delete
+ if (value == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(value)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(value);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] value) {
+ throw new UnsupportedOperationException("putIfAbsent not supported");
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
Review comment:
I am fine with this, but I don't think I agree to the reasoning. `delete(k)` is just syntactic sugar for `put(k, null)`, and `get()` also returns the full list of all values... Thus, this store has some inconsistencies to a regular kv-store all over the place anyway... (what is still ok, as it's an internal store).
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimeOrderedKeyValueBytesStore.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.state.KeyValueStore;
+
+public class ChangeLoggingTimeOrderedKeyValueBytesStore extends ChangeLoggingKeyValueBytesStore {
+
+ ChangeLoggingTimeOrderedKeyValueBytesStore(final KeyValueStore<Bytes, byte[]> inner) {
+ super(inner);
+ }
+
+ @Override
+ public void put(final Bytes key,
+ final byte[] value) {
+ wrapped().put(key, value);
+ // we need to log the new value, which is different from the put value;
Review comment:
Can we update this comment to be clearer:
```
// the provided new value will be added to the list in the inner put()
// we need to log the full new list and thus call get() on the inner store below
// if the value is a tombstone, we delete the whole list and thus can save the get call
```
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] value) {
+ // if the value is null we can skip the get and blind delete
+ if (value == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(value)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(value);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] value) {
+ throw new UnsupportedOperationException("putIfAbsent not supported");
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
+ }
+
+ @Override
+ public byte[] get(final Bytes key) {
+ return wrapped().get(key);
+ }
+
+ @Override
+ public KeyValueIterator<Bytes, byte[]> range(final Bytes from, final Bytes to) {
+ throw new UnsupportedOperationException("range not supported");
+ }
+
+ @Override
+ public KeyValueIterator<Bytes, byte[]> all() {
+ return new WrappedStoreIterator(wrapped().all());
+ }
+
+ @Override
+ public long approximateNumEntries() {
+ return wrapped().approximateNumEntries();
+ }
+
+ private static class WrappedStoreIterator extends AbstractIterator<KeyValue<Bytes, byte[]>>
Review comment:
Should we call this class `ValueListIterator` (or similar) -- "Wrapped" is not very descriptive.
##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedTimestampedKeyAndJoinSideSerializerTest.java
##########
@@ -24,49 +24,49 @@
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.Assert.assertThrows;
-public class KeyAndJoinSideSerializerTest {
+public class TimestampedTimestampedKeyAndJoinSideSerializerTest {
Review comment:
This comment was not addressed yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r711184034
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +211,51 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
+ TimestampedKeyAndJoinSide<K> prevKey = null;
+
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
+
+ if (prevKey != null && !prevKey.equals(timestampedKeyAndJoinSide)) {
Review comment:
Yes, so would `!timestampedKeyAndJoinSide.equals(prevKey)` :) (Not important though)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r708758796
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] value) {
+ // if the value is null we can skip the get and blind delete
+ if (value == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(value)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(value);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] value) {
+ throw new UnsupportedOperationException("putIfAbsent not supported");
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
Review comment:
Seems you did not disable `get()` yet?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710453587
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
Review comment:
In the ChangeloggingListValueStore we always return null (added a TODO as well explaining this is a hack).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#issuecomment-921336698
Merged to trunk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710446435
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
Review comment:
There seems to be some duplicate code in `put()` and `putIfAbsent` would it be worth to refactor and extract some private helpers?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710448218
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
+ }
+
+ @Override
+ public byte[] get(final Bytes key) {
+ return wrapped().get(key);
Review comment:
Actually we still need it for `put()` in `ChangeloggingListValueStore`, to log the new (appended) list value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#issuecomment-921327842
New benchmark results are out, and still looking reasonable:
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1secouterjoin-3-5-3-1-0-SNAPSHOT-SSJOIN-guozhang-ssjoin-2/
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1secleftjoin-3-5-3-1-0-SNAPSHOT-SSJOIN-guozhang-ssjoin-2/
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1minleftjoin-3-5-3-1-0-SNAPSHOT-SSJOIN-guozhang-ssjoin-2/
* http://kstreams-benchmark-results.s3-website-us-west-2.amazonaws.com/experiments/streamstream1minouterjoin-3-5-3-1-0-SNAPSHOT-SSJOIN-guozhang-ssjoin-3/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710446822
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
+ }
+
+ @Override
+ public byte[] get(final Bytes key) {
+ return wrapped().get(key);
Review comment:
As above--we don't call `get()` anywhere, but is it safe or should be disable it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r711238521
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
Review comment:
Ah yes, you're right. Hmm, I guess I'd have to duplicate the logic of putifabsent in both layers then.
I will file a follow-up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710445656
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
Review comment:
Is it save to return `oldValue`? It would be a serialized `List` but the outer metered store expect a plain value? As we don't really check the result of `putIfAbsent` we could return `null`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r701252642
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -146,7 +146,7 @@ public void process(final K key, final V1 value) {
outerJoinWindowStore.ifPresent(store -> {
// Delete the joined record from the non-joined outer window store
- store.put(KeyAndJoinSide.make(!isLeftSide, key), null, otherRecordTimestamp);
+ store.put(KeyAndJoinSide.make(!isLeftSide, key, otherRecordTimestamp), null);
Review comment:
Same as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r711198066
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
Review comment:
The only caller of the `ListValueStore.get()` is the `ChangeloggingListValueStore`. So:
* If logging is enabled, then we need to make sure that the correct value is returned from `get()` in order to log.
* If logging is disabled, then there's no caller of that `get()` anyways so there's no concern of deserialization errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r707817999
##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/ListValueStoreTest.java
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.metrics.Metrics;
+import org.apache.kafka.common.serialization.Serde;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.utils.LogContext;
+import org.apache.kafka.common.utils.Time;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.processor.StateStoreContext;
+import org.apache.kafka.streams.processor.internals.MockStreamsMetrics;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.test.InternalMockProcessorContext;
+import org.apache.kafka.test.MockRecordCollector;
+import org.apache.kafka.test.TestUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+
+import static java.util.Arrays.asList;
+import static org.apache.kafka.test.StreamsTestUtils.toList;
+import static org.junit.Assert.assertEquals;
+
+public class ListValueStoreTest {
+ private static final String STORE_NAME = "rocksDB list value store";
+
+ MockRecordCollector recordCollector;
+ KeyValueStore<Integer, String> listStore;
+ InternalMockProcessorContext<Integer, String> context;
+
+ final File baseDir = TestUtils.tempDirectory("test");
+
+ @Before
+ public void setup() {
+ listStore = buildStore(Serdes.Integer(), Serdes.String());
+
+ recordCollector = new MockRecordCollector();
+ context = new InternalMockProcessorContext<>(
+ baseDir,
+ Serdes.String(),
+ Serdes.Integer(),
+ recordCollector,
+ new ThreadCache(
+ new LogContext("testCache"),
+ 0,
+ new MockStreamsMetrics(new Metrics())));
+ context.setTime(1L);
+
+ listStore.init((StateStoreContext) context, listStore);
+ }
+
+ @After
+ public void after() {
+ listStore.close();
+ }
+
+ <K, V> KeyValueStore<K, V> buildStore(final Serde<K> keySerde,
+ final Serde<V> valueSerde) {
+ return new ListValueStoreBuilder<>(
+ new RocksDbKeyValueBytesStoreSupplier(STORE_NAME, false),
+ keySerde,
+ valueSerde,
+ Time.SYSTEM)
+ .build();
+ }
+
+ @Test
+ public void shouldGetAll() {
+ listStore.put(0, "zero");
+ // should retain duplicates
+ listStore.put(0, "zero again");
+ listStore.put(1, "one");
+ listStore.put(2, "two");
+
+ final KeyValue<Integer, String> zero = KeyValue.pair(0, "zero");
+ final KeyValue<Integer, String> zeroAgain = KeyValue.pair(0, "zero again");
+ final KeyValue<Integer, String> one = KeyValue.pair(1, "one");
+ final KeyValue<Integer, String> two = KeyValue.pair(2, "two");
+
+ assertEquals(
+ asList(zero, zeroAgain, one, two),
+ toList(listStore.all())
Review comment:
Potential memory leak? `toList` does not close the iterator -- should we update `toList` ? Or make sure we close the iterator in the using try-with-resources?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r710453193
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +211,51 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
+ TimestampedKeyAndJoinSide<K> prevKey = null;
+
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
+
+ if (prevKey != null && !prevKey.equals(timestampedKeyAndJoinSide)) {
Review comment:
Ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r711184828
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
Review comment:
But users could disable changelogging, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r707810706
##########
File path: streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedTimestampedKeyAndJoinSideSerializerTest.java
##########
@@ -24,49 +24,49 @@
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.Assert.assertThrows;
-public class KeyAndJoinSideSerializerTest {
+public class TimestampedTimestampedKeyAndJoinSideSerializerTest {
Review comment:
Ack.
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
Review comment:
Ack.
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimeOrderedKeyValueBytesStore.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.state.KeyValueStore;
+
+public class ChangeLoggingTimeOrderedKeyValueBytesStore extends ChangeLoggingKeyValueBytesStore {
+
+ ChangeLoggingTimeOrderedKeyValueBytesStore(final KeyValueStore<Bytes, byte[]> inner) {
+ super(inner);
+ }
+
+ @Override
+ public void put(final Bytes key,
+ final byte[] value) {
+ wrapped().put(key, value);
+ // we need to log the new value, which is different from the put value;
Review comment:
Ack
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ChangeLoggingTimeOrderedKeyValueBytesStore.java
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.state.KeyValueStore;
+
+public class ChangeLoggingTimeOrderedKeyValueBytesStore extends ChangeLoggingKeyValueBytesStore {
Review comment:
Good point :)
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +208,37 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
Review comment:
We actually only need to case in the `else` branch, since there we are switching the left/right sides. I think it was originally added in the if branch just for symmetry.
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImplJoin.java
##########
@@ -280,85 +279,51 @@ private void assertUniqueStoreNames(final WindowBytesStoreSupplier supplier,
}
}
- @SuppressWarnings("unchecked")
- private <K, V1, V2> StoreBuilder<WindowStore<KeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>> sharedOuterJoinWindowStoreBuilder(final JoinWindows windows,
- final StreamJoinedInternal<K, V1, V2> streamJoinedInternal,
- final String joinThisGeneratedName) {
+ private <K, V1, V2> StoreBuilder<KeyValueStore<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>> sharedOuterJoinWindowStoreBuilder(final JoinWindows windows,
+ final StreamJoinedInternal<K, V1, V2> streamJoinedInternal,
+ final String joinThisGeneratedName) {
final boolean persistent = streamJoinedInternal.thisStoreSupplier() == null || streamJoinedInternal.thisStoreSupplier().get().persistent();
- final String storeName = buildOuterJoinWindowStoreName(streamJoinedInternal, joinThisGeneratedName);
-
- final KeyAndJoinSideSerde keyAndJoinSideSerde = new KeyAndJoinSideSerde<>(streamJoinedInternal.keySerde());
- final LeftOrRightValueSerde leftOrRightValueSerde = new LeftOrRightValueSerde(streamJoinedInternal.valueSerde(), streamJoinedInternal.otherValueSerde());
-
- final StoreBuilder<WindowStore<KeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>> builder;
- if (persistent) {
- builder = new TimeOrderedWindowStoreBuilder<KeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>(
- persistentTimeOrderedWindowStore(
- storeName + "-store",
- Duration.ofMillis(windows.size() + windows.gracePeriodMs()),
- Duration.ofMillis(windows.size())
- ),
- keyAndJoinSideSerde,
- leftOrRightValueSerde,
- Time.SYSTEM
- );
- } else {
- builder = Stores.windowStoreBuilder(
- Stores.inMemoryWindowStore(
- storeName + "-store",
- Duration.ofMillis(windows.size() + windows.gracePeriodMs()),
- Duration.ofMillis(windows.size()),
- false
- ),
- keyAndJoinSideSerde,
- leftOrRightValueSerde
- );
- }
-
- if (streamJoinedInternal.loggingEnabled()) {
- builder.withLoggingEnabled(streamJoinedInternal.logConfig());
- } else {
- builder.withLoggingDisabled();
- }
-
- return builder;
- }
+ final String storeName = buildOuterJoinWindowStoreName(streamJoinedInternal, joinThisGeneratedName) + "-store";
Review comment:
No, originally it was added later (see https://github.com/apache/kafka/pull/11252/files/22663b04258efbc7f770ec7c7feca46bb5310340#diff-9ce43046fdef1233ab762e728abd1d3d44d7c270b28dcf6b63aa31a93a30af07L308), but after the refactoring we only added it once now, before the `persistent` flag branch.
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] value) {
+ // if the value is null we can skip the get and blind delete
+ if (value == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(value)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(value);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] value) {
+ throw new UnsupportedOperationException("putIfAbsent not supported");
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
Review comment:
Yeah maybe I'd need to make that a bit clearer: the reason I disallow caller using this call intentionally is to avoid the caller mistakenly trying to interpret/deserialize the returned byte as a single value (also note that I did not implement the deserialization of the list value in get either). Since for stream-stream join we only need the `put` and `range` calls (using `put(null)` for delete too) only, not implementing the `get/delete` is fine --- if we want to support that all the way up to the caller in KStreamImplJoin, then we need a lot more work.
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] value) {
+ // if the value is null we can skip the get and blind delete
+ if (value == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(value)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(value);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] value) {
+ throw new UnsupportedOperationException("putIfAbsent not supported");
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
Review comment:
Note that in the `KStreamKStreamJoinProcessor` the shared store is still defined as `KeyValueStore<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>>`, hence calling its `get/delete` calls expecting to return a single `LeftOrRightValue<V1, V2>` would be doomed to fail.
Thinking about that once more, I think it's better do disable `get` as well for now.
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] value) {
+ // if the value is null we can skip the get and blind delete
+ if (value == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(value)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(value);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] value) {
+ throw new UnsupportedOperationException("putIfAbsent not supported");
+ }
+
+ @Override
+ public void putAll(final List<KeyValue<Bytes, byte[]>> entries) {
+ throw new UnsupportedOperationException("putAll not supported");
+ }
+
+ @Override
+ public byte[] delete(final Bytes key) {
+ // we intentionally disable delete calls since the returned bytes would
+ // represent a list, not a single value; we need to have a new API for delete if we do need it
+ throw new UnsupportedOperationException("delete not supported");
+ }
+
+ @Override
+ public byte[] get(final Bytes key) {
+ return wrapped().get(key);
+ }
+
+ @Override
+ public KeyValueIterator<Bytes, byte[]> range(final Bytes from, final Bytes to) {
+ throw new UnsupportedOperationException("range not supported");
+ }
+
+ @Override
+ public KeyValueIterator<Bytes, byte[]> all() {
+ return new WrappedStoreIterator(wrapped().all());
+ }
+
+ @Override
+ public long approximateNumEntries() {
+ return wrapped().approximateNumEntries();
+ }
+
+ private static class WrappedStoreIterator extends AbstractIterator<KeyValue<Bytes, byte[]>>
Review comment:
Ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r701251823
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredWindowStore.java
##########
@@ -63,6 +63,10 @@
private InternalProcessorContext context;
private TaskId taskId;
+ interface WindowedKeySerde {
+ Bytes serialize(final Bytes key, final long timestamp, final int seqnum);
+ }
+
Review comment:
Ack.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r701252490
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +208,36 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<KeyAndJoinSide<K>, LeftOrRightValue> it = store.all()) {
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<KeyAndJoinSide<K>, LeftOrRightValue> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final KeyAndJoinSide<K> keyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = keyAndJoinSide.getKey();
+ final long timestamp = keyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
// Delete the key from the outer window store now it is emitted
- store.put(record.key.key(), null, record.key.window().start());
+ store.put(keyAndJoinSide, null);
Review comment:
The `delete` call would incur an additional `get` in order to return the deleted value which is not needed, so I intentionally used `put(k, null)` to avoid that extra get. I can leave a comment above explaining this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r711197318
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +211,51 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> it = store.all()) {
+ TimestampedKeyAndJoinSide<K> prevKey = null;
+
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<TimestampedKeyAndJoinSide<K>, LeftOrRightValue<V1, V2>> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final TimestampedKeyAndJoinSide<K> timestampedKeyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = timestampedKeyAndJoinSide.getKey();
+ final long timestamp = timestampedKeyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
+
+ if (prevKey != null && !prevKey.equals(timestampedKeyAndJoinSide)) {
Review comment:
The argument is that if `prevKey == null`, then we do not want to trigger, but `!timestampedKeyAndJoinSide.equals(prevKey)` would pass and hence cause a trigger.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r711210235
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/ListValueStore.java
##########
@@ -0,0 +1,165 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.apache.kafka.common.utils.AbstractIterator;
+import org.apache.kafka.common.utils.Bytes;
+import org.apache.kafka.streams.KeyValue;
+import org.apache.kafka.streams.state.KeyValueIterator;
+import org.apache.kafka.streams.state.KeyValueStore;
+import org.apache.kafka.common.serialization.Serdes;
+import org.apache.kafka.common.serialization.Serde;
+
+/**
+ * A wrapper key-value store that serializes the record values bytes as a list.
+ * As a result put calls would be interpreted as a get-append-put to the underlying RocksDB store.
+ * A put(k,null) will still delete the key, ie, the full list of all values of this key.
+ * Range iterators would also flatten the value lists and return the values one-by-one.
+ *
+ * This store is used for cases where we do not want to de-duplicate values of the same keys but want to retain all such values.
+ */
+@SuppressWarnings("unchecked")
+public class ListValueStore
+ extends WrappedStateStore<KeyValueStore<Bytes, byte[]>, Bytes, byte[]>
+ implements KeyValueStore<Bytes, byte[]> {
+
+ static private final Serde<List<byte[]>> LIST_SERDE = Serdes.ListSerde(ArrayList.class, Serdes.ByteArray());
+
+ ListValueStore(final KeyValueStore<Bytes, byte[]> bytesStore) {
+ super(bytesStore);
+ }
+
+ @Override
+ public void put(final Bytes key, final byte[] addedValue) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue == null) {
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, Collections.singletonList(addedValue)));
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+ }
+
+ @Override
+ public byte[] putIfAbsent(final Bytes key, final byte[] addedValue) {
+ final byte[] oldValue = wrapped().get(key);
+
+ if (oldValue != null) {
+ // if the value is null we can skip the get and blind delete
+ if (addedValue == null) {
+ wrapped().put(key, null);
+ } else {
+ final List<byte[]> list = LIST_SERDE.deserializer().deserialize(null, oldValue);
+ list.add(addedValue);
+
+ wrapped().put(key, LIST_SERDE.serializer().serialize(null, list));
+ }
+ }
+
+ return oldValue;
Review comment:
Yes, but this is not `get()` but `pufIfAbsent()`, right? And `putIfAbsent()` is called by the join-Processor. If we have the logging layer and the logging layer returns `null` for `putIfAbsent()` the metered store is "guarded" -- but if logging is disabled, the metered store would crash when the join-processor calls `putIfAbsent(key,null)` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] spena commented on a change in pull request #11252: KAFKA-13216: Use a KV with list serde for the shared store
Posted by GitBox <gi...@apache.org>.
spena commented on a change in pull request #11252:
URL: https://github.com/apache/kafka/pull/11252#discussion_r701101588
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/KeyAndJoinSideDeserializer.java
##########
@@ -48,15 +50,22 @@ public void configure(final Map<String, ?> configs, final boolean isKey) {
@Override
public KeyAndJoinSide<K> deserialize(final String topic, final byte[] data) {
- final boolean bool = data[0] == 1;
+ final boolean bool = data[8] == 1;
Review comment:
It should be good to add a constant for the `8` number, so it is easily read. for instance, the `rawKey()` has `new byte[data.length - 9]`, which I assume is `len - TIMESTAMP - BOOL`.
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/KeyAndJoinSideSerializer.java
##########
@@ -55,9 +57,11 @@ public void configure(final Map<String, ?> configs, final boolean isKey) {
public byte[] serialize(final String topic, final KeyAndJoinSide<K> data) {
final byte boolByte = (byte) (data.isLeftSide() ? 1 : 0);
final byte[] keyBytes = keySerializer.serialize(topic, data.getKey());
+ final byte[] timestampBytes = timestampSerializer.serialize(topic, data.getTimestamp());
return ByteBuffer
- .allocate(keyBytes.length + 1)
+ .allocate(8 + keyBytes.length + 1)
Review comment:
Should the `8` be a constant variable or just `timestampBytes.length`?
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredWindowStore.java
##########
@@ -63,6 +63,10 @@
private InternalProcessorContext context;
private TaskId taskId;
+ interface WindowedKeySerde {
+ Bytes serialize(final Bytes key, final long timestamp, final int seqnum);
+ }
+
Review comment:
Is this used somewhere?
##########
File path: streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowStore.java
##########
@@ -33,172 +39,112 @@
* For key range queries, like fetch(key, fromTime, toTime), use the {@link RocksDBWindowStore}
* which uses the {@link WindowKeySchema} to serialize the record bytes for efficient key queries.
*/
+@SuppressWarnings("unchecked")
public class RocksDBTimeOrderedWindowStore
Review comment:
Two things:
- Seems `TimeOrderedKeySchema` is not needed anymore. Should the class be removed?
- Should we rename the class to remove the `Window` par?
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -146,7 +146,7 @@ public void process(final K key, final V1 value) {
outerJoinWindowStore.ifPresent(store -> {
// Delete the joined record from the non-joined outer window store
- store.put(KeyAndJoinSide.make(!isLeftSide, key), null, otherRecordTimestamp);
+ store.put(KeyAndJoinSide.make(!isLeftSide, key, otherRecordTimestamp), null);
Review comment:
Should you call `store.delete(KeyAndJoinSide.make(!isLeftSide, key, otherRecordTimestamp))` now?
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKStreamJoin.java
##########
@@ -209,37 +208,36 @@ private void emitNonJoinedOuterRecords(final WindowStore<KeyAndJoinSide<K>, Left
// reset to MAX_VALUE in case the store is empty
sharedTimeTracker.minTime = Long.MAX_VALUE;
- try (final KeyValueIterator<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> it = store.all()) {
+ try (final KeyValueIterator<KeyAndJoinSide<K>, LeftOrRightValue> it = store.all()) {
while (it.hasNext()) {
- final KeyValue<Windowed<KeyAndJoinSide<K>>, LeftOrRightValue> record = it.next();
+ final KeyValue<KeyAndJoinSide<K>, LeftOrRightValue> record = it.next();
- final Windowed<KeyAndJoinSide<K>> windowedKey = record.key;
- final LeftOrRightValue value = record.value;
- sharedTimeTracker.minTime = windowedKey.window().start();
+ final KeyAndJoinSide<K> keyAndJoinSide = record.key;
+ final LeftOrRightValue<V1, V2> value = record.value;
+ final K key = keyAndJoinSide.getKey();
+ final long timestamp = keyAndJoinSide.getTimestamp();
+ sharedTimeTracker.minTime = timestamp;
// Skip next records if window has not closed
- if (windowedKey.window().start() + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
+ if (timestamp + joinAfterMs + joinGraceMs >= sharedTimeTracker.streamTime) {
break;
}
- final K key = windowedKey.key().getKey();
- final long time = windowedKey.window().start();
-
final R nullJoinedValue;
if (isLeftSide) {
nullJoinedValue = joiner.apply(key,
- (V1) value.getLeftValue(),
- (V2) value.getRightValue());
+ value.getLeftValue(),
+ value.getRightValue());
} else {
nullJoinedValue = joiner.apply(key,
- (V1) value.getRightValue(),
- (V2) value.getLeftValue());
+ (V1) value.getRightValue(),
+ (V2) value.getLeftValue());
}
- context().forward(key, nullJoinedValue, To.all().withTimestamp(time));
+ context().forward(key, nullJoinedValue, To.all().withTimestamp(timestamp));
// Delete the key from the outer window store now it is emitted
- store.put(record.key.key(), null, record.key.window().start());
+ store.put(keyAndJoinSide, null);
Review comment:
Should you call `store.delete(keyAndJoinSide)` now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org