aliehsaeedii commented on code in PR #22165:
URL: https://github.com/apache/kafka/pull/22165#discussion_r3656415254
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryTimeOrderedKeyValueChangeBuffer.java:
##########
@@ -465,28 +512,136 @@ private byte[] internalPriorValueForBuffered(final Bytes
key) {
}
}
+ // The legacy V0/V1 restore paths feed the currently-buffered prior value
back in as plain value
+ // bytes, so unwrap it when the in-memory encoding is the headers-aware
format.
+ private byte[] plainPriorValueForBuffered(final Bytes key) {
+ final byte[] priorValue = internalPriorValueForBuffered(key);
+ return storeHeaders ? Utils.rawPlainValue(priorValue) : priorValue;
+ }
+
+ // Normalizes a restored buffer value so its value parts match the
encoding currently used
+ // in-memory (ValueTimestampHeaders blobs when header stores are enabled,
plain values otherwise),
+ // regardless of the changelog version it was read from.
+ private BufferValue normalizeEncoding(final BufferValue value, final
boolean restoredWithHeaders) {
+ if (storeHeaders == restoredWithHeaders) {
+ return value;
+ }
+ if (storeHeaders) {
+ // Plain -> ValueTimestampHeaders. The original headers/timestamp
of legacy records are
+ // unknown, so we use empty headers and the record-context
timestamp.
+ final long timestamp = value.context().timestamp();
+ return new BufferValue(
+ Utils.rawValueTimestampHeaders(value.priorValue(), timestamp),
+ Utils.rawValueTimestampHeaders(value.oldValue(), timestamp),
+ Utils.rawValueTimestampHeaders(value.newValue(), timestamp),
+ value.context()
+ );
+ }
+ // ValueTimestampHeaders -> plain would drop the per-value
headers/timestamps carried by a V4
+ // changelog. This only arises when a store that previously used a
headers store format is
+ // restored without one, i.e. a store-format downgrade. Downgrades are
not supported, so we
+ // fail fast here rather than silently discarding the stored header
data.
+ throw new StreamsException(
+ "Cannot restore suppress buffer '" + storeName + "': its changelog
contains header-aware "
+ + "(V4) records, but the store is configured without a headers
store format. "
+ + "Downgrading the store format is not supported."
+ );
+ }
+
+ private ValueTimestampHeadersSerializer<V>
valueTimestampHeadersSerializer() {
+ if (valueTimestampHeadersSerializer == null) {
+ valueTimestampHeadersSerializer = new
ValueTimestampHeadersSerializer<>(valueSerde.innerSerde().serializer());
+ }
+ return valueTimestampHeadersSerializer;
+ }
+
+ private ValueTimestampHeadersDeserializer<V>
valueTimestampHeadersDeserializer() {
+ if (valueTimestampHeadersDeserializer == null) {
+ valueTimestampHeadersDeserializer = new
ValueTimestampHeadersDeserializer<>(valueSerde.innerSerde().deserializer());
+ }
+ return valueTimestampHeadersDeserializer;
+ }
+
+ // Serialize a single value part. When storeHeaders is set, the value is
wrapped as a
+ // ValueTimestampHeaders blob carrying the given timestamp and headers;
otherwise the plain
+ // value bytes are stored (the pre-existing V3 behavior). Returns null for
a null value.
+ private byte[] serializeValuePart(final V value, final long timestamp,
final Headers headers) {
+ if (value == null) {
+ return null;
+ }
+ if (storeHeaders) {
+ return valueTimestampHeadersSerializer().serialize(changelogTopic,
ValueTimestampHeaders.make(value, timestamp, headers));
+ }
+ return valueSerde.innerSerde().serializer().serialize(changelogTopic,
headers, value);
+ }
+
+ // Deserialize a single stored value part into a ValueTimestampHeaders.
Only valid when
+ // storeHeaders is set (i.e. the part bytes are a ValueTimestampHeaders
blob).
+ private ValueTimestampHeaders<V> deserializeValuePart(final byte[] bytes) {
+ return bytes == null ? null :
valueTimestampHeadersDeserializer().deserialize(changelogTopic, bytes);
+ }
+
+ // Deserialize a single stored value part into the plain value, handling
both the headers-aware
+ // (ValueTimestampHeaders) and plain encodings.
+ private V deserializeValue(final byte[] bytes, final Headers
fallbackHeaders) {
+ if (bytes == null) {
+ return null;
+ }
+ if (storeHeaders) {
+ return
ValueTimestampHeaders.getValueOrNull(deserializeValuePart(bytes));
+ }
+ return
valueSerde.innerSerde().deserializer().deserialize(changelogTopic,
fallbackHeaders, bytes);
+ }
+
@Override
public boolean put(final long time,
final Record<K, Change<V>> record,
final ProcessorRecordContext recordContext) {
requireNonNull(record.value(), "value cannot be null");
requireNonNull(recordContext, "recordContext cannot be null");
- final Bytes serializedKey =
Bytes.wrap(keySerde.serializer().serialize(changelogTopic,
recordContext.headers(), record.key()));
- final Change<byte[]> serialChange =
valueSerde.serializeParts(changelogTopic, recordContext.headers(),
record.value());
-
+ // The record's own headers (not the processing context's) describe
the new value and must be
+ // the ones forwarded for this key on eviction.
+ final RecordHeaders headers = new RecordHeaders(record.headers());
+ final long timestamp = record.timestamp();
+ final Bytes serializedKey =
Bytes.wrap(keySerde.serializer().serialize(changelogTopic, headers,
record.key()));
final BufferValue buffered = getBuffered(serializedKey);
- final byte[] serializedPriorValue;
- if (buffered == null) {
- serializedPriorValue = serialChange.oldValue;
- } else {
- serializedPriorValue = buffered.priorValue();
+
+ // The context stored with the entry carries the currently-processed
record's headers; these
+ // are what get forwarded downstream when the (new) value is emitted.
Everything else is taken
+ // from the processing context, unchanged from before.
+ final ProcessorRecordContext effectiveContext = new
ProcessorRecordContext(
+ recordContext.timestamp(),
+ recordContext.offset(),
+ recordContext.partition(),
+ recordContext.topic(),
+ headers
+ );
+
+ // The old value's original headers/timestamp are not carried by the
incoming record. On an
+ // in-place update we recover them from the entry's previous new value
(whose value is exactly
+ // this update's old value); on the first insert for a key they are
genuinely unknown.
+ Headers oldHeaders = new RecordHeaders();
+ long oldTimestamp = RecordQueue.UNKNOWN;
+ if (storeHeaders && buffered != null) {
+ final ValueTimestampHeaders<V> previousNewValue =
deserializeValuePart(buffered.newValue());
+ if (previousNewValue != null) {
+ oldHeaders = previousNewValue.headers();
+ oldTimestamp = previousNewValue.timestamp();
+ }
}
+ final Change<V> change = record.value();
+ final byte[] newValue = serializeValuePart(change.newValue, timestamp,
headers);
+ // In plain mode the old value is still serialized with the current
record's headers, exactly
+ // as before, so the stored bytes are unchanged for non-header stores.
+ final byte[] oldValue = serializeValuePart(change.oldValue,
oldTimestamp, storeHeaders ? oldHeaders : headers);
Review Comment:
Good catch, fixed.
One note: the tombstone case turns out to be
order-independent, the newValue == null returns early without touching the
headers, so only the old value writes either way. The order matters for the
ordinary update path.
There is a tombstone question in headers mode, though, and I'd like your
view: there the old value is serialized against the headers recovered from the
previous new value, not against the live record headers, so a tombstone leaves
the emitted headers unwritten where plain mode would carry the old value's. On
eviction I fall back to the buffered context's headers. Is that the behaviour
we'd want, or should headers mode match plain mode here?
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryTimeOrderedKeyValueChangeBuffer.java:
##########
@@ -71,8 +75,14 @@ public final class
InMemoryTimeOrderedKeyValueChangeBuffer<K, V, T> implements T
private static final byte[] V_1_CHANGELOG_HEADER_VALUE = {(byte) 1};
private static final byte[] V_2_CHANGELOG_HEADER_VALUE = {(byte) 2};
private static final byte[] V_3_CHANGELOG_HEADER_VALUE = {(byte) 3};
+ // V4 is identical to V3 on the wire, except that the prior/old/new value
parts are each encoded
+ // as a ValueTimestampHeaders blob
([headersSize][headers][timestamp][value]) instead of a plain
+ // value. It is only written when headers-aware stores are enabled
(dsl.store.format=HEADERS).
+ private static final byte[] V_4_CHANGELOG_HEADER_VALUE = {(byte) 4};
Review Comment:
you're right!
But a suppress entry holds three value parts (prior/old/new) that can each
come from a different record, so it's not a single header set like the other
stores — I'll use distinct record-header keys per part. We'd use distinct keys
(vh.old, vh.prior, …) rather than one.
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryTimeOrderedKeyValueChangeBuffer.java:
##########
@@ -465,28 +513,165 @@ private byte[] internalPriorValueForBuffered(final Bytes
key) {
}
}
+ // The legacy V0/V1 restore paths feed the currently-buffered prior value
back in as plain value
+ // bytes, so unwrap it when the in-memory encoding is the headers-aware
format.
+ private byte[] plainPriorValueForBuffered(final Bytes key) {
+ final byte[] priorValue = internalPriorValueForBuffered(key);
+ return storeHeaders ? unwrapHeadersFormatToPlainValue(priorValue) :
priorValue;
+ }
+
+ // Normalizes a restored buffer value so its value parts match the
encoding currently used
+ // in-memory (ValueTimestampHeaders blobs when header stores are enabled,
plain values otherwise),
+ // regardless of the changelog version it was read from.
+ private BufferValue normalizeEncoding(final BufferValue value, final
boolean restoredWithHeaders) {
+ if (storeHeaders == restoredWithHeaders) {
+ return value;
+ }
+ if (storeHeaders) {
+ // Plain -> ValueTimestampHeaders. The original headers/timestamp
of legacy records are
+ // unknown, so we use empty headers and the record-context
timestamp.
+ final long timestamp = value.context().timestamp();
+ return new BufferValue(
+ wrapPlainValueAsHeadersFormat(value.priorValue(), timestamp),
+ wrapPlainValueAsHeadersFormat(value.oldValue(), timestamp),
+ wrapPlainValueAsHeadersFormat(value.newValue(), timestamp),
+ value.context()
+ );
+ }
+ // ValueTimestampHeaders -> plain would drop the per-value
headers/timestamps carried by a V4
+ // changelog. This only arises when a store that previously used a
headers store format is
+ // restored without one, i.e. a store-format downgrade. Downgrades are
not supported, so we
+ // fail fast here rather than silently discarding the stored header
data.
+ throw new StreamsException(
+ "Cannot restore suppress buffer '" + storeName + "': its changelog
contains header-aware "
+ + "(V4) records, but the store is configured without a headers
store format. "
+ + "Downgrading the store format is not supported."
+ );
+ }
+
+ private ValueTimestampHeadersSerializer<V>
valueTimestampHeadersSerializer() {
+ if (valueTimestampHeadersSerializer == null) {
+ valueTimestampHeadersSerializer = new
ValueTimestampHeadersSerializer<>(valueSerde.innerSerde().serializer());
+ }
+ return valueTimestampHeadersSerializer;
+ }
+
+ private ValueTimestampHeadersDeserializer<V>
valueTimestampHeadersDeserializer() {
+ if (valueTimestampHeadersDeserializer == null) {
+ valueTimestampHeadersDeserializer = new
ValueTimestampHeadersDeserializer<>(valueSerde.innerSerde().deserializer());
+ }
+ return valueTimestampHeadersDeserializer;
+ }
+
+ // Serialize a single value part. When storeHeaders is set, the value is
wrapped as a
+ // ValueTimestampHeaders blob carrying the given timestamp and headers;
otherwise the plain
+ // value bytes are stored (the pre-existing V3 behavior). Returns null for
a null value.
+ private byte[] serializeValuePart(final V value, final long timestamp,
final Headers headers) {
+ if (value == null) {
+ return null;
+ }
+ if (storeHeaders) {
+ return valueTimestampHeadersSerializer().serialize(changelogTopic,
ValueTimestampHeaders.make(value, timestamp, headers));
+ }
+ return valueSerde.innerSerde().serializer().serialize(changelogTopic,
headers, value);
+ }
+
+ // Deserialize a single stored value part into a ValueTimestampHeaders.
Only valid when
+ // storeHeaders is set (i.e. the part bytes are a ValueTimestampHeaders
blob).
+ private ValueTimestampHeaders<V> deserializeValuePart(final byte[] bytes) {
+ return bytes == null ? null :
valueTimestampHeadersDeserializer().deserialize(changelogTopic, bytes);
+ }
+
+ // Deserialize a single stored value part into the plain value, handling
both the headers-aware
+ // (ValueTimestampHeaders) and plain encodings.
+ private V deserializeValue(final byte[] bytes, final Headers
fallbackHeaders) {
+ if (bytes == null) {
+ return null;
+ }
+ if (storeHeaders) {
+ return
ValueTimestampHeaders.getValueOrNull(deserializeValuePart(bytes));
+ }
+ return
valueSerde.innerSerde().deserializer().deserialize(changelogTopic,
fallbackHeaders, bytes);
+ }
+
+ // Wraps a plain value part as a ValueTimestampHeaders blob
([headersSize=0][timestamp][value])
+ // without needing a value serde. Used to normalize restored V0-V3 records
to the in-memory
+ // encoding used when header stores are enabled.
+ private static byte[] wrapPlainValueAsHeadersFormat(final byte[]
plainValue, final long timestamp) {
+ if (plainValue == null) {
+ return null;
+ }
+ final ByteBuffer buffer =
ByteBuffer.allocate(ByteUtils.sizeOfVarint(0) + Long.BYTES + plainValue.length);
+ ByteUtils.writeVarint(0, buffer); // empty headers
+ buffer.putLong(timestamp);
+ buffer.put(plainValue);
+ return buffer.array();
+ }
+
+ // Strips the ValueTimestampHeaders wrapper
([headersSize][headers][timestamp]) from a value
+ // part, leaving the plain value bytes. Used to normalize restored V4
records to the in-memory
+ // encoding used when header stores are disabled.
+ private static byte[] unwrapHeadersFormatToPlainValue(final byte[]
headersFormatValue) {
+ if (headersFormatValue == null) {
+ return null;
+ }
+ final ByteBuffer buffer = ByteBuffer.wrap(headersFormatValue);
+ final int headersSize = ByteUtils.readVarint(buffer);
+ buffer.position(buffer.position() + headersSize + Long.BYTES);
+ final byte[] plainValue = new byte[buffer.remaining()];
+ buffer.get(plainValue);
+ return plainValue;
+ }
+
@Override
public boolean put(final long time,
final Record<K, Change<V>> record,
final ProcessorRecordContext recordContext) {
requireNonNull(record.value(), "value cannot be null");
requireNonNull(recordContext, "recordContext cannot be null");
- final Bytes serializedKey =
Bytes.wrap(keySerde.serializer().serialize(changelogTopic,
recordContext.headers(), record.key()));
- final Change<byte[]> serialChange =
valueSerde.serializeParts(changelogTopic, recordContext.headers(),
record.value());
-
+ // The record's own headers (not the processing context's) describe
the new value and must be
+ // the ones forwarded for this key on eviction.
+ final RecordHeaders newHeaders = new RecordHeaders(record.headers());
+ final long newTimestamp = record.timestamp();
+ final Bytes serializedKey =
Bytes.wrap(keySerde.serializer().serialize(changelogTopic, newHeaders,
record.key()));
final BufferValue buffered = getBuffered(serializedKey);
- final byte[] serializedPriorValue;
- if (buffered == null) {
- serializedPriorValue = serialChange.oldValue;
- } else {
- serializedPriorValue = buffered.priorValue();
+
+ // The context stored with the entry carries the currently-processed
record's headers; these
+ // are what get forwarded downstream when the (new) value is emitted.
Everything else is taken
+ // from the processing context, unchanged from before.
+ final ProcessorRecordContext effectiveContext = new
ProcessorRecordContext(
Review Comment:
You're right — reverted. The recordContext.headers() already is
record.headers(); effectiveContext was a no-op that additionally dropped
sourceRawKey/sourceRawValue via the 5-arg constructor. On evict I now read each
part's ValueTimestampHeaders blob directly, which is what actually matters for
the old value (it comes from an earlier record than the new one).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
