bbeaudreault commented on code in PR #5373:
URL: https://github.com/apache/hbase/pull/5373#discussion_r1385321321
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java:
##########
@@ -486,52 +500,171 @@ public boolean shouldUseScanner(Scan scan, HStore store,
long oldestUnexpiredTS)
@Override
public boolean seekToPreviousRow(Cell originalKey) throws IOException {
try {
- try {
- boolean keepSeeking = false;
- Cell key = originalKey;
- do {
- Cell seekKey = PrivateCellUtil.createFirstOnRow(key);
- if (seekCount != null) seekCount.increment();
- if (!hfs.seekBefore(seekKey)) {
- this.cur = null;
- return false;
- }
- Cell curCell = hfs.getCell();
- Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(curCell);
-
- if (seekCount != null) seekCount.increment();
- if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
- this.cur = null;
- return false;
- }
-
- setCurrentCell(hfs.getCell());
- this.stopSkippingKVsIfNextRow = true;
- boolean resultOfSkipKVs;
- try {
- resultOfSkipKVs = skipKVsNewerThanReadpoint();
- } finally {
- this.stopSkippingKVsIfNextRow = false;
- }
- if (!resultOfSkipKVs || getComparator().compareRows(cur,
firstKeyOfPreviousRow) > 0) {
- keepSeeking = true;
- key = firstKeyOfPreviousRow;
- continue;
- } else {
- keepSeeking = false;
- }
- } while (keepSeeking);
- return true;
- } finally {
- realSeekDone = true;
+ if (isFastSeekingEncoding) {
+ return seekToPreviousRowStateless(originalKey);
+ } else if (previousRow == null ||
getComparator().compareRows(previousRow, originalKey) > 0) {
+ return seekToPreviousRowWithoutHint(originalKey);
+ } else {
+ return seekToPreviousRowWithHint(originalKey);
}
} catch (FileNotFoundException e) {
throw e;
} catch (IOException ioe) {
throw new IOException("Could not seekToPreviousRow " + this + " to key "
+ originalKey, ioe);
+ } finally {
+ this.realSeekDone = true;
}
}
+ private boolean seekToPreviousRowWithHint(Cell originalKey) throws
IOException {
+ do {
+ if (previousRow == null) {
+ return seekToPreviousRowWithoutHint(originalKey);
+ }
+
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(previousRow);
+ if (!seekBeforeAndSaveKeyToPreviousRow(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (!reseekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ setReadpointAndSkipNewerKvs()
+ && getComparator().compareRows(cur, firstKeyOfPreviousRow) <= 0
+ ) {
+ return true;
+ }
+ } while (true);
+ }
+
+ private boolean seekToPreviousRowWithoutHint(Cell originalKey) throws
IOException {
+ boolean keepSeeking;
+ Cell key = originalKey;
+ do {
+ // Rewind to the cell before the beginning of this row
+ Cell keyAtBeginningOfRow = PrivateCellUtil.createFirstOnRow(key);
+ if (!seekBefore(keyAtBeginningOfRow)) {
+ return false;
+ }
+
+ // Rewind before this row and save what we find as a seek hint
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(hfs.getCell());
+ if (!seekBeforeAndSaveKeyToPreviousRow(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ // Seek back to the start of the previous row
+ if (!reseekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ !setReadpointAndSkipNewerKvs()
+ || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0
+ ) {
+ keepSeeking = true;
+ key = firstKeyOfPreviousRow;
+ } else {
+ keepSeeking = false;
+ }
+ } while (keepSeeking);
+ return true;
+ }
+
+ /**
+ * This seekToPreviousRow method requires two seeks from the beginning of a
block. It should be
+ * used if the cost for seeking to the beginning of a block is low.
+ */
+ private boolean seekToPreviousRowStateless(Cell originalKey) throws
IOException {
+ boolean keepSeeking;
+ Cell key = originalKey;
+ do {
+ Cell keyAtBeginningOfRow = PrivateCellUtil.createFirstOnRow(key);
+ if (!seekBefore(keyAtBeginningOfRow)) {
+ return false;
+ }
+
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(hfs.getCell());
+ if (!seekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ !setReadpointAndSkipNewerKvs()
+ || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0
+ ) {
+ keepSeeking = true;
+ key = firstKeyOfPreviousRow;
+ } else {
+ keepSeeking = false;
+ }
+ } while (keepSeeking);
+ return true;
+ }
+
+ private boolean seekBefore(Cell seekKey) throws IOException {
+ if (seekCount != null) seekCount.increment();
+ if (!hfs.seekBefore(seekKey)) {
+ this.cur = null;
+ return false;
+ }
+
+ return true;
+ }
+
+ private boolean seekBeforeAndSaveKeyToPreviousRow(Cell seekKey) throws
IOException {
+ if (seekCount != null) seekCount.increment();
+ if (!hfs.seekBefore(seekKey)) {
+ // Since the above seek failed, we need to position ourselves back at
the start of the
+ // block or else our reseek might fail
+ if (!hfs.seekTo()) {
Review Comment:
im not sure this fallback is necessary. according to javadoc, seekBefore
should only return false if seekKey is <= the first key in the storefile. In
that case, it's unlikely for the seekTo here to return false -- it only returns
false if the file is empty, but the file can't be empty if we got here (we
always will have succeeded at doing at least one seekBefore before calling this
method).
Is there a valid case where the seekKey is <= start of file, and it makes
sense to reset to the start of the file? What would we seek to? I think we'd
end up re-calling this method again with the same seekKey.
I have a feeling that we should just return false and set cur to null if the
seekBefore returns false. It might be helpful to have a test for this case?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java:
##########
@@ -486,52 +500,171 @@ public boolean shouldUseScanner(Scan scan, HStore store,
long oldestUnexpiredTS)
@Override
public boolean seekToPreviousRow(Cell originalKey) throws IOException {
try {
- try {
- boolean keepSeeking = false;
- Cell key = originalKey;
- do {
- Cell seekKey = PrivateCellUtil.createFirstOnRow(key);
- if (seekCount != null) seekCount.increment();
- if (!hfs.seekBefore(seekKey)) {
- this.cur = null;
- return false;
- }
- Cell curCell = hfs.getCell();
- Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(curCell);
-
- if (seekCount != null) seekCount.increment();
- if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
- this.cur = null;
- return false;
- }
-
- setCurrentCell(hfs.getCell());
- this.stopSkippingKVsIfNextRow = true;
- boolean resultOfSkipKVs;
- try {
- resultOfSkipKVs = skipKVsNewerThanReadpoint();
- } finally {
- this.stopSkippingKVsIfNextRow = false;
- }
- if (!resultOfSkipKVs || getComparator().compareRows(cur,
firstKeyOfPreviousRow) > 0) {
- keepSeeking = true;
- key = firstKeyOfPreviousRow;
- continue;
- } else {
- keepSeeking = false;
- }
- } while (keepSeeking);
- return true;
- } finally {
- realSeekDone = true;
+ if (isFastSeekingEncoding) {
+ return seekToPreviousRowStateless(originalKey);
+ } else if (previousRow == null ||
getComparator().compareRows(previousRow, originalKey) > 0) {
+ return seekToPreviousRowWithoutHint(originalKey);
+ } else {
+ return seekToPreviousRowWithHint(originalKey);
}
} catch (FileNotFoundException e) {
throw e;
} catch (IOException ioe) {
throw new IOException("Could not seekToPreviousRow " + this + " to key "
+ originalKey, ioe);
+ } finally {
+ this.realSeekDone = true;
}
}
+ private boolean seekToPreviousRowWithHint(Cell originalKey) throws
IOException {
Review Comment:
Can you add javadoc to the 3 main new methods here? It'd be helpful to note:
- WithHint -- does 1 seek and 1 reseek. Maintains state, which only makes
sense in the context of a sequential row-by-row reverse scan. previousRow
should be reset if that will not be the case.
- WithoutHint -- does 2 seeks and 1 reseek. The extra expense is with the
intent of speeding up subsequent calls to WithHint.
- Stateless -- does 2 seeks
- Seek is slower because it needs to start from the beginning of the file,
while reseek goes forward from the current position.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java:
##########
@@ -486,52 +500,171 @@ public boolean shouldUseScanner(Scan scan, HStore store,
long oldestUnexpiredTS)
@Override
public boolean seekToPreviousRow(Cell originalKey) throws IOException {
try {
- try {
- boolean keepSeeking = false;
- Cell key = originalKey;
- do {
- Cell seekKey = PrivateCellUtil.createFirstOnRow(key);
- if (seekCount != null) seekCount.increment();
- if (!hfs.seekBefore(seekKey)) {
- this.cur = null;
- return false;
- }
- Cell curCell = hfs.getCell();
- Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(curCell);
-
- if (seekCount != null) seekCount.increment();
- if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
- this.cur = null;
- return false;
- }
-
- setCurrentCell(hfs.getCell());
- this.stopSkippingKVsIfNextRow = true;
- boolean resultOfSkipKVs;
- try {
- resultOfSkipKVs = skipKVsNewerThanReadpoint();
- } finally {
- this.stopSkippingKVsIfNextRow = false;
- }
- if (!resultOfSkipKVs || getComparator().compareRows(cur,
firstKeyOfPreviousRow) > 0) {
- keepSeeking = true;
- key = firstKeyOfPreviousRow;
- continue;
- } else {
- keepSeeking = false;
- }
- } while (keepSeeking);
- return true;
- } finally {
- realSeekDone = true;
+ if (isFastSeekingEncoding) {
+ return seekToPreviousRowStateless(originalKey);
+ } else if (previousRow == null ||
getComparator().compareRows(previousRow, originalKey) > 0) {
+ return seekToPreviousRowWithoutHint(originalKey);
+ } else {
+ return seekToPreviousRowWithHint(originalKey);
}
} catch (FileNotFoundException e) {
throw e;
} catch (IOException ioe) {
throw new IOException("Could not seekToPreviousRow " + this + " to key "
+ originalKey, ioe);
+ } finally {
+ this.realSeekDone = true;
}
}
+ private boolean seekToPreviousRowWithHint(Cell originalKey) throws
IOException {
+ do {
+ if (previousRow == null) {
+ return seekToPreviousRowWithoutHint(originalKey);
+ }
+
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(previousRow);
+ if (!seekBeforeAndSaveKeyToPreviousRow(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (!reseekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ setReadpointAndSkipNewerKvs()
+ && getComparator().compareRows(cur, firstKeyOfPreviousRow) <= 0
+ ) {
+ return true;
+ }
+ } while (true);
+ }
+
+ private boolean seekToPreviousRowWithoutHint(Cell originalKey) throws
IOException {
+ boolean keepSeeking;
+ Cell key = originalKey;
+ do {
+ // Rewind to the cell before the beginning of this row
+ Cell keyAtBeginningOfRow = PrivateCellUtil.createFirstOnRow(key);
+ if (!seekBefore(keyAtBeginningOfRow)) {
+ return false;
+ }
+
+ // Rewind before this row and save what we find as a seek hint
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(hfs.getCell());
+ if (!seekBeforeAndSaveKeyToPreviousRow(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ // Seek back to the start of the previous row
+ if (!reseekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ !setReadpointAndSkipNewerKvs()
+ || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0
+ ) {
+ keepSeeking = true;
+ key = firstKeyOfPreviousRow;
+ } else {
+ keepSeeking = false;
Review Comment:
for this and the below method, could we follow the same syntax as the above
method?
```java
do {
...
if (
setReadpointAndSkipNewerKvs()
&& getComparator().compareRows(cur, firstKeyOfPreviousRow) <= 0
) {
return true;
}
key = firstKeyOfPreviousRow;
} while (true);
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java:
##########
@@ -486,52 +500,171 @@ public boolean shouldUseScanner(Scan scan, HStore store,
long oldestUnexpiredTS)
@Override
public boolean seekToPreviousRow(Cell originalKey) throws IOException {
try {
- try {
- boolean keepSeeking = false;
- Cell key = originalKey;
- do {
- Cell seekKey = PrivateCellUtil.createFirstOnRow(key);
- if (seekCount != null) seekCount.increment();
- if (!hfs.seekBefore(seekKey)) {
- this.cur = null;
- return false;
- }
- Cell curCell = hfs.getCell();
- Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(curCell);
-
- if (seekCount != null) seekCount.increment();
- if (!seekAtOrAfter(hfs, firstKeyOfPreviousRow)) {
- this.cur = null;
- return false;
- }
-
- setCurrentCell(hfs.getCell());
- this.stopSkippingKVsIfNextRow = true;
- boolean resultOfSkipKVs;
- try {
- resultOfSkipKVs = skipKVsNewerThanReadpoint();
- } finally {
- this.stopSkippingKVsIfNextRow = false;
- }
- if (!resultOfSkipKVs || getComparator().compareRows(cur,
firstKeyOfPreviousRow) > 0) {
- keepSeeking = true;
- key = firstKeyOfPreviousRow;
- continue;
- } else {
- keepSeeking = false;
- }
- } while (keepSeeking);
- return true;
- } finally {
- realSeekDone = true;
+ if (isFastSeekingEncoding) {
+ return seekToPreviousRowStateless(originalKey);
+ } else if (previousRow == null ||
getComparator().compareRows(previousRow, originalKey) > 0) {
+ return seekToPreviousRowWithoutHint(originalKey);
+ } else {
+ return seekToPreviousRowWithHint(originalKey);
}
} catch (FileNotFoundException e) {
throw e;
} catch (IOException ioe) {
throw new IOException("Could not seekToPreviousRow " + this + " to key "
+ originalKey, ioe);
+ } finally {
+ this.realSeekDone = true;
}
}
+ private boolean seekToPreviousRowWithHint(Cell originalKey) throws
IOException {
+ do {
+ if (previousRow == null) {
+ return seekToPreviousRowWithoutHint(originalKey);
+ }
+
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(previousRow);
+ if (!seekBeforeAndSaveKeyToPreviousRow(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (!reseekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ setReadpointAndSkipNewerKvs()
+ && getComparator().compareRows(cur, firstKeyOfPreviousRow) <= 0
+ ) {
+ return true;
+ }
+ } while (true);
+ }
+
+ private boolean seekToPreviousRowWithoutHint(Cell originalKey) throws
IOException {
+ boolean keepSeeking;
+ Cell key = originalKey;
+ do {
+ // Rewind to the cell before the beginning of this row
+ Cell keyAtBeginningOfRow = PrivateCellUtil.createFirstOnRow(key);
+ if (!seekBefore(keyAtBeginningOfRow)) {
+ return false;
+ }
+
+ // Rewind before this row and save what we find as a seek hint
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(hfs.getCell());
+ if (!seekBeforeAndSaveKeyToPreviousRow(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ // Seek back to the start of the previous row
+ if (!reseekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ !setReadpointAndSkipNewerKvs()
+ || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0
+ ) {
+ keepSeeking = true;
+ key = firstKeyOfPreviousRow;
+ } else {
+ keepSeeking = false;
+ }
+ } while (keepSeeking);
+ return true;
+ }
+
+ /**
+ * This seekToPreviousRow method requires two seeks from the beginning of a
block. It should be
+ * used if the cost for seeking to the beginning of a block is low.
+ */
+ private boolean seekToPreviousRowStateless(Cell originalKey) throws
IOException {
+ boolean keepSeeking;
+ Cell key = originalKey;
+ do {
+ Cell keyAtBeginningOfRow = PrivateCellUtil.createFirstOnRow(key);
+ if (!seekBefore(keyAtBeginningOfRow)) {
+ return false;
+ }
+
+ Cell firstKeyOfPreviousRow =
PrivateCellUtil.createFirstOnRow(hfs.getCell());
+ if (!seekAtOrAfter(firstKeyOfPreviousRow)) {
+ return false;
+ }
+
+ if (
+ !setReadpointAndSkipNewerKvs()
+ || getComparator().compareRows(cur, firstKeyOfPreviousRow) > 0
+ ) {
+ keepSeeking = true;
+ key = firstKeyOfPreviousRow;
+ } else {
+ keepSeeking = false;
+ }
+ } while (keepSeeking);
+ return true;
+ }
+
+ private boolean seekBefore(Cell seekKey) throws IOException {
+ if (seekCount != null) seekCount.increment();
+ if (!hfs.seekBefore(seekKey)) {
+ this.cur = null;
+ return false;
+ }
+
+ return true;
+ }
+
+ private boolean seekBeforeAndSaveKeyToPreviousRow(Cell seekKey) throws
IOException {
+ if (seekCount != null) seekCount.increment();
+ if (!hfs.seekBefore(seekKey)) {
+ // Since the above seek failed, we need to position ourselves back at
the start of the
+ // block or else our reseek might fail
+ if (!hfs.seekTo()) {
+ this.cur = null;
+ return false;
+ }
+ this.previousRow = null;
+ } else {
+ this.previousRow = hfs.getCell();
+ }
+
+ return true;
+ }
+
+ private boolean seekAtOrAfter(Cell seekKey) throws IOException {
+ if (seekCount != null) seekCount.increment();
+ if (!seekAtOrAfter(hfs, seekKey)) {
+ this.cur = null;
+ return false;
+ }
+
+ return true;
+ }
+
+ private boolean reseekAtOrAfter(Cell seekKey) throws IOException {
+ if (seekCount != null) seekCount.increment();
+ if (!reseekAtOrAfter(hfs, seekKey)) {
+ this.cur = null;
+ return false;
+ }
+
+ return true;
+ }
+
+ private boolean setReadpointAndSkipNewerKvs() throws IOException {
Review Comment:
this method name is misleading -- readpoint here refers to memstore readPt,
and this method is not setting that. Maybe just move the `setCurrentCell` call
into each of the above methods, and call this
`setReadpointAndSkipNewerKvsReversed()` or something.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
