[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #842: HDDS-3455. Change MiniLoadGenerator to a pluggable model.

[GitBox <gi...@apache.org>]

adoroszlai commented on a change in pull request #842:
URL: https://github.com/apache/hadoop-ozone/pull/842#discussion_r413094530



##########
File path: hadoop-ozone/fault-injection-test/mini-chaos-tests/src/test/java/org/apache/hadoop/ozone/loadgenerators/LoadExecutors.java
##########
@@ -70,15 +73,19 @@ private void load(long runTimeMillis) {
 
 
   public void startLoad(long time) {
-    LOG.info("Starting {} threads for {}", numThreads, generator);
-    try {
-      generator.initialize();
-      for (int i = 0; i < numThreads; i++) {
-        futures.add(CompletableFuture.runAsync(
-            () -> load(time), executor));
+    LOG.info("Starting {} threads for {} genrators", numThreads,
+        generators.size());
+    for (LoadGenerator gen : generators) {
+      try {
+        LOG.info("Initializing {} generator", gen);
+        gen.initialize();
+      } catch (Throwable t) {
+        LOG.error("Failed to initialize loadgen:{}", gen, t);
       }
-    } catch (Throwable t) {
-      LOG.error("Failed to initialize loadgen:{}", generator, t);
+    }
+
+    for (int i = 0; i < numThreads; i++) {
+      futures.add(CompletableFuture.runAsync(() -> load(time), executor));

Review comment:
       If some generator fails to `initialize()`, should we still consider it in `load(long)`?  I think it would be more safe to add each generator to a separate list, from which `load(long)` should choose, only after successful initialization.

##########
File path: hadoop-ozone/fault-injection-test/mini-chaos-tests/src/test/java/org/apache/hadoop/ozone/MiniOzoneLoadGenerator.java
##########
@@ -44,66 +39,99 @@
   private static final Logger LOG =
       LoggerFactory.getLogger(MiniOzoneLoadGenerator.class);
 
-  private final List<LoadExecutors> loadExecutors;
+  private final List<LoadGenerator> loadGenerators;
+  private final LoadExecutors loadExecutor;
 
   private final OzoneVolume volume;
   private final OzoneConfiguration conf;
   private final String omServiceID;
 
-  MiniOzoneLoadGenerator(OzoneVolume volume, int numClients, int numThreads,
-      int numBuffers, OzoneConfiguration conf, String omServiceId)
+  MiniOzoneLoadGenerator(OzoneVolume volume, int numThreads,
+      int numBuffers, OzoneConfiguration conf, String omServiceId,
+      List<Class<? extends LoadGenerator>> loadGenratorClazzes)
       throws Exception {
     DataBuffer buffer = new DataBuffer(numBuffers);
-    loadExecutors = new ArrayList<>();
+    loadGenerators = new ArrayList<>();
     this.volume = volume;
     this.conf = conf;
     this.omServiceID = omServiceId;
 
-    // Random Load
-    String mixBucketName = RandomStringUtils.randomAlphabetic(10).toLowerCase();
-    volume.createBucket(mixBucketName);
-    List<LoadBucket> ozoneBuckets = new ArrayList<>(numClients);
-    for (int i = 0; i < numClients; i++) {
-      ozoneBuckets.add(new LoadBucket(volume.getBucket(mixBucketName),
-          conf, omServiceId));
+    for(Class<? extends LoadGenerator> clazz : loadGenratorClazzes) {
+      addLoads(clazz, buffer);
     }
-    RandomLoadGenerator loadGenerator =
-        new RandomLoadGenerator(buffer, ozoneBuckets);
-    loadExecutors.add(new LoadExecutors(numThreads, loadGenerator));
 
-    // Aged Load
-    addLoads(numThreads,
-        bucket -> new AgedLoadGenerator(buffer, bucket));
-
-    //Filesystem Load
-    addLoads(numThreads,
-        bucket -> new FilesystemLoadGenerator(buffer, bucket));
-
-    //Repl Load
-    addLoads(numThreads,
-        bucket -> new ReadOnlyLoadGenerator(buffer, bucket, 20));
+    this.loadExecutor = new LoadExecutors(numThreads, loadGenerators);
   }
 
-  private void addLoads(int numThreads,
-                        Function<LoadBucket, LoadGenerator> function)
-      throws Exception {
+  private void addLoads(Class<? extends LoadGenerator> clazz,
+                        DataBuffer buffer) throws Exception {
     String bucketName = RandomStringUtils.randomAlphabetic(10).toLowerCase();
     volume.createBucket(bucketName);
-    LoadBucket bucket = new LoadBucket(volume.getBucket(bucketName), conf,
-        omServiceID);
-    LoadGenerator loadGenerator = function.apply(bucket);
-    loadExecutors.add(new LoadExecutors(numThreads, loadGenerator));
+    LoadBucket ozoneBucket = new LoadBucket(volume.getBucket(bucketName),
+        conf, omServiceID);
+
+    LoadGenerator loadGenerator = clazz
+        .getConstructor(DataBuffer.class, LoadBucket.class)
+        .newInstance(buffer, ozoneBucket);

Review comment:
       Can you please document the required signature for the constructors of `LoadGenerator` implementations in the interface?




----------------------------------------------------------------
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.

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



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