jsancio commented on code in PR #22805:
URL: https://github.com/apache/kafka/pull/22805#discussion_r3617142814
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -106,7 +107,7 @@ object DynamicBrokerConfig {
Using.resource(
RecordsSnapshotReader.of(
rawSnapshotReader,
- raftManager.recordSerde,
+ RecordsDecodingStrategy.dataAndControl(raftManager.recordSerde),
Review Comment:
Interesting. It would be nice to remove this complexity from external (most)
user of KRaft. It is okay for kraft (raft module) to have to handle this
complexity. What do you think? For example, users of the raft module should not
have to import internal namespaces (`import
org.apache.kafka.raft.internals.RecordsDecodingStrategy`).
This comment applies to a few places.
##########
raft/src/main/java/org/apache/kafka/raft/internals/RecordsBatchReader.java:
##########
@@ -108,7 +108,7 @@ public static <T> RecordsBatchReader<T> of(
) {
return new RecordsBatchReader<>(
baseOffset,
- new RecordsIterator<>(records, serde, bufferSupplier,
maxBatchSize, doCrcValidation, logContext),
+ new RecordsIterator<>(records,
RecordsDecodingStrategy.dataAndControl(serde), bufferSupplier, maxBatchSize,
doCrcValidation, logContext),
Review Comment:
Feel free to split this across multiple lines.
```java
new RecordsIterator<>(
records,
RecordsDecodingStrategy.dataAndControl(serde),
bufferSupplier,
maxBatchSize,
doCrcValidation,
logContext
),
```
##########
raft/src/main/java/org/apache/kafka/raft/internals/RecordsDecodingStrategy.java:
##########
@@ -0,0 +1,255 @@
+/*
+ * 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.raft.internals;
+
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.record.internal.DefaultRecordBatch;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.common.utils.internals.BufferSupplier;
+import org.apache.kafka.common.utils.internals.ByteUtils;
+import org.apache.kafka.raft.Batch;
+import org.apache.kafka.raft.ControlRecord;
+import org.apache.kafka.server.common.serialization.RecordSerde;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.BiFunction;
+
+/**
+ * Decides which records {@link RecordsIterator} decodes when turning a batch
into a {@link Batch}.
+ * A batch's records are decoded only when this strategy is interested in
them; otherwise the batch
+ * is returned as a {@link Batch#notDecoded} batch carrying only the offset
information.
+ *
+ * <p>Use one of the factory methods to select the behavior:
+ * <ul>
+ * <li>{@link #dataAndControl} decodes both the control and data
records.</li>
+ * <li>{@link #controlOnly} decodes only the control records and skips the
data records. Used by
+ * the internal kraft partition listener, which needs no serde.</li>
+ * <li>{@link #dataOnly} decodes only the data records and skips the control
records.</li>
+ * <li>{@link #none} skips both.</li>
+ * </ul>
+ */
+public final class RecordsDecodingStrategy<T> {
+ private final boolean decodeControlRecords;
+ // When present, data records are decoded with this serde; when empty,
they are skipped.
+ private final Optional<RecordSerde<T>> serde;
+
+ private RecordsDecodingStrategy(boolean decodeControlRecords,
Optional<RecordSerde<T>> serde) {
+ this.decodeControlRecords = decodeControlRecords;
+ this.serde = serde;
+ }
+
+ /**
+ * Decodes both the control and data records of a batch.
+ */
+ public static <T> RecordsDecodingStrategy<T> dataAndControl(RecordSerde<T>
serde) {
+ return new RecordsDecodingStrategy<>(true, Optional.of(serde));
+ }
+
+ /**
+ * Decodes only the data records of a batch and skips the control records.
+ */
+ public static <T> RecordsDecodingStrategy<T> dataOnly(RecordSerde<T>
serde) {
+ return new RecordsDecodingStrategy<>(false, Optional.of(serde));
+ }
+
+ /**
+ * Decodes only the control records of a batch and skips the data records.
+ */
+ public static <T> RecordsDecodingStrategy<T> controlOnly() {
+ return new RecordsDecodingStrategy<>(true, Optional.empty());
+ }
+
+ /**
+ * Skips both the control and data records of a batch.
+ */
+ public static <T> RecordsDecodingStrategy<T> none() {
+ return new RecordsDecodingStrategy<>(false, Optional.empty());
+ }
+
+ Batch<T> readBatch(DefaultRecordBatch batch, BufferSupplier
bufferSupplier, int numRecords) {
+ if (batch.isControlBatch()) {
+ return decodeControlRecords ? readControlBatch(batch,
bufferSupplier, numRecords) : notDecodedBatch(batch, numRecords);
+ } else {
+ return serde.isPresent() ? readDataBatch(batch, serde.get(),
bufferSupplier, numRecords) : notDecodedBatch(batch, numRecords);
+ }
+ }
+
+ private Batch<T> readControlBatch(DefaultRecordBatch batch, BufferSupplier
bufferSupplier, int numRecords) {
+ InputStream input = batch.recordInputStream(bufferSupplier);
+ try {
+ List<ControlRecord> records = new ArrayList<>(numRecords);
+ for (int i = 0; i < numRecords; i++) {
+ records.add(readRecord(input, batch.sizeInBytes(),
bufferSupplier, RecordsDecodingStrategy::decodeControlRecord));
+ }
+ return Batch.control(
+ batch.baseOffset(),
+ batch.partitionLeaderEpoch(),
+ batch.maxTimestamp(),
+ batch.sizeInBytes(),
+ records
+ );
+ } finally {
+ Utils.closeQuietly(input, "BytesStream for input containing
records");
+ }
+ }
+
+ private Batch<T> readDataBatch(DefaultRecordBatch batch, RecordSerde<T>
serde, BufferSupplier bufferSupplier, int numRecords) {
Review Comment:
Looks like this can be `private static`.
##########
checkstyle/suppressions.xml:
##########
@@ -155,7 +155,7 @@
<!-- Raft -->
<suppress checks="NPathComplexity"
- files="(DynamicVoter|RecordsIterator).java"/>
+ files="(DynamicVoter|DecodingStrategy).java"/>
Review Comment:
Can we use Java annotations on the specific method(s) that need this. E.g.
`@SuppressWarnings({"NPathComplexity"})`
##########
raft/src/main/java/org/apache/kafka/raft/internals/RecordsDecodingStrategy.java:
##########
@@ -0,0 +1,255 @@
+/*
+ * 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.raft.internals;
+
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.record.internal.DefaultRecordBatch;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.common.utils.internals.BufferSupplier;
+import org.apache.kafka.common.utils.internals.ByteUtils;
+import org.apache.kafka.raft.Batch;
+import org.apache.kafka.raft.ControlRecord;
+import org.apache.kafka.server.common.serialization.RecordSerde;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.BiFunction;
+
+/**
+ * Decides which records {@link RecordsIterator} decodes when turning a batch
into a {@link Batch}.
+ * A batch's records are decoded only when this strategy is interested in
them; otherwise the batch
+ * is returned as a {@link Batch#notDecoded} batch carrying only the offset
information.
+ *
+ * <p>Use one of the factory methods to select the behavior:
+ * <ul>
+ * <li>{@link #dataAndControl} decodes both the control and data
records.</li>
+ * <li>{@link #controlOnly} decodes only the control records and skips the
data records. Used by
+ * the internal kraft partition listener, which needs no serde.</li>
+ * <li>{@link #dataOnly} decodes only the data records and skips the control
records.</li>
+ * <li>{@link #none} skips both.</li>
+ * </ul>
+ */
+public final class RecordsDecodingStrategy<T> {
+ private final boolean decodeControlRecords;
+ // When present, data records are decoded with this serde; when empty,
they are skipped.
+ private final Optional<RecordSerde<T>> serde;
+
+ private RecordsDecodingStrategy(boolean decodeControlRecords,
Optional<RecordSerde<T>> serde) {
+ this.decodeControlRecords = decodeControlRecords;
+ this.serde = serde;
+ }
+
+ /**
+ * Decodes both the control and data records of a batch.
+ */
+ public static <T> RecordsDecodingStrategy<T> dataAndControl(RecordSerde<T>
serde) {
+ return new RecordsDecodingStrategy<>(true, Optional.of(serde));
+ }
+
+ /**
+ * Decodes only the data records of a batch and skips the control records.
+ */
+ public static <T> RecordsDecodingStrategy<T> dataOnly(RecordSerde<T>
serde) {
+ return new RecordsDecodingStrategy<>(false, Optional.of(serde));
+ }
+
+ /**
+ * Decodes only the control records of a batch and skips the data records.
+ */
+ public static <T> RecordsDecodingStrategy<T> controlOnly() {
+ return new RecordsDecodingStrategy<>(true, Optional.empty());
+ }
+
+ /**
+ * Skips both the control and data records of a batch.
+ */
+ public static <T> RecordsDecodingStrategy<T> none() {
+ return new RecordsDecodingStrategy<>(false, Optional.empty());
+ }
+
+ Batch<T> readBatch(DefaultRecordBatch batch, BufferSupplier
bufferSupplier, int numRecords) {
+ if (batch.isControlBatch()) {
+ return decodeControlRecords ? readControlBatch(batch,
bufferSupplier, numRecords) : notDecodedBatch(batch, numRecords);
+ } else {
+ return serde.isPresent() ? readDataBatch(batch, serde.get(),
bufferSupplier, numRecords) : notDecodedBatch(batch, numRecords);
Review Comment:
This looks like
```java
serde
.map(value -> readDataBatch(...))
.orElseGet(() -> notDecodeBatch(...))
```
##########
raft/src/main/java/org/apache/kafka/raft/internals/RecordsDecodingStrategy.java:
##########
@@ -0,0 +1,255 @@
+/*
+ * 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.raft.internals;
+
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.record.internal.DefaultRecordBatch;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.common.utils.internals.BufferSupplier;
+import org.apache.kafka.common.utils.internals.ByteUtils;
+import org.apache.kafka.raft.Batch;
+import org.apache.kafka.raft.ControlRecord;
+import org.apache.kafka.server.common.serialization.RecordSerde;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.BiFunction;
+
+/**
+ * Decides which records {@link RecordsIterator} decodes when turning a batch
into a {@link Batch}.
+ * A batch's records are decoded only when this strategy is interested in
them; otherwise the batch
+ * is returned as a {@link Batch#notDecoded} batch carrying only the offset
information.
+ *
+ * <p>Use one of the factory methods to select the behavior:
+ * <ul>
+ * <li>{@link #dataAndControl} decodes both the control and data
records.</li>
+ * <li>{@link #controlOnly} decodes only the control records and skips the
data records. Used by
+ * the internal kraft partition listener, which needs no serde.</li>
+ * <li>{@link #dataOnly} decodes only the data records and skips the control
records.</li>
+ * <li>{@link #none} skips both.</li>
+ * </ul>
+ */
+public final class RecordsDecodingStrategy<T> {
+ private final boolean decodeControlRecords;
+ // When present, data records are decoded with this serde; when empty,
they are skipped.
+ private final Optional<RecordSerde<T>> serde;
+
+ private RecordsDecodingStrategy(boolean decodeControlRecords,
Optional<RecordSerde<T>> serde) {
+ this.decodeControlRecords = decodeControlRecords;
+ this.serde = serde;
+ }
+
+ /**
+ * Decodes both the control and data records of a batch.
+ */
+ public static <T> RecordsDecodingStrategy<T> dataAndControl(RecordSerde<T>
serde) {
+ return new RecordsDecodingStrategy<>(true, Optional.of(serde));
+ }
+
+ /**
+ * Decodes only the data records of a batch and skips the control records.
+ */
+ public static <T> RecordsDecodingStrategy<T> dataOnly(RecordSerde<T>
serde) {
+ return new RecordsDecodingStrategy<>(false, Optional.of(serde));
+ }
+
+ /**
+ * Decodes only the control records of a batch and skips the data records.
+ */
+ public static <T> RecordsDecodingStrategy<T> controlOnly() {
+ return new RecordsDecodingStrategy<>(true, Optional.empty());
+ }
+
+ /**
+ * Skips both the control and data records of a batch.
+ */
+ public static <T> RecordsDecodingStrategy<T> none() {
+ return new RecordsDecodingStrategy<>(false, Optional.empty());
+ }
+
+ Batch<T> readBatch(DefaultRecordBatch batch, BufferSupplier
bufferSupplier, int numRecords) {
+ if (batch.isControlBatch()) {
+ return decodeControlRecords ? readControlBatch(batch,
bufferSupplier, numRecords) : notDecodedBatch(batch, numRecords);
+ } else {
+ return serde.isPresent() ? readDataBatch(batch, serde.get(),
bufferSupplier, numRecords) : notDecodedBatch(batch, numRecords);
+ }
+ }
+
+ private Batch<T> readControlBatch(DefaultRecordBatch batch, BufferSupplier
bufferSupplier, int numRecords) {
+ InputStream input = batch.recordInputStream(bufferSupplier);
+ try {
+ List<ControlRecord> records = new ArrayList<>(numRecords);
+ for (int i = 0; i < numRecords; i++) {
+ records.add(readRecord(input, batch.sizeInBytes(),
bufferSupplier, RecordsDecodingStrategy::decodeControlRecord));
+ }
+ return Batch.control(
+ batch.baseOffset(),
+ batch.partitionLeaderEpoch(),
+ batch.maxTimestamp(),
+ batch.sizeInBytes(),
+ records
+ );
+ } finally {
+ Utils.closeQuietly(input, "BytesStream for input containing
records");
+ }
+ }
+
+ private Batch<T> readDataBatch(DefaultRecordBatch batch, RecordSerde<T>
serde, BufferSupplier bufferSupplier, int numRecords) {
+ InputStream input = batch.recordInputStream(bufferSupplier);
+ try {
+ List<T> records = new ArrayList<>(numRecords);
+ for (int i = 0; i < numRecords; i++) {
+ records.add(readRecord(input, batch.sizeInBytes(),
bufferSupplier, (key, value) -> decodeDataRecord(key, value, serde)));
+ }
+ return Batch.data(
+ batch.baseOffset(),
+ batch.partitionLeaderEpoch(),
+ batch.maxTimestamp(),
+ batch.sizeInBytes(),
+ records
+ );
+ } finally {
+ Utils.closeQuietly(input, "BytesStream for input containing
records");
+ }
+ }
+
+ private Batch<T> notDecodedBatch(DefaultRecordBatch batch, int numRecords)
{
Review Comment:
Looks like this can be `private static`.
##########
raft/src/main/java/org/apache/kafka/raft/Batch.java:
##########
@@ -221,4 +221,41 @@ public static <T> Batch<T> data(
List.of()
);
}
+
+ /**
+ * Create a batch whose records were not decoded, carrying only the offset
information.
Review Comment:
Not only it is not decoded, it is also not included. The bytes of the batch
are not included. How about naming this method `skipped`? I don't remember if
we already discussed this.
##########
raft/src/main/java/org/apache/kafka/snapshot/RecordsSnapshotReader.java:
##########
@@ -110,15 +110,22 @@ public void close() {
public static <T> RecordsSnapshotReader<T> of(
RawSnapshotReader snapshot,
- RecordSerde<T> serde,
+ RecordsDecodingStrategy<T> decodingStrategy,
Review Comment:
Maybe we need an public version of this class that just takes a serde and a
private version of this class that takes a decoding strategy. The public
version of this classes just uses the internal version. How does that change
look?
--
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]
