HDDS-6734. ozone admin pipeline list CLI is not backward compatible

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationFactor.java

@@ -76,14 +76,18 @@ public static HddsProtos.ReplicationFactor toProto(
     if (replicationFactor == null) {
       return null;
     }
-    switch (replicationFactor) {
+    return replicationFactor.toProto();
+  }
+
+  public HddsProtos.ReplicationFactor toProto() {
+    switch (this) {
     case ONE:
       return HddsProtos.ReplicationFactor.ONE;
     case THREE:
       return HddsProtos.ReplicationFactor.THREE;
     default:
       throw new IllegalArgumentException(
-          "Unsupported ProtoBuf replication factor: " + replicationFactor);
+          "Unsupported ProtoBuf replication factor: " + this);
     }
   }
 

hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/ListPipelinesSubcommand.java

@@ -20,7 +20,9 @@
 
 import com.google.common.base.Strings;
 import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
 import org.apache.hadoop.hdds.client.ReplicationConfig;
+import org.apache.hadoop.hdds.client.ReplicationFactor;
 import org.apache.hadoop.hdds.client.ReplicationType;
 import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.hdds.scm.cli.ScmSubcommand;
@@ -29,6 +31,8 @@
 import picocli.CommandLine;
 
 import java.io.IOException;
+import java.util.Optional;
+import java.util.function.Predicate;
 import java.util.stream.Stream;
 
 /**
@@ -47,41 +51,76 @@ public class ListPipelinesSubcommand extends ScmSubcommand {
   private String replicationType;
 
   @CommandLine.Option(
-      names = {"-r", "--replication", "-ffc", "--filterByFactor"},
+      names = {"-r", "--replication"},
       description = "Filter listed pipelines by replication, eg ONE, THREE or "
       + "for EC rs-3-2-1024k",
       defaultValue = "")
   private String replication;
 
+  @CommandLine.Option(
+      names = {"-ffc", "--filterByFactor"},
+      description = "[deprecated] Filter pipelines by factor (e.g. ONE, THREE) "
+          + " (implies RATIS replication type)")
+  private ReplicationFactor factor;
+
   @CommandLine.Option(names = {"-s", "--state", "-fst", "--filterByState"},
       description = "Filter listed pipelines by State, eg OPEN, CLOSED",
       defaultValue = "")
   private String state;
 
   @Override
   public void execute(ScmClient scmClient) throws IOException {
+    Optional<Predicate<? super Pipeline>> replicationFilter =
+        getReplicationFilter();
+
     Stream<Pipeline> stream = scmClient.listPipelines().stream();
-    if (!Strings.isNullOrEmpty(replication)) {
-      if (Strings.isNullOrEmpty(replicationType)) {
-        throw new IOException(
-            "ReplicationType cannot be null if replication is passed");
-      }
-      ReplicationConfig repConfig =
-          ReplicationConfig.parse(ReplicationType.valueOf(replicationType),
-              replication, new OzoneConfiguration());
-      stream = stream.filter(
-          p -> p.getReplicationConfig().equals(repConfig));
-    } else if (!Strings.isNullOrEmpty(replicationType)) {
-      stream = stream.filter(
-          p -> p.getReplicationConfig()
-              .getReplicationType()
-              .toString()
-              .compareToIgnoreCase(replicationType) == 0);
+    if (replicationFilter.isPresent()) {
+      stream = stream.filter(replicationFilter.get());
     }
     if (!Strings.isNullOrEmpty(state)) {
       stream = stream.filter(p -> p.getPipelineState().toString()
           .compareToIgnoreCase(state) == 0);
     }
     stream.forEach(System.out::println);
   }
+
+  private Optional<Predicate<? super Pipeline>> getReplicationFilter() {
+    boolean hasReplication = !Strings.isNullOrEmpty(replication);
+    boolean hasFactor = factor != null;
+    boolean hasReplicationType = !Strings.isNullOrEmpty(replicationType);
+
+    if (hasFactor) {
+      if (hasReplication) {
+        throw new IllegalArgumentException(
+            "Factor and replication are mutually exclusive");
+      }
+
+      ReplicationConfig replicationConfig =
+          RatisReplicationConfig.getInstance(factor.toProto());
+      return Optional.of(
+          p -> replicationConfig.equals(p.getReplicationConfig()));
+    }
+
+    if (hasReplication) {
+      if (!hasReplicationType) {
+        throw new IllegalArgumentException(
+            "Replication type is required if replication is set");
+      }
+
+      ReplicationConfig replicationConfig =
+          ReplicationConfig.parse(ReplicationType.valueOf(replicationType),
+              replication, new OzoneConfiguration());
+      return Optional.of(
+          p -> replicationConfig.equals(p.getReplicationConfig()));
+    }
+
+    if (hasReplicationType) {
+      return Optional.of(p -> p.getReplicationConfig()
+          .getReplicationType()
+          .toString()
+          .compareToIgnoreCase(replicationType) == 0);
+    }
+
+    return Optional.empty();
+  }
 }

hadoop-hdds/tools/src/test/java/org/apache/hadoop/hdds/scm/cli/pipeline/TestListPipelinesSubCommand.java

@@ -95,13 +95,25 @@ public void testOnlyOpenReturned() throws IOException {
     Assert.assertEquals(-1, output.indexOf("CLOSED"));
   }
 
-  @Test(expected = IOException.class)
+  @Test(expected = IllegalArgumentException.class)
   public void testExceptionIfReplicationWithoutType() throws IOException {
     CommandLine c = new CommandLine(cmd);
     c.parseArgs("-r", "THREE");
     cmd.execute(scmClient);
   }
 
+  @Test
+  public void testReplicationType() throws IOException {
+    CommandLine c = new CommandLine(cmd);
+    c.parseArgs("-t", "STANDALONE");
+    cmd.execute(scmClient);
+
+    String output = outContent.toString(DEFAULT_ENCODING);
+    Assert.assertEquals(1, output.split(
+        System.getProperty("line.separator")).length);
+    Assert.assertEquals(-1, output.indexOf("EC"));
+  }
+
   @Test
   public void testReplicationAndType() throws IOException {
     CommandLine c = new CommandLine(cmd);
@@ -114,6 +126,25 @@ public void testReplicationAndType() throws IOException {
     Assert.assertEquals(-1, output.indexOf("EC"));
   }
 
+  @Test
+  public void testLegacyFactorWithoutType() throws IOException {
+    CommandLine c = new CommandLine(cmd);
+    c.parseArgs("-ffc", "THREE");
+    cmd.execute(scmClient);
+
+    String output = outContent.toString(DEFAULT_ENCODING);
+    Assert.assertEquals(2, output.split(
+        System.getProperty("line.separator")).length);
+    Assert.assertEquals(-1, output.indexOf("EC"));
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void factorAndReplicationAreMutuallyExclusive() throws IOException {
+    CommandLine c = new CommandLine(cmd);
+    c.parseArgs("-r", "THREE", "-ffc", "ONE");
+    cmd.execute(scmClient);
+  }
+
   @Test
   public void testReplicationAndTypeEC() throws IOException {
     CommandLine c = new CommandLine(cmd);
@@ -140,6 +171,19 @@ public void testReplicationAndTypeAndState() throws IOException {
     Assert.assertEquals(-1, output.indexOf("EC"));
   }
 
+  @Test
+  public void testLegacyFactorAndState() throws IOException {
+    CommandLine c = new CommandLine(cmd);
+    c.parseArgs("-ffc", "THREE", "-fst", "OPEN");
+    cmd.execute(scmClient);
+
+    String output = outContent.toString(DEFAULT_ENCODING);
+    Assert.assertEquals(1, output.split(
+        System.getProperty("line.separator")).length);
+    Assert.assertEquals(-1, output.indexOf("CLOSED"));
+    Assert.assertEquals(-1, output.indexOf("EC"));
+  }
+
   private List<Pipeline> createPipelines() {
     List<Pipeline> pipelines = new ArrayList<>();
     pipelines.add(createPipeline(StandaloneReplicationConfig.getInstance(ONE),