You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/05/27 21:57:33 UTC
[GitHub] [phoenix] swaroopak opened a new pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
swaroopak opened a new pull request #789:
URL: https://github.com/apache/phoenix/pull/789
…cation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r432646729
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForNonTxGlobalIndexIT.java
##########
@@ -588,9 +588,101 @@ public void testIndexToolForIncrementalRebuild() throws Exception {
}
}
+ @Test
+ public void testIndexToolForIncrementalVerify() throws Exception {
+ String schemaName = generateUniqueName();
+ String dataTableName = generateUniqueName();
+ String dataTableFullName = SchemaUtil.getTableName(schemaName, dataTableName);
+ String indexTableName = generateUniqueName();
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ conn.setAutoCommit(true);
+ conn.createStatement().execute("CREATE TABLE "+dataTableFullName+" "
+ + "(key1 BIGINT NOT NULL, key2 BIGINT NOT NULL, val1 VARCHAR, val2 BIGINT, "
+ + "val3 BIGINT, val4 DOUBLE, val5 BIGINT, val6 VARCHAR "
+ + "CONSTRAINT my_pk PRIMARY KEY(key1, key2)) "+tableDDLOptions);
+ conn.createStatement().execute(String.format(
+ "CREATE INDEX "+indexTableName+" ON "+dataTableFullName+" (val3) INCLUDE(val5)"));
+ long t0 = EnvironmentEdgeManager.currentTimeMillis();
Review comment:
Is the edge manager injected somewhere? Otherwise this seems prone to flapping if the wall clock milliseconds don't act quite the way you're expecting. Suggest either injecting a manual edge, or picking fixed timestamps for each time variable.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
##########
@@ -1081,11 +1086,39 @@ private RegionScanner rebuildIndices(final RegionScanner innerScanner, final Reg
rawScan.setRaw(true);
rawScan.setMaxVersions();
rawScan.getFamilyMap().clear();
- rawScan.setFilter(null);
rawScan.setCacheBlocks(false);
for (byte[] family : scan.getFamilyMap().keySet()) {
rawScan.addFamily(family);
}
+ rawScan.setFilter(null);
+ //override the filter to skip scan when lower bound of timerange is passed
Review comment:
More comments please about why we're scanning through everything twice. I figured it out (grabbing the keys that existed at minTime, then doing a skip scan of just those keys), but I had to stare at it for a few min. :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] swaroopak merged pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
swaroopak merged pull request #789:
URL: https://github.com/apache/phoenix/pull/789
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] swaroopak commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r437744234
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
+ incrScan.addFamily(family);
+ }
+ if(nextStartKey != null) {
+ incrScan.setStartRow(nextStartKey);
+ }
+ List<KeyRange> keys = new ArrayList<>();
+ try(RegionScanner scanner = region.getScanner(incrScan)) {
+ List<Cell> row = new ArrayList<>();
+ int rowCount = 0;
+ // collect row keys that have been modified in the given time-range
+ // up to the size of page to build skip scan filter
+ do {
+ hasMoreIncr = scanner.nextRaw(row);
+ if (!row.isEmpty()) {
+ keys.add(PVarbinary.INSTANCE.getKeyRange(CellUtil.cloneRow(row.get(0))));
+ rowCount++;
+ }
+ row.clear();
+ } while (hasMoreIncr && rowCount < pageSizeInRows);
+ }
+ if (!hasMoreIncr && keys.isEmpty()) {
+ return null;
+ }
+ if (!keys.isEmpty()) {
+ nextStartKey = ByteUtil.calculateTheClosestNextRowKeyForPrefix(keys.get(keys.size() - 1).getLowerRange());
+ }
+ try {
+ ScanRanges scanRanges = ScanRanges.createPointLookup(keys);
+ scanRanges.initializeScan(incrScan);
+ SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
+ incrScan.setFilter(new SkipScanFilter(skipScanFilter, true));
Review comment:
Yes, I will add a test for view index
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r437814858
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
+ incrScan.addFamily(family);
+ }
+ if(nextStartKey != null) {
+ incrScan.setStartRow(nextStartKey);
Review comment:
This is at the HBase level and all row keys are sorted in the ascending order. Phoenix flips the bits in row keys for DESC pks. This should be a concern at the Phoenix level but not here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r437817866
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
+ incrScan.addFamily(family);
+ }
+ if(nextStartKey != null) {
+ incrScan.setStartRow(nextStartKey);
+ }
+ List<KeyRange> keys = new ArrayList<>();
+ try(RegionScanner scanner = region.getScanner(incrScan)) {
+ List<Cell> row = new ArrayList<>();
+ int rowCount = 0;
+ // collect row keys that have been modified in the given time-range
+ // up to the size of page to build skip scan filter
+ do {
+ hasMoreIncr = scanner.nextRaw(row);
+ if (!row.isEmpty()) {
+ keys.add(PVarbinary.INSTANCE.getKeyRange(CellUtil.cloneRow(row.get(0))));
+ rowCount++;
+ }
+ row.clear();
+ } while (hasMoreIncr && rowCount < pageSizeInRows);
+ }
+ if (!hasMoreIncr && keys.isEmpty()) {
+ return null;
+ }
+ if (!keys.isEmpty()) {
+ nextStartKey = ByteUtil.calculateTheClosestNextRowKeyForPrefix(keys.get(keys.size() - 1).getLowerRange());
+ }
+ try {
+ ScanRanges scanRanges = ScanRanges.createPointLookup(keys);
+ scanRanges.initializeScan(incrScan);
+ SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
+ incrScan.setFilter(new SkipScanFilter(skipScanFilter, true));
+ //putting back the min time to 0 for index and data reads
+ incrScan.setTimeRange(0, scan.getTimeRange().getMax());
+ scan.setTimeRange(0, scan.getTimeRange().getMax());
+ return region.getScanner(incrScan);
+ } catch (IndexOutOfBoundsException ex) {
Review comment:
You can eliminate IndexOutOfBoundsException by checking if key.size() == 0 and if so, returning innerScanner.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r437714322
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
+ incrScan.addFamily(family);
+ }
+ if(nextStartKey != null) {
+ incrScan.setStartRow(nextStartKey);
Review comment:
If we don't have a test for index rebuild with a DESC pk, (incremental or not), we really should.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r437813724
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
Review comment:
We can have families in raw scans but not column qualifiers as HBase returns exception when columns are specified.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] swaroopak commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r437778774
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
+ incrScan.addFamily(family);
+ }
+ if(nextStartKey != null) {
+ incrScan.setStartRow(nextStartKey);
Review comment:
Yes, that's the idea. Let me poke around if there is a test on DESC pk.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #789: PHOENIX-5783: Implement starttime in IndexTool for rebuild and verifi…
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #789:
URL: https://github.com/apache/phoenix/pull/789#discussion_r437648684
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -118,6 +118,9 @@
private IndexVerificationOutputRepository verificationOutputRepository;
private boolean skipped = false;
private boolean shouldVerifyCheckDone = false;
+ private byte[] nextStartKey;
+ private boolean hasMoreIncr = false;
+ private long minTimestamp = 0 ;
Review comment:
Looks like minTimestamp is just set once from the Scan (or 0 if no verify type), so can this be final?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
Review comment:
curious why we're clearing the families set by copying the original Scan and then adding them back?
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolForNonTxGlobalIndexIT.java
##########
@@ -587,6 +589,113 @@ public void testIndexToolForIncrementalRebuild() throws Exception {
}
}
+ @Test
+ public void testIndexToolForIncrementalVerify() throws Exception {
Review comment:
Let's also have a test where we incrementally rebuild a view index. Since the copy-constructor of Scan also copies over filters, I _expect_ they're OK, but better to test rather than assume. :-)
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
+ incrScan.addFamily(family);
+ }
+ if(nextStartKey != null) {
+ incrScan.setStartRow(nextStartKey);
Review comment:
Even if a table is sorted DESC, it's ok to go from the start of the table down to the end so long as we're consistent about order and get every row we need, right? (I think so, but want to ask because we've had a lot of bugs over the years from hidden assumptions about sorting)
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/IndexRebuildRegionScanner.java
##########
@@ -1398,7 +1410,64 @@ public boolean next(List<Cell> results) throws IOException {
SINGLE_COLUMN, AGG_TIMESTAMP, rowCountBytes, 0, rowCountBytes.length);
}
results.add(aggKeyValue);
- return hasMore;
+ return hasMore || hasMoreIncr;
+ }
+
+ private RegionScanner getLocalScanner() throws IOException {
+ // override the filter to skip scan and open new scanner
+ // when lower bound of timerange is passed or newStartKey was populated
+ // from previous call to next()
+ if(minTimestamp!= 0) {
+ Scan incrScan = new Scan(scan);
+ incrScan.setTimeRange(minTimestamp, scan.getTimeRange().getMax());
+ incrScan.setRaw(true);
+ incrScan.setMaxVersions();
+ incrScan.getFamilyMap().clear();
+ incrScan.setCacheBlocks(false);
+ for (byte[] family : scan.getFamilyMap().keySet()) {
+ incrScan.addFamily(family);
+ }
+ if(nextStartKey != null) {
+ incrScan.setStartRow(nextStartKey);
+ }
+ List<KeyRange> keys = new ArrayList<>();
+ try(RegionScanner scanner = region.getScanner(incrScan)) {
+ List<Cell> row = new ArrayList<>();
+ int rowCount = 0;
+ // collect row keys that have been modified in the given time-range
+ // up to the size of page to build skip scan filter
+ do {
+ hasMoreIncr = scanner.nextRaw(row);
+ if (!row.isEmpty()) {
+ keys.add(PVarbinary.INSTANCE.getKeyRange(CellUtil.cloneRow(row.get(0))));
+ rowCount++;
+ }
+ row.clear();
+ } while (hasMoreIncr && rowCount < pageSizeInRows);
+ }
+ if (!hasMoreIncr && keys.isEmpty()) {
+ return null;
+ }
+ if (!keys.isEmpty()) {
+ nextStartKey = ByteUtil.calculateTheClosestNextRowKeyForPrefix(keys.get(keys.size() - 1).getLowerRange());
+ }
+ try {
+ ScanRanges scanRanges = ScanRanges.createPointLookup(keys);
+ scanRanges.initializeScan(incrScan);
+ SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
+ incrScan.setFilter(new SkipScanFilter(skipScanFilter, true));
Review comment:
What if there's already a Filter on that scan (such as if we're rebuilding a view index). I _think_ that's OK, because we generated our key list for the SkipScanFilter using the pre-existing view filter on incrScan which we got from the original rebuild Scan, but let's make sure we test and are really sure about that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org