satishd commented on code in PR #14328:
URL: https://github.com/apache/kafka/pull/14328#discussion_r1314490212


##########
storage/src/test/java/org/apache/kafka/tiered/storage/integration/ReassignReplicaExpandTest.java:
##########
@@ -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.tiered.storage.integration;
+
+import org.apache.kafka.tiered.storage.TieredStorageTestBuilder;
+import org.apache.kafka.tiered.storage.TieredStorageTestHarness;
+import org.apache.kafka.tiered.storage.specs.KeyValueSpec;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.kafka.common.utils.Utils.mkEntry;
+import static org.apache.kafka.common.utils.Utils.mkMap;
+
+public class ReassignReplicaExpandTest extends TieredStorageTestHarness {
+
+    protected final Integer broker0 = 0;
+    protected final Integer broker1 = 1;
+
+    /**
+     * Cluster of two brokers
+     * @return number of brokers in the cluster
+     */
+    @Override
+    public int brokerCount() {
+        return 2;
+    }
+
+    /**
+     * Number of partitions in the '__remote_log_metadata' topic
+     * @return number of partitions in the '__remote_log_metadata' topic
+     */
+    @Override
+    public int numRemoteLogMetadataPartitions() {
+        return 2;
+    }
+
+    @Override
+    protected void writeTestSpecifications(TieredStorageTestBuilder builder) {
+        final String topicA = "topicA";
+        final String topicB = "topicB";
+        final Integer p0 = 0;
+        final Integer partitionCount = 5;
+        final Integer replicationFactor = 2;
+        final Integer maxBatchCountPerSegment = 1;
+        final Map<Integer, List<Integer>> replicaAssignment = null;
+        final boolean enableRemoteLogStorage = true;
+        final List<Integer> metadataPartitions = new ArrayList<>();
+        for (int i = 0; i < numRemoteLogMetadataPartitions(); i++) {
+            metadataPartitions.add(i);
+        }
+
+        builder
+                // create topicA with 5 partitions and 2 RF
+                .createTopic(topicA, partitionCount, replicationFactor, 
maxBatchCountPerSegment,

Review Comment:
   What is the reason for creating topicA? No messages are sent to that topic 
and the respective checks. But I see a check whether metadata partitions are 
assigned or not for this topic. 



##########
storage/src/test/java/org/apache/kafka/tiered/storage/integration/ReassignReplicaMoveTest.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.tiered.storage.integration;
+
+import java.util.Collections;
+import java.util.List;
+
+public final class ReassignReplicaMoveTest extends ReassignReplicaExpandTest {
+
+    @Override
+    protected List<Integer> replicaIds() {

Review Comment:
   Partition is moved because the num of replicas is still 1 but it is assigned 
to another broker. 
   Please add these details so that it will be easy for readers to follow the 
test clearly. 



##########
storage/src/test/java/org/apache/kafka/tiered/storage/integration/ReassignReplicaShrinkTest.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.tiered.storage.integration;
+
+import org.apache.kafka.tiered.storage.TieredStorageTestBuilder;
+import org.apache.kafka.tiered.storage.TieredStorageTestHarness;
+import org.apache.kafka.tiered.storage.specs.KeyValueSpec;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.kafka.common.utils.Utils.mkEntry;
+import static org.apache.kafka.common.utils.Utils.mkMap;
+
+public final class ReassignReplicaShrinkTest extends TieredStorageTestHarness {
+
+    /**
+     * Cluster of two brokers
+     * @return number of brokers in the cluster
+     */
+    @Override
+    public int brokerCount() {
+        return 2;
+    }
+
+    /**
+     * Number of partitions in the '__remote_log_metadata' topic
+     * @return number of partitions in the '__remote_log_metadata' topic
+     */
+    @Override
+    public int numRemoteLogMetadataPartitions() {
+        return 2;
+    }
+
+    @Override
+    protected void writeTestSpecifications(TieredStorageTestBuilder builder) {
+        final Integer broker0 = 0;
+        final Integer broker1 = 1;
+        final String topicA = "topicA";
+        final Integer p0 = 0;
+        final Integer p1 = 1;
+        final Integer partitionCount = 2;
+        final Integer replicationFactor = 2;
+        final Integer maxBatchCountPerSegment = 1;
+        final boolean enableRemoteLogStorage = true;
+        final Map<Integer, List<Integer>> replicaAssignment = mkMap(
+                mkEntry(p0, Arrays.asList(broker0, broker1)),
+                mkEntry(p1, Arrays.asList(broker1, broker0))
+        );
+
+        builder
+                // create topicA with 2 partitions and 2 RF
+                .createTopic(topicA, partitionCount, replicationFactor, 
maxBatchCountPerSegment,
+                        replicaAssignment, enableRemoteLogStorage)
+                // send records to partition 0, expect that the segments are 
uploaded and removed from local log dir
+                .expectSegmentToBeOffloaded(broker0, topicA, p0, 0, new 
KeyValueSpec("k0", "v0"))
+                .expectSegmentToBeOffloaded(broker0, topicA, p0, 1, new 
KeyValueSpec("k1", "v1"))
+                .expectEarliestLocalOffsetInLogDirectory(topicA, p0, 2L)
+                .produce(topicA, p0, new KeyValueSpec("k0", "v0"), new 
KeyValueSpec("k1", "v1"),
+                        new KeyValueSpec("k2", "v2"))
+                // send records to partition 1, expect that the segments are 
uploaded and removed from local log dir
+                .expectSegmentToBeOffloaded(broker1, topicA, p1, 0, new 
KeyValueSpec("k0", "v0"))
+                .expectSegmentToBeOffloaded(broker1, topicA, p1, 1, new 
KeyValueSpec("k1", "v1"))
+                .expectEarliestLocalOffsetInLogDirectory(topicA, p1, 2L)
+                .produce(topicA, p1, new KeyValueSpec("k0", "v0"), new 
KeyValueSpec("k1", "v1"),
+                        new KeyValueSpec("k2", "v2"))
+                // shrink the replication factor to 1
+                .shrinkReplica(topicA, p0, Collections.singletonList(broker1))
+                .shrinkReplica(topicA, p1, Collections.singletonList(broker0))
+                .expectLeader(topicA, p0, broker1, false)

Review Comment:
   For partition p0, shrunk the replicas to 1 viz broker1. This should have 
become the leader as the earlier leader was removed from the assignment. 
   
   This is similar to partition p1 with shrunk replica of broker0. 



##########
core/src/main/scala/kafka/cluster/Partition.scala:
##########
@@ -1647,11 +1647,13 @@ class Partition(val topicPartition: TopicPartition,
     * @param newOffset The new offset to start the log with
     * @param isFuture True iff the truncation should be performed on the 
future log of this partition
     */
-  def truncateFullyAndStartAt(newOffset: Long, isFuture: Boolean): Unit = {
+  def truncateFullyAndStartAt(newOffset: Long,
+                              isFuture: Boolean,
+                              logStartOffsetOpt: Option[Long] = None): Unit = {

Review Comment:
   Please add javadoc for this new parameter. Same comment at other places like 
`LogManager` and `UnifiedLog`.



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to