[
https://issues.apache.org/jira/browse/PHOENIX-2628?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15152962#comment-15152962
]
James Taylor commented on PHOENIX-2628:
---------------------------------------
Thanks for the wip patch, [~rajeshbabu]. This is coming along nicely, but I
still think it's better to cut a new RC for 4.7.0 tomorrow without these
changes as they're big and potentially disruptive. Plus, there's more work to
do and we want to make sure it all gets done "right".
Here's some feedback:
- We'll need someone like [~apurtell] to review and bless
IndexHalfStoreFileReader and IndexHalfStoreFileReaderGenerator. For example, is
it copacetic to derive from {{StoreFile.Reader}}? Are public and/or evolving
APIs being used exclusively? If not, we'll need to come to an agreement/plan
with the HBase community for ones that aren't. It'd be a shame to do all this
work and end up in the same place we're in now.
- It looks like a lot of the complication comes from the handling splits/merges
during a query and that the only place I could find that reacts to this is in
ChunkedResultIterator (which is essentially deprecated after PHOENIX-1428 for
HBase 0.98.17+, 1.0.3+, 1.1.3+, and 1.2+). Is that correct and if so, is that
because a split that occurs in the first scan is already handled correctly? Can
a split occur while iterating through the rows from the first scan and if not,
why not?
- For an aggregate query that is grouping on a subset of the leading PK
columns, we're not holding the region lock for the entire scan. This would be
similar to the above situation, I believe.
- Wouldn't it be simpler/better to just support this new local indexing for the
above versions and simplify the implementation? Or is there some case where the
StaleRegionBoundaryCacheException is being handled on the client-side that I'm
not seeing?
I have more detailed feedback too for smaller things, like:
- This can be simpler, no? {{ScanUtil.isLocalIndex(scan)?true:false}}
- For the new TableResultIterator constructor, if you're including the
QueryPlan as an argument, then you can likely remove the TableRef argument and
get it from the QueryPlan instead.
- For the new QueryPlan method, instead of passing null through, why not pass
context.getScan() here:
{code}
@Override
public ResultIterator iterator(ParallelScanGrouper scanGrouper) throws
SQLException {
- ResultIterator iterator = new
DelegateResultIterator(delegate.iterator(scanGrouper)) {
+ return iterator(scanGrouper, null);
+ }
+
+ @Override
+ public ResultIterator iterator(ParallelScanGrouper scanGrouper, Scan scan)
throws SQLException {
+ ResultIterator iterator = new
DelegateResultIterator(delegate.iterator(scanGrouper, scan)) {
{code}
Then you can avoid a bunch of these kinds of checks:
{code}
+ this.scan = scan == null ? context.getScan() : scan;
{code}
> Ensure split when iterating through results handled correctly
> -------------------------------------------------------------
>
> Key: PHOENIX-2628
> URL: https://issues.apache.org/jira/browse/PHOENIX-2628
> Project: Phoenix
> Issue Type: Bug
> Reporter: James Taylor
> Assignee: Rajeshbabu Chintaguntla
> Attachments: PHOENIX-2628-wip.patch, PHOENIX-2628.patch
>
>
> We should start with a test case to ensure this works correctly, both for
> scans and aggregates.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
