-
Notifications
You must be signed in to change notification settings - Fork 993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHOENIX-6211 Paged scan filters #973
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial comments
@@ -1343,6 +1346,36 @@ public int prepareIndexMutations(Put put, Delete del, Map<byte[], List<Mutation> | |||
return indexMutations.size(); | |||
} | |||
|
|||
static boolean adjustScanFilter(Scan scan) { | |||
// For rebuilds we use count (*) as query for regular tables which ends up setting the FKOF on scan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please spell out FirstKeyOnlyFilter. Took me a minute to figure it out. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -1406,6 +1407,7 @@ public void testWithVariousSQLsForMultipleViews() throws Exception { | |||
} | |||
} | |||
|
|||
@Ignore("Fails with StaleRegionBoundaryCacheException. Mutations on a SCN connection could be the reason") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpisaac what are the implications of ignoring these tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gjacoby126 @kadirozde I have a fix for these test failures. It was failing in BaseScannerRegionObserver.preScannerOpen by throwing throwIfScanOutOfRegion when deletion of LocalIndexes are involved. All the ViewTTLIT tests are passing after the fix on my local machine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpisaac - are the test fixes already in so that @kadirozde can rebase on them, or still just local to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0001-Fixes-for-local-index-scans-and-additional-tests.patch.txt
@kadirozde @gjacoby126 I have attached the changes for the local index changes and some additional tests and added some comments on the PhoenixTTLRegionObserver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpisaac, Thank you very much. I will apply your patch soon.
if (!(scan.getFilter() instanceof PagedFilter)) { | ||
byte[] pageSizeMsBytes = scan.getAttribute(BaseScannerRegionObserver.SERVER_PAGE_SIZE_MS); | ||
if (pageSizeMsBytes != null) { | ||
scan.setFilter(new PagedFilter(scan.getFilter(), Bytes.toLong(pageSizeMsBytes)/2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why divided by 2? To allow for round trip time back to client? Good to have a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion.
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GlobalIndexRegionScanner.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/GlobalIndexRegionScanner.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixTTLRegionObserver.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixTTLRegionObserver.java
Outdated
Show resolved
Hide resolved
@@ -26,6 +27,9 @@ | |||
import org.apache.hadoop.hbase.util.Bytes; | |||
import org.apache.phoenix.hbase.index.util.GenericKeyValueBuilder; | |||
import org.apache.phoenix.util.PhoenixKeyValueUtil; | |||
import org.apache.phoenix.util.ScanUtil; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: seems like several unnecessary imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
💔 -1 overall
This message was automatically generated. |
} | ||
|
||
private boolean next(List<Cell> results, boolean raw) throws IOException { | ||
boolean hasMore = raw ? delegate.nextRaw(results) : delegate.next(results); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we throw an exception at either line 47 or line 56, do we clean up our state properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add exception handling here.
phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetResultIterator.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewTTLIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PagedRegionScanner.java
Show resolved
Hide resolved
@kadirozde - other than above comments lgtm. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thanks @kadirozde . Looks like you just need to rebase GlobalIndexChecker.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
The design doc is at https://docs.google.com/document/d/1Vt28i9JLQPG3lAnbW3RcO7fEUG4KT5AhyydLMiau59k/edit?usp=sharing