atharvalade commented on code in PR #3159: URL: https://github.com/apache/iggy/pull/3159#discussion_r3139433830
########## foreign/java/bench/src/main/java/org/apache/iggy/bench/cli/IggyBenchCommand.java: ########## @@ -0,0 +1,152 @@ +/* + * 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.iggy.bench.cli; + +import picocli.CommandLine.Command; +import picocli.CommandLine.ExitCode; +import picocli.CommandLine.Model.CommandSpec; +import picocli.CommandLine.Option; +import picocli.CommandLine.Spec; + +import java.util.concurrent.Callable; + +@Command( + name = "iggy-bench", + mixinStandardHelpOptions = true, + subcommands = {PinnedProducerCommand.class}, + description = "Benchmark CLI for the Apache Iggy Java SDK.") +public final class IggyBenchCommand implements Callable<Integer> { + + @Option( + names = {"--message-size", "-m"}, + defaultValue = "1000", + description = "Message size in bytes.") + private int messageSize = 1000; + + @Option( + names = {"--messages-per-batch", "-P"}, + defaultValue = "1000", + description = "Messages per batch.") + private int messagesPerBatch = 1000; + + @Option( + names = {"--message-batches", "-b"}, + defaultValue = "1000", + description = "Number of message batches.") + private int messageBatches = 1000; + + @Option( + names = {"--total-data", "-T"}, + defaultValue = "0", + description = "Total data volume in bytes.") + private long totalData; Review Comment: Parent command defines `-p` for `--password` (`IggyBenchCommand` L92). Subcommand defines `-p `for `--producers` (PinnedProducerCommand L59). In picocli, subcommand scope shadows parent scope. If a user writes `iggy-bench pp` `-p` `secretpass` intending to set password, picocli will attempt to parse "secretpass" as an integer for `--producers` and throw a confusing type-conversion error. The user would need to know to write `iggy-bench -p secretpass pp` instead. and also the same for `IggyBenchCommand.java` L92 ########## foreign/java/bench/src/main/java/org/apache/iggy/bench/cli/IggyBenchCommand.java: ########## @@ -0,0 +1,152 @@ +/* + * 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.iggy.bench.cli; + +import picocli.CommandLine.Command; +import picocli.CommandLine.ExitCode; +import picocli.CommandLine.Model.CommandSpec; +import picocli.CommandLine.Option; +import picocli.CommandLine.Spec; + +import java.util.concurrent.Callable; + +@Command( + name = "iggy-bench", + mixinStandardHelpOptions = true, + subcommands = {PinnedProducerCommand.class}, + description = "Benchmark CLI for the Apache Iggy Java SDK.") +public final class IggyBenchCommand implements Callable<Integer> { + + @Option( + names = {"--message-size", "-m"}, + defaultValue = "1000", + description = "Message size in bytes.") + private int messageSize = 1000; + + @Option( + names = {"--messages-per-batch", "-P"}, + defaultValue = "1000", + description = "Messages per batch.") + private int messagesPerBatch = 1000; + + @Option( + names = {"--message-batches", "-b"}, + defaultValue = "1000", + description = "Number of message batches.") + private int messageBatches = 1000; + + @Option( + names = {"--total-data", "-T"}, + defaultValue = "0", + description = "Total data volume in bytes.") + private long totalData; + + @Option( + names = {"--rate-limit", "-r"}, + defaultValue = "0", + description = "(NOT USED CURRENTLY) Optional total rate limit in bytes per second.") + private long rateLimit; Review Comment: Both the parent `(--total-data)` and this subcommand `(--max-topic-size)` bind `-T`. Since both are long, iggy-bench pp `-T` 5000 quietly sets topic size instead of total data with zero indication anything went wrong. I think it would be better to pick a different short flag for one of them. ########## foreign/java/bench/src/main/java/org/apache/iggy/bench/provision/ResourceProvisioner.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.iggy.bench.provision; + +import org.apache.iggy.bench.models.cli.GlobalCliArgs; +import org.apache.iggy.bench.models.cli.PinnedProducerCliArgs; +import org.apache.iggy.bench.models.provision.ProvisionedResources; +import org.apache.iggy.client.blocking.tcp.IggyTcpClient; +import org.apache.iggy.identifier.StreamId; +import org.apache.iggy.topic.CompressionAlgorithm; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.math.BigInteger; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; + +public final class ResourceProvisioner { + + private static final Logger log = LoggerFactory.getLogger(ResourceProvisioner.class); + + public ProvisionedResources provisionResources( + GlobalCliArgs globalCliArgs, PinnedProducerCliArgs pinnedProducerCliArgs) { + var streamNames = new ArrayList<String>(); + var topicNames = List.of("topic-1"); + + var client = IggyTcpClient.builder() + .credentials(globalCliArgs.username(), globalCliArgs.password()) + .buildAndLogin(); + + var existingStreamNames = client.streams().getStreams().stream() + .map(stream -> stream.name()) + .toList(); + + for (var i = 1; i <= pinnedProducerCliArgs.producers(); i++) { + var streamName = "bench-stream-" + i; + streamNames.add(streamName); + + if (existingStreamNames.contains(streamName) && !globalCliArgs.reuseStreams()) { + log.info("Deleting pre-existing stream '{}'", streamName); + client.streams().deleteStream(StreamId.of(streamName)); + } + + if (existingStreamNames.contains(streamName) && globalCliArgs.reuseStreams()) { + log.info("Appending to existing stream '{}'", streamName); + } else { + log.info("Creating the test stream '{}'", streamName); + + client.streams().createStream(streamName); + + var maxTopicSize = pinnedProducerCliArgs.maxTopicSize() == 0L + ? "server default" + : Long.toString(pinnedProducerCliArgs.maxTopicSize()); + var messageExpiry = pinnedProducerCliArgs.messageExpiry() == 0L + ? "never" + : Long.toString(pinnedProducerCliArgs.messageExpiry()); + + log.info( + "Creating the test topic '{}' for stream '{}' with max topic size: {}, message expiry: {}", + topicNames.get(0), + streamName, + maxTopicSize, + messageExpiry); + + client.topics() + .createTopic( + StreamId.of(streamName), + 1L, + CompressionAlgorithm.None, + BigInteger.valueOf(pinnedProducerCliArgs.messageExpiry()), + BigInteger.valueOf(pinnedProducerCliArgs.maxTopicSize()), + Optional.empty(), + topicNames.get(0)); + } + } + + client.close(); Review Comment: `IggyTcpClient` implements `Closeable`. The client is created at L45 but `client.close()` only runs at L95 on the happy path. If any operation between L49-93 throws (e.g., server rejects `createStream`, network error), the TCP connection and its underlying NIO/connection-pool resources leak. For a benchmark tool that may be run repeatedly, this compounds. -- 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]
