adoroszlai commented on code in PR #6577: URL: https://github.com/apache/ozone/pull/6577#discussion_r1576493535
########## hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/CloseAllPipelinesSubcommand.java: ########## @@ -0,0 +1,83 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.hdds.scm.cli.pipeline; + +import org.apache.hadoop.hdds.cli.HddsVersionProvider; +import org.apache.hadoop.hdds.client.ReplicationFactor; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.scm.cli.ScmSubcommand; +import org.apache.hadoop.hdds.scm.client.ScmClient; +import org.apache.hadoop.hdds.scm.pipeline.Pipeline; +import picocli.CommandLine; + +import java.io.IOException; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.Stream; + +/** + * Handler of close pipeline command. + */ [email protected]( + name = "closeAll", Review Comment: I suggest adding the new options to the existing `ozone admin pipeline close` command, instead of introducing a new command. Existing parameter for pipeline ID can be changed to arity = 0..1. ########## hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/ListPipelinesSubcommand.java: ########## @@ -100,7 +100,9 @@ public void execute(ScmClient scmClient) throws IOException { } } - private Optional<Predicate<? super Pipeline>> getReplicationFilter() { + static Optional<Predicate<? super Pipeline>> getReplicationFilter( + String replication, ReplicationFactor factor, String replicationType + ) { Review Comment: This method can then be moved to the mixin class. ########## hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/CloseAllPipelinesSubcommand.java: ########## @@ -0,0 +1,83 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.hdds.scm.cli.pipeline; + +import org.apache.hadoop.hdds.cli.HddsVersionProvider; +import org.apache.hadoop.hdds.client.ReplicationFactor; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.scm.cli.ScmSubcommand; +import org.apache.hadoop.hdds.scm.client.ScmClient; +import org.apache.hadoop.hdds.scm.pipeline.Pipeline; +import picocli.CommandLine; + +import java.io.IOException; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.Stream; + +/** + * Handler of close pipeline command. + */ [email protected]( + name = "closeAll", + description = "Close all pipelines", + mixinStandardHelpOptions = true, + versionProvider = HddsVersionProvider.class) +public class CloseAllPipelinesSubcommand extends ScmSubcommand { + + @CommandLine.Option(names = {"-t", "--type"}, + description = "Filter listed pipelines by replication type, RATIS or EC", + defaultValue = "") + private String replicationType; + + @CommandLine.Option( + names = {"-r", "--replication"}, + description = "Filter listed pipelines by replication, eg ONE, THREE or " Review Comment: Wording related to listing (e.g. `listed` here) should be omitted. ########## hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/CloseAllPipelinesSubcommand.java: ########## @@ -0,0 +1,83 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.hdds.scm.cli.pipeline; + +import org.apache.hadoop.hdds.cli.HddsVersionProvider; +import org.apache.hadoop.hdds.client.ReplicationFactor; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.scm.cli.ScmSubcommand; +import org.apache.hadoop.hdds.scm.client.ScmClient; +import org.apache.hadoop.hdds.scm.pipeline.Pipeline; +import picocli.CommandLine; + +import java.io.IOException; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.Stream; + +/** + * Handler of close pipeline command. + */ [email protected]( + name = "closeAll", + description = "Close all pipelines", + mixinStandardHelpOptions = true, + versionProvider = HddsVersionProvider.class) +public class CloseAllPipelinesSubcommand extends ScmSubcommand { + + @CommandLine.Option(names = {"-t", "--type"}, + description = "Filter listed pipelines by replication type, RATIS or EC", + defaultValue = "") + private String replicationType; + + @CommandLine.Option( + 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", "--filter-by-factor"}, + description = "[deprecated] Filter pipelines by factor (e.g. ONE, THREE) " + + " (implies RATIS replication type)") + private ReplicationFactor factor; + + @Override + public void execute(ScmClient scmClient) throws IOException { + Optional<Predicate<? super Pipeline>> replicationFilter = + ListPipelinesSubcommand.getReplicationFilter(replication, factor, replicationType); + + Stream<Pipeline> stream = + scmClient.listPipelines() + .stream() + .filter(p -> p.getPipelineState() != Pipeline.PipelineState.CLOSED); + if (replicationFilter.isPresent()) { + stream = stream.filter(replicationFilter.get()); + } + + stream.forEach(pipeline -> { + try { + scmClient.closePipeline(HddsProtos.PipelineID.newBuilder().setId(pipeline.getId().getId().toString()).build()); + } catch (IOException e) { + System.out.println("Can't close pipeline: " + pipeline.getId() + ", cause: " + e.getMessage()); Review Comment: Let's print problems to `System.err` instead. Also, I think `Error closing pipeline` sounds better than `Can't close pipeline`. ########## hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/CloseAllPipelinesSubcommand.java: ########## @@ -0,0 +1,83 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.hdds.scm.cli.pipeline; + +import org.apache.hadoop.hdds.cli.HddsVersionProvider; +import org.apache.hadoop.hdds.client.ReplicationFactor; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.scm.cli.ScmSubcommand; +import org.apache.hadoop.hdds.scm.client.ScmClient; +import org.apache.hadoop.hdds.scm.pipeline.Pipeline; +import picocli.CommandLine; + +import java.io.IOException; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.Stream; + +/** + * Handler of close pipeline command. + */ [email protected]( + name = "closeAll", + description = "Close all pipelines", + mixinStandardHelpOptions = true, + versionProvider = HddsVersionProvider.class) +public class CloseAllPipelinesSubcommand extends ScmSubcommand { + + @CommandLine.Option(names = {"-t", "--type"}, + description = "Filter listed pipelines by replication type, RATIS or EC", + defaultValue = "") + private String replicationType; + + @CommandLine.Option( + 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", "--filter-by-factor"}, + description = "[deprecated] Filter pipelines by factor (e.g. ONE, THREE) " + + " (implies RATIS replication type)") + private ReplicationFactor factor; Review Comment: Instead of duplicating these options from `ListPipelinesSubcommand`, please extract them into a separate class that can be used as a `Mixin` in both. See `ListOptions` as an example. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
