[GitHub] [ozone] adoroszlai commented on a diff in pull request #4251: HDDS-7908. Support OM Metadata operation Generator in `Ozone freon`

["adoroszlai (via GitHub)" <gi...@apache.org>]

adoroszlai commented on code in PR #4251:
URL: https://github.com/apache/ozone/pull/4251#discussion_r1098444618


##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/BaseFreonGenerator.java:
##########
@@ -484,6 +548,10 @@ public void setThreadNo(int threadNo) {
     this.threadNo = threadNo;
   }
 
+  public long getThreadSequenceId() {
+    return threadSequenceId.get();

Review Comment:
   Please add javadoc comment explaining purpose of this new sequence ID.



##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/BaseFreonGenerator.java:
##########
@@ -117,6 +138,11 @@ public class BaseFreonGenerator {
   private ExecutorService executor;
   private ProgressBar progressBar;
 
+  private final ThreadLocal<Long> threadSequenceId = new ThreadLocal<>();
+  private final AtomicLong id = new AtomicLong(0);
+
+  private final AtomicBoolean completion = new AtomicBoolean(false);

Review Comment:
   Nit: please rename to `completed`.



##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/BaseFreonGenerator.java:
##########
@@ -92,6 +98,18 @@ public class BaseFreonGenerator {
       defaultValue = "10")
   private int threadNo;
 
+  @Option(names = {"--timebase"},
+      description = "If set, freon will run for the duration of the --runtime"
+          + " specified even if the --number-of-tests operation"
+          + " has been completed.",
+      defaultValue = "false")
+  private boolean timebase;
+
+  @Option(names = {"--runtime"},
+      description = "Tell freon to terminate processing after"
+          + "the specified period of time in seconds.")
+  private long runtime;

Review Comment:
   I would prefer `--min-duration` and `--max-duration` options instead of `--runtime` and `--timebase`.  Min and max may be used in any combination (i.e. none, either or both of them), the only restriction being `min <= max`.
   
   Also, it would be nice to accept duration in the format we accept in config files (see `TimeDurationUtil` for code).



##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/BaseFreonGenerator.java:
##########
@@ -266,14 +311,27 @@ public void init() {
         }, 10);
 
     executor = Executors.newFixedThreadPool(threadNo);
-
-    progressBar = new ProgressBar(System.out, testNo, successCounter::get,
-        freonCommand.isInteractive());

Review Comment:
   Please keep the `isInteractive` flag.  Progress bar should not be console-based in non-interactive environment.  (Try `docker-compose exec -T scm ozone freon ...` for testing.)



##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/BaseFreonGenerator.java:
##########
@@ -311,6 +369,12 @@ public void printReport() {
     Consumer<String> print = freonCommand.isInteractive()
         ? System.out::println
         : LOG::info;
+
+    messages.add("\nOption:");
+    for (CommandLine.Model.OptionSpec option : spec.options()) {
+      String name = option.longestName();
+      messages.add(name + "=" + option.getValue());
+    }

Review Comment:
   Can you please print these only for `--verbose` mode?  (Ideally please extract this part to a separate task.)



-- 
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: issues-unsubscribe@ozone.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org