kevinrr888 commented on PR #4816:
URL: https://github.com/apache/accumulo/pull/4816#issuecomment-2347164444
### Comment about `IteratorEnvironment.getConfig()`:
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()`
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()`
Christopher 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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
