Comments on branch. You can truncate this commit after reading.

adapter-cqld4/src/main/java/io/nosqlbench/adapter/cqld4/opdispensers/Cqld4PreparedStmtDispenser.java

@@ -91,7 +91,8 @@ public class Cqld4PreparedStmtDispenser extends Cqld4BaseOpDispenser {
                 isRetryReplace(),
                 getMaxLwtRetries(),
                 processors,
-                getExpectedResultExpression()
+                getExpectedResultExpression() // TODO/MVEL: When this is moved to BaseOpDispenser + StandardAction
+                // there will be no need to add it to the value type for the op, since the dispenser can hold it
             );
         } catch (Exception exception) {
             return CQLD4PreparedStmtDiagnostics.rebindWithDiagnostics(

adapter-cqld4/src/main/java/io/nosqlbench/adapter/cqld4/optypes/Cqld4CqlOp.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 nosqlbench
+ * Copyright (c) 2022-2023 nosqlbench
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -109,6 +109,10 @@ public abstract class Cqld4CqlOp implements CycleOp<ResultSet>, VariableCapture,
 
         Iterator<Row> reader = rs.iterator();
         int pages = 0;
+        // TODO/MVEL: An optimization to this would be to collect the results in a result set processor,
+        // but allow/require this processor to be added to an op _only_ in the event that it would
+        // be needed by a downstream consumer like the MVEL expected result evaluator
+
         var resultRows = new ArrayList<Row>();
         while (true) {
             int pageRows = rs.getAvailableWithoutFetching();
@@ -125,6 +129,7 @@ public abstract class Cqld4CqlOp implements CycleOp<ResultSet>, VariableCapture,
                 break;
             }
             totalRows += pageRows; // TODO JK what is this for?
+            // TODO/MVEL: JK: this is meant to go to a total-rows metric, although it is not wired correctly yet
         }
         processors.flush();
         return rs;
@@ -156,6 +161,11 @@ public abstract class Cqld4CqlOp implements CycleOp<ResultSet>, VariableCapture,
 
     @Override
     public boolean verified() { // TODO JK can this be made CQL agnostic? And moved to BaseOpDispenser?
+        // TODO/MVEL: Yes, it can. The initial implementation should, and it should actually be inside
+        // the main StandardAction, _after_ CycleOp or ChainingOp result is computed
         return MVEL.executeExpression(expectedResultExpression, results.get(), boolean.class);
+        // TODO/MVEL: Wherever this logic lives, we might want to have a symbolic description which
+        // is emitted for logging our metrics purposes indicating the success or failure outcomes.
+        // perhaps something like expected-name: .... and metrics could be then <expected-name>-success and <expected-name>-error
     }
 }

adapters-api/src/main/java/io/nosqlbench/engine/api/activityimpl/BaseOpDispenser.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 nosqlbench
+ * Copyright (c) 2022-2023 nosqlbench
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -69,6 +69,8 @@ public abstract class BaseOpDispenser<T extends Op, S> implements OpDispenser<T>
         configureExpectations(op);
     }
 
+    // TODO/MVEL: Please add some error handling around that explains to the user
+    // what happened in the event of a compilation failure.
     private void configureExpectations(ParsedOp op) {
         op.getOptionalStaticValue("expected-result", String.class)
             .map(MVEL::compileExpression)

adapters-api/src/main/java/io/nosqlbench/engine/api/activityimpl/uniform/flowtypes/Op.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 nosqlbench
+ * Copyright (c) 2022-2023 nosqlbench
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -33,6 +33,8 @@ package io.nosqlbench.engine.api.activityimpl.uniform.flowtypes;
  */
 // TODO: optimize the runtime around the specific op type
 public interface Op extends OpResultSize {
+    // TODO/MVEL: Let's take this out of here to keep Op as a tagging interface
+    // I think it will sit fine on the BaseOpDispenser, and lifecycles align for this nicely
     default boolean verified() {
         return false;
     }

engine-api/src/main/java/io/nosqlbench/engine/api/activityimpl/uniform/actions/StandardAction.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 nosqlbench
+ * Copyright (c) 2022-2023 nosqlbench
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -115,12 +115,13 @@ public class StandardAction<A extends StandardActivity<R, ?>, R extends Op> impl
 
                         if (dispenser.getExpectedResultExpression() != null) { // TODO JK refactor the whole if/else break/continue tree
                             if (op.verified()) { // TODO JK Could this be moved to BaseOpDispenser?
-                                logger.info(() -> "Verification of result passed");
+                                logger.info(() -> "Verification of result passed"); // TODO/MVEL: this is too verbose per cycle
                                 break;
                             } else {
                                 // retry
                                 var triesLeft = maxTries - tries;
                                 logger.info("Verification of result did not pass - {} retries left", triesLeft);
+                                // TODO/MVEL: I think we should designate a separate logging channel for verification logic
                                 if (triesLeft == 0) {
                                     var retriesExhausted = new RuntimeException("Max retries for verification step exhausted."); // TODO JK do we need a dedicated exception here? VerificationRetriesExhaustedException?
                                     var errorDetail = errorHandler.handleError(retriesExhausted, cycle, nanos);
@@ -129,6 +130,9 @@ public class StandardAction<A extends StandardActivity<R, ?>, R extends Op> impl
                                     break;
                                 }
                                 continue;
+                                // TODO/MVEL: I think we should collapse all this if possible to throwing a UnverifiedError and let error handlers do their thing.
+                                // TODO/MVEL: This would work nicely with existing mechanisms and allow users to route errors and status codes as they like.
+                                // TODO/MVEL: A future refinement would be to allow customized error handlers (+-) per dispenser
                             }
                         } else {
                             break;