-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-14491: [5/N] Basic operations for RocksDB versioned store #13188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
ecbdeb3
ef05b8d
da5ac54
4eeea87
642651e
ddeb18b
42fb81d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| /* | ||
| * 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; | ||
|
|
||
| import org.apache.kafka.streams.errors.InvalidStateStoreException; | ||
| import org.apache.kafka.streams.processor.StateStore; | ||
|
|
||
| /** | ||
| * A key-value store that stores multiple record versions per key, and supports timestamp-based | ||
| * retrieval operations to return the latest record (per key) as of a specified timestamp. | ||
| * Only one record is stored per key and timestamp, i.e., a second call to | ||
| * {@link #put(Object, Object, long)} with the same key and timestamp will replace the first. | ||
| * <p> | ||
| * Each store instance has an associated, fixed-duration "history retention" which specifies | ||
| * how long old record versions should be kept for. In particular, a versioned store guarantees | ||
| * to return accurate results for calls to {@link #get(Object, long)} where the provided timestamp | ||
| * bound is within history retention of the current observed stream time. (Queries with timestamp | ||
| * bound older than the specified history retention are considered invalid.) | ||
| * | ||
| * @param <K> The key type | ||
| * @param <V> The value type | ||
| */ | ||
| public interface VersionedKeyValueStore<K, V> extends StateStore { | ||
|
|
||
| /** | ||
| * Add a new record version associated with this key. | ||
|
vcrfxia marked this conversation as resolved.
Outdated
|
||
| * | ||
| * @param key The key | ||
| * @param value The value, it can be {@code null}; | ||
| * if the serialized bytes are also {@code null} it is interpreted as a delete | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kafka has an implicit contract that We should just say, "can be null; null is treated as delete".
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense that null object must always be serialized to null bytes, but it's also possible that a non-null object serializes to null bytes, right? My intention with this javadoc was to clarify that it's the serialization that determines whether the put() is treated as a delete -- even if the value itself is not null, as long as its serialization is null then it counts as a delete. If you think this clarification is more confusing than its worth, I can remove it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That also violations of the contract (it must be null-to-null and non-null-to-non-null), because downstream it would imply that a null-byte array is not deserialized to a null object. (Note: on restore, we actually don't deserialize data and would treat the null-byte array as delete, so stuff breaks if it was not a tombstone...) I understand your intent, but putting it into the JavaDocs might give the wrong impression that it's ok to do this, while it's not...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm... I found this comment in the code the other day, which gives the impression that it is valid to serialize a non-null value to null bytes:
Is this no longer true?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR was controversial... And I am still not a fan of it -> #7679 (comment) It potentially breaks stuff -- of course, if users follow the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thanks for the additional context. I'll update the javadoc as you suggested. |
||
| * @param timestamp The timestamp for this record version | ||
| * @throws NullPointerException If {@code null} is used for key. | ||
| */ | ||
| void put(K key, V value, long timestamp); | ||
|
|
||
| /** | ||
| * Delete the value associated with this key from the store, at the specified timestamp | ||
| * (if there is such a value), and return the deleted value. | ||
| * <p> | ||
| * This operation is semantically equivalent to {@link #get(Object, long)} #get(key, timestamp))} | ||
|
vcrfxia marked this conversation as resolved.
Outdated
|
||
| * followed by {@link #put(Object, Object, long) #put(key, null, timestamp)}. | ||
| * | ||
| * @param key The key | ||
| * @param timestamp The timestamp for this delete | ||
| * @return The value and timestamp of the latest record associated with this key | ||
| * as of the deletion timestamp (inclusive), or {@code null} if any of | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would simplify: case (1) and (2) is basically the same -- "is a tombstone" is a weird formulation IMHO.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also add something like this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. The reason I had separated the cases (1) and (2) was because there was confusion about this case (latest record version is a tombstone) during initial discussion leading up to the KIP and it was requested that the javadocs be explicit about this. But a lot has changed since those initial discussions so let me update it. Also, I assume it's fine to update javadocs proposed in the KIP after it's been accepted, right? Should I go back and update the KIP too?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally -- JavaDocs are not public API. it's well, docs; if we have them in the KIP, it's just a template/placeholder. Not need to go back to the KIP (otherwise we could never change JavaDocs without altering older KIPs 😂) |
||
| * (1) the store contains no records for this key, (2) the latest record | ||
| * for this key as of the deletion timestamp is a tombstone, or | ||
| * (3) the deletion timestamp is older than this store's history retention | ||
| * (i.e., this store no longer contains data for the provided timestamp). | ||
| * @throws NullPointerException If {@code null} is used for key. | ||
| */ | ||
| VersionedRecord<V> delete(K key, long timestamp); | ||
|
|
||
| /** | ||
| * Get the latest (by timestamp) record associated with this key. | ||
|
vcrfxia marked this conversation as resolved.
Outdated
|
||
| * | ||
| * @param key The key to fetch | ||
| * @return The value and timestamp of the latest record associated with this key, or | ||
|
vcrfxia marked this conversation as resolved.
Outdated
|
||
| * {@code null} if either (1) the store contains no records for this key or (2) the | ||
| * latest record for this key is a tombstone. | ||
|
vcrfxia marked this conversation as resolved.
Outdated
|
||
| * @throws NullPointerException If null is used for key. | ||
| * @throws InvalidStateStoreException if the store is not initialized | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is missing in Maybe we should do a follow up PR to check all existing interfaces if the do document this exception?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I can make a quick pass for this in a follow-up after this is merged.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in #13615. |
||
| */ | ||
| VersionedRecord<V> get(K key); | ||
|
|
||
| /** | ||
| * Get the latest record associated with this key with timestamp not exceeding the specified | ||
| * timestamp bound. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sound complicated... maybe ->
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worry that "associated with this timestamp" could be confusing because the returned record will often have timestamp smaller than the timestamp bound. (This happens whenever there is no record version with the exact provided timestamp.) What do you think about
? I'll make the update.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the concept of a "validity interval" is easiest to understand?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm. Currently "validFrom" and "validTo" are not mentioned anywhere in the public-facing interfaces (VersionedKeyValueStore, VersionedRecord, etc) in javadocs or as parameter names; they are only "officially" introduced as concepts in the javadocs for RocksDBVersionedStore and its implementation helpers. I'm a bit hesitant to introduce these concepts in the public-facing docs at this time if the only purpose is to clarify what's returned from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point.
I cannot remember this; personally I always found it very intuitive (also because it's use in temporal tables in SQL), but of course this could just be me. I agree that we should not use it only here, but rather introduce it "globally" or not use it at all. It's "just" JavaDocs and just easy to change in follow up releases if thy cause confusion, so I don't feel strong about it and leave it to your judgement.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I'll omit for now, and we can always revisit in the future. |
||
| * | ||
| * @param key The key to fetch | ||
| * @param asOfTimestamp The timestamp bound. This bound is inclusive; if a record | ||
| * (for the specified key) exists with this timestamp, then | ||
| * this is the record that will be returned. | ||
| * @return The value and timestamp of the latest record associated with this key | ||
| * satisfying the provided timestamp bound, or {@code null} if any of | ||
| * (1) the store contains no records for this key, (2) the latest record | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cf comment on |
||
| * for this key satisfying the provided timestamp bound is a tombstone, or | ||
| * (3) the provided timestamp bound is older than this store's history retention | ||
| * (i.e., this store no longer contains data for the provided timestamp bound). | ||
| * @throws NullPointerException If null is used for key. | ||
| * @throws InvalidStateStoreException if the store is not initialized | ||
| */ | ||
| VersionedRecord<V> get(K key, long asOfTimestamp); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| /* | ||
| * 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; | ||
|
|
||
| import org.apache.kafka.streams.KeyValue; | ||
|
|
||
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Combines a value from a {@link KeyValue} with a timestamp, for use as the return type | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, we don't know how
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied from ValueAndTimestamp (link) 🤷 I'll remove it. |
||
| * from {@link VersionedKeyValueStore#get(Object, long)} and related methods. | ||
| * | ||
| * @param <V> The value type | ||
| */ | ||
| public final class VersionedRecord<V> { | ||
| private final V value; | ||
| private final long timestamp; | ||
|
|
||
| /** | ||
| * Create a new {@link VersionedRecord} instance. {@code value} cannot be {@code null}. | ||
| * | ||
| * @param value the value | ||
| * @param timestamp the timestamp | ||
| */ | ||
| public VersionedRecord(final V value, final long timestamp) { | ||
| this.value = Objects.requireNonNull(value); | ||
| this.timestamp = timestamp; | ||
| } | ||
|
|
||
| public V value() { | ||
| return value; | ||
| } | ||
|
|
||
| public long timestamp() { | ||
| return timestamp; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "<" + value + "," + timestamp + ">"; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o == null || getClass() != o.getClass()) { | ||
| return false; | ||
| } | ||
| final VersionedRecord<?> that = (VersionedRecord<?>) o; | ||
| return timestamp == that.timestamp && | ||
| Objects.equals(value, that.value); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(value, timestamp); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we specify what "now" is with regard to history retention time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line above says "where the provided timestamp bound is within history retention of the current observed stream team." Do you think that needs additional clarification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry. Seems I missed it... disregard my original comment.