Removed TabletIteratorEnvironment, replaced with smaller impl#5587
Removed TabletIteratorEnvironment, replaced with smaller impl#5587dlmarion merged 7 commits intoapache:2.1from
Conversation
In apache#5490 we created a ClientIteratorEnvironment builder and class. As part of that change we added a constructor to TabletIteratorEnvironment for use in testing. Issue apache#5503 was created to subsequently remove that constructor. This change removes TabletIteratorEnvironment entirely and replaces it with a subclass of ClientIteratorEnvironment. Closes apache#5503
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
I think the main concern I would have for these is whether or not they affect the public API or SPI. It does not appear that this change affects either. It's just an internal impl change. So, I think this change should be fine.
|
Full IT build successful |
|
|
||
| @Override | ||
| public boolean isFullMajorCompaction() { | ||
| if (getIteratorScope() != IteratorScope.majc) { |
There was a problem hiding this comment.
could this check be pushed to the super class?
|
|
||
| @Override | ||
| public boolean isUserCompaction() { | ||
| if (getIteratorScope() != IteratorScope.majc) { |
There was a problem hiding this comment.
could this check be pushed to the super class?
There was a problem hiding this comment.
In the parent class this checks that the scope is scan for some reason.
There was a problem hiding this comment.
When I remove this, then IteratorEnvIT fails at line 165. I' not sure why ClientIteratorEnvironment.isUserCompaction is checking for scope == scan. I'm going to look into this further as that would fix this issue.
There was a problem hiding this comment.
Ok, I resolved the disparity. In the client context, this throws an exception if the scope is scan. In the server context, this throws an exception if the scope is minc.
| @Override | ||
| public boolean isSamplingEnabled() { | ||
| if (getSamplerConfiguration() == null) { | ||
| return false; | ||
| } else { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Sample config could be present w/o sampling being enabled. Seems like this should just return if the boolean is set in the super class that indicates sampling is enabled.
| @Override | |
| public boolean isSamplingEnabled() { | |
| if (getSamplerConfiguration() == null) { | |
| return false; | |
| } else { | |
| return true; | |
| } | |
| } |
There was a problem hiding this comment.
Removed method in SystemIteratorEnvironmentImpl in bf54f5d
|
|
||
| @Deprecated(since = "2.0.0") | ||
| @Override | ||
| public AccumuloConfiguration getConfig() { |
There was a problem hiding this comment.
There will be a different configuration object impl returned for this methiod now that could have slightly different behavior. Not sure if this would ever matter.
There was a problem hiding this comment.
From what I can tell, all of the places where this was used, it's based off Context.getTableConfiguration(TableId).
|
Full IT build successful after the latest round of changes. |
SystemIteratorEnvironmentImpl was added in apache#5587, but did not implement the isRunningLowOnMemory method. It deferred to its parent class implementation, ClientIteratorEnvironment.isRunningLowOnMemory, which just returns false.
3 participants
In #5490 we created a ClientIteratorEnvironment builder and class. As part of that change we added a constructor to TabletIteratorEnvironment for use in testing. Issue #5503 was created to subsequently remove that constructor. This change removes TabletIteratorEnvironment entirely and replaces it with a subclass of ClientIteratorEnvironment.
Closes #5503