`IteratorEnvironment` fixes #4816

kevinrr888 · 2024-08-21T13:57:16Z

Fixed some inconsistencies and bugs in (non-test) implementations of IteratorEnvironment
- Fixed RFileScanners unexpected UnsupportedOperationException when calling env.getServiceEnv() or env.getConfig() (both deprecated) or env.getPluginEnv()
- Some impls returned a new ServiceEnvironment for each call to getServiceEnv()/getPluginEnv(). This did not align as well with the IteratorEnvIT which would assert that these two calls return the same object. Memoize a supplier to the ServiceEnvironment to avoid creating the object unless needed and to return the same object each time.
- Seems like isUserCompaction() and isFullMajorCompaction() are supposed to be throwing an IllegalStateException if it's not a major compaction. Added missing javadoc to isUserCompaction() and changed impls that were not adhering to these docs.
Added tests for these environments to IteratorEnvIT

Questions:

Need confirmation, but it was my interpretation that a TableId doesn't make sense in the context of RFileScanner. Is this correct? ANSWERED
I have getConfiguration() throw an error in RFileScanners IteratorEnvironment since I'm not sure how we would return the system config in this context and I don't think it would even make sense here. Does this seem right? ANSWERED
Made the IteratorEnvironment for RFileScanner return null and not throw an error on env.getTableId() so something like: env.getPluginEnv().getConfiguration(env.getTableId()) can be done. Thoughts on this? ANSWERED

closes apache#4810 - Fixed some inconsistencies and bugs in (non-test) implementations of `IteratorEnvironment` - Fixed `RFileScanner`s unexpected `UnsupportedOperationException` when calling `env.getServiceEnv()` or `env.getConfig()` (both deprecated) or `env.getPluginEnv()` - Changed the impls to return the same object for calls to `getServiceEnv()`/`getPluginEnv()` (expected by `IteratorEnvIT`) - Fixed inconsistencies in functionality of `isUserCompaction()` and `isFullMajorCompaction()` when it is not a major compaction. Added missing javadoc to `isUserCompaction()` - Added tests for these environments to `IteratorEnvIT`

keith-turner

Still looking at this, posting comments I have so far. Will circle back to this later.

core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java

- Updated RFile javadocs - Added description of new supported functionality for `RFile.ScannerOptions.withTableProperties()` - Moved `@see` tag in javadoc for `RFile.{ScannerOptions/SummaryOptions}.withTableProperties(Map<String,String>)` which was incorrect - Changed `RFileScanner.IterEnv.getTableId()` to return a dummy table id to be used for calling `RFileScanner.IterEnv.getConfiguration(TableId)` - Added functionality for `RFileScanner.IterEnv.instantiate()` - Added error msg to invalid uses of `RFileScanner.IterEnv.getServiceEnv()/getPluginEnv()` methods

core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java

test/src/main/java/org/apache/accumulo/test/IteratorEnvIT.java

- Minor RFileScanner changes - Refactored IteratorEnvIT to ensure expected assertions are executed

keith-turner · 2024-08-26T22:34:46Z

core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java

+   * True if compaction was user initiated. Will throw IllegalStateException if
+   * {@link #getIteratorScope()} != {@link IteratorScope#majc}.


Suggested change

* True if compaction was user initiated. Will throw IllegalStateException if

* {@link #getIteratorScope()} != {@link IteratorScope#majc}.

* @return True if compaction was user initiated, false otherwise.

* @throws IllegalStateException if {@link #getIteratorScope()} != {@link IteratorScope#majc}

ctubbsii · 2024-09-04T16:21:11Z

core/src/main/java/org/apache/accumulo/core/client/rfile/RFile.java

+     * <p>
+     * Starting with {@code 2.1.4}, {@link PluginEnvironment#getConfiguration(TableId)} (obtained by
+     * {@link IteratorEnvironment#getPluginEnv()}) will return the properties passed in here.
+     *


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.

Okay, I will remove this in my next commit. Should I just remove the mention of 2.1.4 or that new part entirely?

ctubbsii · 2024-09-04T16:33:44Z

core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java

+      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);
+      }


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.

I implemented it here since I thought that because it was deprecated but not yet removed, there should still be functionality for the method. The other impls of IteratorEnvironment still have this implemented; assumed same should be the case here. Less importantly, this also made testing the different impls of IteratorEnvironment in IteratorEnvIT more straight forward as each would call getConfig(), getServiceEnv(), and getPluginEnv(). If this shouldn't be implemented here, I can remove it, or I could change the default impl to use getPluginEnv() like you said. Whichever you think would be better.

ctubbsii · 2024-09-04T16:42:45Z

core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java

-    public PluginEnvironment getPluginEnv() {
-      return new ClientServiceEnvironmentImpl(context.get());


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.

Thanks, this was something I was unsure about when implementing this. Some IteratorEnvironments had both getServiceEnv() and getPluginEnv() (e.g., OfflineIterator) while some had just getServiceEnv() (e.g., TabletIteratorEnvironment), and I wasn't sure which to go with. I'll fix the impls to use both.

ctubbsii · 2024-09-04T16:43:56Z

core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java

+  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");


Why wouldn't this be null?

Discussed here: #4816 (comment). Either null or a dummy would work though.

ctubbsii · 2024-09-04T16:51:23Z

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;


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

IteratorEnvironment doesn't have any implementation for getServiceEnv() or getPluginEnv(). Or are you suggesting changing IteratorEnvironment?

ctubbsii · 2024-09-04T16:58:19Z

core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java

+   * @return true if the compaction is a full major compaction; false otherwise
+   * @throws IllegalStateException if {@link #getIteratorScope()} != {@link IteratorScope#majc}.


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. Have you considered that as an alternative?

I hadn't looked into why an exception was thrown, I had just kept the functionality the same and consistent across impls. I assumed there was probably a reason for wanting all impls to throw an exception and didn't look any further.

Are you suggesting to drop the documentation about the exception case and revert the changes I made to adhere to the exception case, or are you suggesting changing the functionality to no longer throw the exception, or something else?

ctubbsii · 2024-09-11T20:14:33Z

After a conversation with @kevinrr888, I added commit 5924b02 to incorporate some of the changes we discussed, that he intends to build on and polish up. The main points included were:

Push a reasonable getConfig() implementation up into IteratorEnvironment's default method (depends on the ability to have getTableId() return a null value)
Include concrete implementations of getPluginEnv() instead of just getServiceEnv() to make it easier to just drop the deprecated one when merging to newer branches
Consider using a Proxy instance of TableId instead of a real instance, so it isn't possible to be confused with any similar user-constructed TableId (may not work)
Update javadocs in RFile to make it clearer how properties set on the scanner builder (such as iterators) will be available to client-side iterators from the plugin environment

The main thing left unresolved after our conversation was how to fix the mismatch between the implementations and the javadoc for all the methods that inconsistently return null or throw IllegalStateException or UnsupportedOperationException, when calling a method that doesn't apply to a given situation (like asking for a table ID or table name when operating directly on RFiles or asking if it's a user-initiated compaction when it's a scan). Nearly every caller seems to have a unique set a requirements/implementation, and it's a bit hard to home in on the best implementation, especially for the default methods, and make sure the javadoc matches the behavior.

- Changed ClientSideIteratorScanner.getConfig() to use the safer getPluginEnv() - Added getPluginEnv() impl to all IteratorEnvironments (besides test envs) - Improved RFile.ScannerOptions.withTableProperties() javadoc to better explain how iterators will have access to these properties - Opted for RFileScanner.IterEnv.getTableId() to return null instead of a dummy id - Removed incorrect impl of RFileScanner.IterEnv.get(Service/Plugin)Env().getTableName() to instead just throw UnsupOpExc since the method doesn't make sense anyways. Co-authored-by: Christopher Tubbs <[email protected]>

kevinrr888 · 2024-09-12T20:19:37Z

Some comments about decisions in a68ec11

Comment about `IteratorEnvironment.getConfig()`:

TLDR: opted for not having a default impl for IteratorEnvironment.getConfig() (keep default as throwing UOE)

It might be strange to have a default impl for this method but not others... If we do want a default impl for this that isnt just throwing a UOE, we should probably change the others too... But that should be done in a follow on if that is desired. Keeping the default as throwing a UOE keeps it consistent with the other default impls/more inline with the original approach (throw UOE by default, concrete impls should have an actual impl). The only IteratorEnvironment which would be using the default getConfig() would be ClientSideIteratorScanner. But we can just add the impl there (done in latest commit). It is also hard to know what would be a good default impl since its not exactly clear how getConfig() was originally working.

Comment about `RFileScanner.IterEnv.getTableId()`

TLDR: Opted for having RFileScanner.IterEnv.getTableId() return null

Proxy can only be used to create proxies for interfaces, not concrete classes, so can't be used for TableId. Opting to just return null with added javadoc.

Background: we want to support getConfiguration(TableId) since this is the method defined as returning the props set on the table and getConfiguration() should not be returning table props, so we cant substitute that method. So, something needs to be passed, but it doesn't matter what since it wont be used. The way getConfiguration(TableId) will be called is env.getPluginEnv().getConfiguration(env.getTableId()) (from javadocs for IteratorEnvironment.getPluginEnv()) So, we should have env.getTableId() returning something.

No solutions seem great, but Proxy is a no-go, a real dummy id may be confusing (e.g., a user may think that it is a real table id), null may lead to NPE. I figure null makes the most sense here. A NPE would only occur if the user is trying to do something with the table id which they shouldnt be doing anyways. It is also now noted in the javadocs that null will be returned so if a NPE does occur, it should be easy to see why.

Comment about UOE/ISE

Did not change anything further than I had already changed in my earlier commits relating to when/how UOE or ISE are thrown, how its documented etc. Some future considerations would be

IteratorEnvironment.getAuthorizations() currently documented as throwing UnsupportedOperationException if scope != scan (should probably be ISE if anything).
IteratorEnvironment.isFullMajorCompaction() and isUserCompaction() should maybe throw ISE in the default if the scope is majc
- Maybe these shouldn't be throwing exceptions at all and only returning false... This would probably be the cleanest if possible. Would have to look more into why these are throwing exceptions in the first place.

Comment about `RFileScanner`s `createServiceEnv()`

@ctubbsii suggested having this be an inner class instead of anon class. Leaving this comment as a reminder. Will do once the other more important changes are looked at; want to avoid confusing diff between commits for now.

kevinrr888 requested review from keith-turner and ctubbsii August 21, 2024 13:57

kevinrr888 self-assigned this Aug 21, 2024

keith-turner reviewed Aug 21, 2024

View reviewed changes

ctubbsii added this to the 2.1.4 milestone Aug 21, 2024

kevinrr888 linked an issue Aug 22, 2024 that may be closed by this pull request

RFileScanner's IteratorEnvironment throws UnsupportedOperationException for getConfig() and getServiceEnv() #4810

Open

keith-turner reviewed Aug 22, 2024

View reviewed changes

review changes:

85ae622

- Minor RFileScanner changes - Refactored IteratorEnvIT to ensure expected assertions are executed

keith-turner approved these changes Aug 26, 2024

View reviewed changes

kevinrr888 added 2 commits August 27, 2024 09:42

IteratorEnvironment javadoc fixes

3660d3a

Merge branch '2.1' into 2.1-feature-4810

242b520

ctubbsii reviewed Sep 4, 2024

View reviewed changes

kevinrr888 force-pushed the 2.1-feature-4810 branch from 5924b02 to a68ec11 Compare September 12, 2024 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`IteratorEnvironment` fixes #4816

`IteratorEnvironment` fixes #4816

kevinrr888 commented Aug 21, 2024 •

edited

Loading

keith-turner left a comment

keith-turner Aug 26, 2024

ctubbsii Sep 4, 2024

kevinrr888 Sep 5, 2024

ctubbsii Sep 4, 2024

kevinrr888 Sep 5, 2024

ctubbsii Sep 4, 2024

kevinrr888 Sep 5, 2024

ctubbsii Sep 4, 2024

kevinrr888 Sep 5, 2024

ctubbsii Sep 4, 2024

kevinrr888 Sep 5, 2024

ctubbsii Sep 4, 2024 •

edited

Loading

kevinrr888 Sep 5, 2024 •

edited

Loading

ctubbsii commented Sep 11, 2024

kevinrr888 commented Sep 12, 2024 •

edited

Loading

		* True if compaction was user initiated. Will throw IllegalStateException if
		* {@link #getIteratorScope()} != {@link IteratorScope#majc}.

		public PluginEnvironment getPluginEnv() {
		return new ClientServiceEnvironmentImpl(context.get());

		* @return true if the compaction is a full major compaction; false otherwise
		* @throws IllegalStateException if {@link #getIteratorScope()} != {@link IteratorScope#majc}.

IteratorEnvironment fixes #4816

Are you sure you want to change the base?

IteratorEnvironment fixes #4816

Conversation

kevinrr888 commented Aug 21, 2024 • edited Loading

keith-turner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctubbsii Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

kevinrr888 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

ctubbsii commented Sep 11, 2024

kevinrr888 commented Sep 12, 2024 • edited Loading

Comment about IteratorEnvironment.getConfig():

Comment about RFileScanner.IterEnv.getTableId()

Comment about UOE/ISE

Comment about RFileScanners createServiceEnv()

`IteratorEnvironment` fixes #4816

`IteratorEnvironment` fixes #4816

kevinrr888 commented Aug 21, 2024 •

edited

Loading

ctubbsii Sep 4, 2024 •

edited

Loading

kevinrr888 Sep 5, 2024 •

edited

Loading

kevinrr888 commented Sep 12, 2024 •

edited

Loading

Comment about `IteratorEnvironment.getConfig()`:

Comment about `RFileScanner.IterEnv.getTableId()`

Comment about `RFileScanner`s `createServiceEnv()`