kevinrr888 commented on code in PR #5525:
URL: https://github.com/apache/accumulo/pull/5525#discussion_r2070431134


##########
test/src/main/java/org/apache/accumulo/test/IteratorEnvIT.java:
##########
@@ -71,121 +92,123 @@ public void configureMiniCluster(MiniAccumuloConfigImpl 
cfg, Configuration hadoo
   /**
    * Basic scan iterator to test IteratorEnvironment returns what is expected.
    */
-  public static class ScanIter extends WrappingIterator {
+  public static class ScanIter extends Filter {
     IteratorScope scope = IteratorScope.scan;
 
     @Override
     public void init(SortedKeyValueIterator<Key,Value> source, 
Map<String,String> options,
         IteratorEnvironment env) throws IOException {
       super.init(source, options, env);
-      testEnv(scope, options, env);
-
       // Checking for compaction on a scan should throw an error.
-      try {
-        env.isUserCompaction();
-        throw new RuntimeException(
-            "Test failed - Expected to throw IllegalStateException when 
checking compaction on a scan.");
-      } catch (IllegalStateException e) {}
-      try {
-        env.isFullMajorCompaction();
-        throw new RuntimeException(
-            "Test failed - Expected to throw IllegalStateException when 
checking compaction on a scan.");
-      } catch (IllegalStateException e) {}
+      testEnv(scope, options, env, true);
+    }
+
+    @Override
+    public boolean accept(Key k, Value v) {
+      // The only reason for filtering out some data is as a way to verify 
init() and testEnv()
+      // have been called
+      return !k.getColumnFamily().toString().equals(BAD_COL_FAM);
     }
   }
 
   /**
    * Basic compaction iterator to test IteratorEnvironment returns what is 
expected.
    */
-  public static class MajcIter extends WrappingIterator {
+  public static class MajcIter extends Filter {
     IteratorScope scope = IteratorScope.majc;
 
     @Override
     public void init(SortedKeyValueIterator<Key,Value> source, 
Map<String,String> options,
         IteratorEnvironment env) throws IOException {
       super.init(source, options, env);
-      testEnv(scope, options, env);
-      try {
-        env.isUserCompaction();
-      } catch (IllegalStateException e) {
-        throw new RuntimeException("Test failed");
-      }
-      try {
-        env.isFullMajorCompaction();
-      } catch (IllegalStateException e) {
-        throw new RuntimeException("Test failed");
-      }
+      // Checking for compaction on majc should not throw an error.
+      testEnv(scope, options, env, false);
+    }
+
+    @Override
+    public boolean accept(Key k, Value v) {
+      // The only reason for filtering out some data is as a way to verify 
init() and testEnv()
+      // have been called
+      return !k.getColumnFamily().toString().equals(BAD_COL_FAM);
     }
   }
 
   /**
    *
    */
-  public static class MincIter extends WrappingIterator {
+  public static class MincIter extends Filter {
     IteratorScope scope = IteratorScope.minc;
 
     @Override
     public void init(SortedKeyValueIterator<Key,Value> source, 
Map<String,String> options,
         IteratorEnvironment env) throws IOException {
       super.init(source, options, env);
-      testEnv(scope, options, env);
-      try {
-        env.isUserCompaction();
-        throw new RuntimeException(
-            "Test failed - Expected to throw IllegalStateException when 
checking compaction on a scan.");
-      } catch (IllegalStateException e) {}
-      try {
-        env.isFullMajorCompaction();
-        throw new RuntimeException(
-            "Test failed - Expected to throw IllegalStateException when 
checking compaction on a scan.");
-      } catch (IllegalStateException e) {}
+      // Checking for compaction on minc should throw an error.
+      testEnv(scope, options, env, true);
+    }
+
+    @Override
+    public boolean accept(Key k, Value v) {
+      // The only reason for filtering out some data is as a way to verify 
init() and testEnv()
+      // have been called
+      return !k.getColumnFamily().toString().equals(BAD_COL_FAM);
     }
   }
 
   /**
    * Test the environment methods return what is expected.
    */
-  private static void testEnv(IteratorScope scope, Map<String,String> opts,
-      IteratorEnvironment env) {
-    TableId expectedTableId = TableId.of(opts.get("expected.table.id"));
+  private static void testEnv(IteratorScope scope, Map<String,String> opts, 
IteratorEnvironment env,
+      boolean checkCompactionShouldThrow) {
+    String expTableIdString = opts.get(EXPECTED_TABLE_ID_OPT);
+    TableId expectedTableId = expTableIdString == null ? null : 
TableId.of(expTableIdString);
+
+    if (checkCompactionShouldThrow) {
+      assertThrows(IllegalStateException.class, env::isUserCompaction,
+          "Test failed - Expected to throw IllegalStateException when checking 
compaction");
+      assertThrows(IllegalStateException.class, env::isFullMajorCompaction,
+          "Test failed - Expected to throw IllegalStateException when checking 
compaction");
+    } else {
+      assertDoesNotThrow(env::isUserCompaction,
+          "Test failed - Expected not to throw exception when checking 
compaction");
+      assertDoesNotThrow(env::isFullMajorCompaction,
+          "Test failed - Expected not to throw exception when checking 
compaction");
+    }
 
     // verify getServiceEnv() and getPluginEnv() are the same objects,
     // so further checks only need to use getPluginEnv()
     @SuppressWarnings("deprecation")
     ServiceEnvironment serviceEnv = env.getServiceEnv();
     PluginEnvironment pluginEnv = env.getPluginEnv();
-    if (serviceEnv != pluginEnv) {
-      throw new RuntimeException("Test failed - 
assertSame(getServiceEnv(),getPluginEnv())");
-    }
+    assertEquals(serviceEnv, pluginEnv, "Test failed - 
assertSame(getServiceEnv(),getPluginEnv())");
 
     // verify property exists on the table config (deprecated and new),
     // with and without custom prefix, but not in the system config
     @SuppressWarnings("deprecation")
-    String accTableConf = 
env.getConfig().get("table.custom.iterator.env.test");
-    if (!"value1".equals(accTableConf)) {
-      throw new RuntimeException("Test failed - Expected table property not 
found in getConfig().");
-    }
+    String accTableConf = env.getConfig().get(CUSTOM_PROP_KEY);
+    assertEquals(CUSTOM_PROP_VAL, accTableConf,
+        "Test failed - Expected table property not found in getConfig().");
     var tableConf = pluginEnv.getConfiguration(env.getTableId());
-    if (!"value1".equals(tableConf.get("table.custom.iterator.env.test"))) {
-      throw new RuntimeException("Test failed - Expected table property not 
found in table conf.");
-    }
-    if (!"value1".equals(tableConf.getTableCustom("iterator.env.test"))) {
-      throw new RuntimeException("Test failed - Expected table property not 
found in table conf.");
-    }
+    assertEquals(CUSTOM_PROP_VAL, tableConf.get(CUSTOM_PROP_KEY),
+        "Test failed - Expected table property not found in table conf.");
+    assertEquals(CUSTOM_PROP_VAL, 
tableConf.getTableCustom(CUSTOM_PROP_KEY_SUFFIX),
+        "Test failed - Expected table property not found in table conf.");
     var systemConf = pluginEnv.getConfiguration();
-    if (systemConf.get("table.custom.iterator.env.test") != null) {
-      throw new RuntimeException("Test failed - Unexpected table property 
found in system conf.");
-    }
+    assertNull(systemConf.get(CUSTOM_PROP_KEY),
+        "Test failed - Unexpected table property found in system conf.");
 
     // check other environment settings
-    if (!scope.equals(env.getIteratorScope())) {
-      throw new RuntimeException("Test failed - Error getting iterator scope");
-    }
-    if (env.isSamplingEnabled()) {
-      throw new RuntimeException("Test failed - isSamplingEnabled returned 
true, expected false");
-    }
-    if (!expectedTableId.equals(env.getTableId())) {
-      throw new RuntimeException("Test failed - Error getting Table ID");
+    assertEquals(scope, env.getIteratorScope(), "Test failed - Error getting 
iterator scope");
+    assertFalse(env.isSamplingEnabled(),
+        "Test failed - isSamplingEnabled returned true, expected false");
+    assertEquals(expectedTableId, env.getTableId(), "Test failed - Error 
getting Table ID");
+    assertNull(env.getSamplerConfiguration());
+    assertThrows(Exception.class, env::cloneWithSamplingEnabled);

Review Comment:
   should we configure sampling for the tables in this test somehow and verify 
the result of env.cloneWithSamplingEnabled? Or is this check sufficient for 
this test



-- 
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: notifications-unsubscr...@accumulo.apache.org

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

Reply via email to