PHOENIX-5645 - GlobalIndexChecker should prevent compaction from purg… #662
Conversation
gjacoby126
force-pushed
the
PHOENIX-5645
branch
from
29bc10e
to
d7450d2
Compare
|
Note that the new test I just pushed up to verify TTL behavior is currently failing. |
gjacoby126
force-pushed
the
PHOENIX-5645
branch
from
d7450d2
to
02cb7ba
Compare
| import org.apache.hadoop.hbase.regionserver.Region; | ||
| import org.apache.hadoop.hbase.regionserver.RegionScanner; | ||
| import org.apache.hadoop.hbase.regionserver.ScanInfo; |
There was a problem hiding this comment.
nit: are these imports used in this file?
There was a problem hiding this comment.
Thanks, removed them.
| } | ||
|
|
||
| @Override | ||
| public InternalScanner preCompactScannerOpen( |
There was a problem hiding this comment.
There is a lot of commonality in the function body of the above two hooks. Is it possible to throw that in a common function and invoke it from both the hooks?
There was a problem hiding this comment.
I thought of that, but they're just a bit different and some of the differences were in the middle, so I didn't see an easy way to consolidate. If you do please let me know -- always nice to eliminate dupes.
There was a problem hiding this comment.
I think it is more like readability vs DRY. We could put everything into a function and have a boolean passed but I think conditionals make things more complicated.
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/GlobalIndexCheckerIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java
Outdated
Show resolved
Hide resolved
| ScanInfo scanInfo = | ||
| ScanInfoUtil.getScanInfoForFlushesAndCompactions(conf, oldScanInfo, | ||
| store, scanType); | ||
| return new StoreScanner(store, scanInfo, scan, scanners, scanType, |
There was a problem hiding this comment.
unrelated to this PR. @gjacoby126 @abhishek-chouhan Looking at StoreScanner, seems like a scope to refactor the class into using the builder pattern. Any thoughts?
There was a problem hiding this comment.
The builder pattern would improve the StoreScanner's readability, yes, though since StoreScanner's IA.Private and really only supposed to be used within HBase, it would be to improve the HBase codebase more than Phoenix. Hopefully in the medium term we can get HBASE-23602 done so that we have a higher-level, supported way to do this.
phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/ScanInfoUtil.java
Outdated
Show resolved
Hide resolved
| ScanInfo oldScanInfo = store.getScanInfo(); | ||
|
|
||
| Configuration conf = c.getEnvironment().getConfiguration(); | ||
| //minor compactions and flushes both use "compact retain deletes" |
There was a problem hiding this comment.
as per the comment here, should line 440 also be in other hook?
There was a problem hiding this comment.
Compactions already set the scan type in the hook via the passed in parameter.
…ing very recently deleted cells
| try (Connection conn = DriverManager.getConnection(getUrl())) { | ||
| String dataTableName = generateUniqueName(); | ||
| String indexStem = generateUniqueName(); | ||
| createTableAndIndexes(conn, dataTableName, indexStem); |
There was a problem hiding this comment.
nit: You seem to be creating dataTable and indexes in each of these tests. Recommend you to move it to a @before method
There was a problem hiding this comment.
Unfortunately the tables are slightly different each time.
gjacoby126
force-pushed
the
PHOENIX-5645
branch
from
0773996
to
19d1841
Compare
…ing very recently deleted cells
…ing very recently deleted cells