Re: [PR] `IteratorEnvironment` fixes [accumulo]

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

ctubbsii commented on code in PR #4816:
URL: https://github.com/apache/accumulo/pull/4816#discussion_r1744085449


##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFile.java:
##########
@@ -172,17 +175,27 @@ public interface ScannerOptions {
      * {@link Property#TABLE_PREFIX} may be accepted and used. For example, cache and crypto
      * properties could be passed here.
      *
+     * <p>
+     * Starting with {@code 2.1.4}, {@link PluginEnvironment#getConfiguration(TableId)} (obtained by
+     * {@link IteratorEnvironment#getPluginEnv()}) will return the properties passed in here.
+     *

Review Comment:
   I'm not sure these comments about "starting with 2.1.4" make sense to include here. It should have always worked this way. This is just fixing a bug. I don't think we need to document the implementation details of the corrected behavior for the bug fix in the javadoc.



##########
core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java:
##########
@@ -134,15 +142,22 @@ public SamplerConfiguration getSamplerConfiguration() {
       return samplerConfig;
     }
 
-    @Deprecated(since = "2.1.0")
     @Override
-    public ServiceEnvironment getServiceEnv() {
-      return new ClientServiceEnvironmentImpl(context.get());
+    @Deprecated(since = "2.0.0")
+    public AccumuloConfiguration getConfig() {
+      var ctx = context.get();
+      try {
+        return new ConfigurationCopy(
+            ctx.tableOperations().getConfiguration(ctx.getTableName(tableId.get())));
+      } catch (AccumuloException | TableNotFoundException e) {
+        throw new RuntimeException("Error getting table configuration", e);
+      }

Review Comment:
   What is the rationale for implementing `getConfig`, and specifically as a copy of the table configuration? The behavior of this method in the interface was never well-defined, and was using an internal API and should never be called by user code. I'm not sure why it needs an implementation here, rather than just throwing an exception, like the deprecated default method does in the interface.
   
   If we really do need this, the default method in the interface should implement it by calling the methods on `getPluginEnv()` and wrapping it similarly to what you did here, instead of that default method throwing an exception.



##########
core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java:
##########
@@ -134,15 +142,22 @@ public SamplerConfiguration getSamplerConfiguration() {
       return samplerConfig;
     }
 
-    @Deprecated(since = "2.1.0")
     @Override
-    public ServiceEnvironment getServiceEnv() {
-      return new ClientServiceEnvironmentImpl(context.get());
+    @Deprecated(since = "2.0.0")
+    public AccumuloConfiguration getConfig() {
+      var ctx = context.get();
+      try {
+        return new ConfigurationCopy(
+            ctx.tableOperations().getConfiguration(ctx.getTableName(tableId.get())));
+      } catch (AccumuloException | TableNotFoundException e) {
+        throw new RuntimeException("Error getting table configuration", e);
+      }
     }
 
+    @Deprecated(since = "2.1.0")
     @Override
-    public PluginEnvironment getPluginEnv() {
-      return new ClientServiceEnvironmentImpl(context.get());

Review Comment:
   This is the method that should be implemented. The `getServiceEnv()` method only existed for backwards compatibility. The code should not rely on the interface implementation of `getPluginEnv()` calling the `getServiceEnv()` here, because that behavior is removed when the deprecated code is dropped. Instead, it's best to keep both methods, as before your change, so that when the deprecated one is dropped in subsequent versions, the correct implementation is kept.



##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -66,18 +67,25 @@
 import org.apache.accumulo.core.spi.cache.BlockCacheManager;
 import org.apache.accumulo.core.spi.cache.CacheEntry;
 import org.apache.accumulo.core.spi.cache.CacheType;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
 import org.apache.accumulo.core.spi.crypto.CryptoEnvironment;
 import org.apache.accumulo.core.spi.crypto.CryptoService;
+import org.apache.accumulo.core.util.ConfigurationImpl;
 import org.apache.accumulo.core.util.LocalityGroupUtil;
 import org.apache.hadoop.fs.FSDataInputStream;
 import org.apache.hadoop.io.Text;
 
 import com.google.common.base.Preconditions;
+import com.google.common.base.Suppliers;
 
 class RFileScanner extends ScannerOptions implements Scanner {
 
   private static final byte[] EMPTY_BYTES = new byte[0];
   private static final Range EMPTY_RANGE = new Range();
+  private static final String errorMsg =
+      "This scanner is unrelated to any table or accumulo instance;"
+          + " it operates directly on files. Therefore, it can not support this operation.";
+  private static final TableId dummyTableId = TableId.of("RFileScannerFakeTableId");

Review Comment:
   Why wouldn't this be null?



##########
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java:
##########
@@ -311,14 +319,27 @@ public void updateScanIteratorOption(String iteratorName, String key, String val
   }
 
   private class IterEnv implements IteratorEnvironment {
+    private final Supplier<ServiceEnvironment> serviceEnvironment;

Review Comment:
   What prevents us from just using the implementation from `super.getPluginEnv()` and `super.getServiceEnv()`?



##########
core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java:
##########
@@ -52,16 +52,16 @@ default AccumuloConfiguration getConfig() {
   }
 
   /**
-   * Return the executed scope of the Iterator. Value will be one of the following:
-   * {@link IteratorScope#scan}, {@link IteratorScope#minc}, {@link IteratorScope#majc}
+   * @return the executed scope of the Iterator. Value will be one of the following:
+   *         {@link IteratorScope#scan}, {@link IteratorScope#minc}, {@link IteratorScope#majc}
    */
   default IteratorScope getIteratorScope() {
     throw new UnsupportedOperationException();
   }
 
   /**
-   * Return true if the compaction is a full major compaction. Will throw IllegalStateException if
-   * {@link #getIteratorScope()} != {@link IteratorScope#majc}.
+   * @return true if the compaction is a full major compaction; false otherwise
+   * @throws IllegalStateException if {@link #getIteratorScope()} != {@link IteratorScope#majc}.

Review Comment:
   I think I would have preferred fixing the documentation to drop the exception case, rather than forcing an exception to be thrown in all the scan cases. `false otherwise` should be sufficient.



-- 
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-unsubscribe@accumulo.apache.org

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