Skip to content
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.

Sign up for GitHub

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

Jump to bottom

HBASE-24637 - Reseek regression related to filter SKIP hinting #2663

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+96 −6
Open

HBASE-24637 - Reseek regression related to filter SKIP hinting #2663

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b286f2
Patch to improve SEEK vs NEXT performance based on config
ramkrish86 Nov 16, 2020
31e0de3
Logic mistake
ramkrish86 Dec 1, 2020
e8029c3
Make changes for RowCol cases and add some comments as Andrew suggested
ramkrish86 Dec 2, 2020
4654078
Apply this optimization for user scans only
ramkrish86 Dec 2, 2020
974e6cc
Add a new config and ensure that we honour that config
ramkrish86 Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ protected static class HFileScannerImpl implements HFileScanner {
private ByteBufferKeyOnlyKeyValue bufBackedKeyOnlyKv = new ByteBufferKeyOnlyKeyValue();
// A pair for reusing in blockSeek() so that we don't garbage lot of objects
final ObjectIntPair<ByteBuffer> pair = new ObjectIntPair<>();
private boolean seekToSameBlock;

/**
* The next indexed key is to keep track of the indexed key of the next data block.
Expand Down Expand Up @@ -385,6 +386,11 @@ public boolean isSeeked(){
return blockBuffer != null;
}

@Override
public boolean isSeekToSameBlock() {
return seekToSameBlock;
}

@Override
public String toString() {
return "HFileScanner for reader " + String.valueOf(getReader());
Expand Down Expand Up @@ -981,8 +987,11 @@ protected int loadBlockAndSeekToKey(HFileBlock seekToBlock, Cell nextIndexedKey,
Cell key, boolean seekBefore) throws IOException {
if (this.curBlock == null || this.curBlock.getOffset() != seekToBlock.getOffset()) {
updateCurrentBlock(seekToBlock);
seekToSameBlock = true;
} else if (rewind) {
blockBuffer.rewind();
} else {
seekToSameBlock = true;
}
// Update the nextIndexedKey
this.nextIndexedKey = nextIndexedKey;
Expand Down
7 changes: 7 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,13 @@ public interface HFileScanner extends Shipper, Closeable {
*/
Cell getNextIndexedKey();

/**
* @return true if we seeked to the current block(based on the seek key)
*/
default boolean isSeekToSameBlock() {
return false;
}

/**
* Close this HFile scanner and do necessary cleanup.
*/
Expand Down
5 changes: 5 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,11 @@ public Cell getNextIndexedKey() {
return current == null ? null : current.getNextIndexedKey();
}

@Override
public boolean isSeekToSameBlock() {
return current == null ? false : current.isSeekToSameBlock();
}

@Override
public void shipped() throws IOException {
for (KeyValueScanner scanner : this.scannersForDelayedClose) {
Expand Down
7 changes: 7 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,4 +181,11 @@ boolean requestSeek(Cell kv, boolean forward, boolean useBloom)
* see HFileWriterImpl#getMidpoint, or null if not known.
*/
public Cell getNextIndexedKey();

/**
* @return true if we seeked to the current block(based on the seek key)
*/
public default boolean isSeekToSameBlock() {
return false;
}
}
5 changes: 5 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,4 +575,9 @@ public Cell getNextIndexedKey() {
public void shipped() throws IOException {
this.hfs.shipped();
}

@Override
public boolean isSeekToSameBlock() {
return hfs.isSeekToSameBlock();
}
}
33 changes: 31 additions & 2 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ public class StoreScanner extends NonReversedNonLazyKeyValueScanner
private Cell prevCell = null;

private final long preadMaxBytes;
private final long switchToNextOnlyBytes;
private long bytesRead;
private boolean seekToSameBlock;

/** We don't ever expect to change this, the constant is just for clarity. */
static final boolean LAZY_SEEK_ENABLED_BY_DEFAULT = true;
Expand Down Expand Up @@ -205,6 +207,9 @@ private StoreScanner(HStore store, Scan scan, ScanInfo scanInfo,
this.scanUsePread = this.readType != Scan.ReadType.STREAM;
}
this.preadMaxBytes = scanInfo.getPreadMaxBytes();
// TODO : Introduce config here at the ScanInfo level. Determine based on number of blocks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be something deeply internal that no user could meaningfully set unless they read all the code and become a wizard. Please figure this out based on current configuration.

No new config!

// rather than bytes read?
this.switchToNextOnlyBytes = this.preadMaxBytes;
this.cellsPerHeartbeatCheck = scanInfo.getCellsPerTimeoutCheck();
// Parallel seeking is on if the config allows and more there is more than one store file.
if (store != null && store.getStorefilesCount() > 1) {
Expand Down Expand Up @@ -684,7 +689,7 @@ public boolean next(List<Cell> outResult, ScannerContext scannerContext) throws
matcher.clearCurrentRow();
seekOrSkipToNextRow(cell);
} else if (qcode == ScanQueryMatcher.MatchCode.INCLUDE_AND_SEEK_NEXT_COL) {
seekOrSkipToNextColumn(cell);
doSeekCol(cell);
} else {
this.heap.next();
}
Expand Down Expand Up @@ -728,7 +733,7 @@ public boolean next(List<Cell> outResult, ScannerContext scannerContext) throws
break;

case SEEK_NEXT_COL:
seekOrSkipToNextColumn(cell);
doSeekCol(cell);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it is not need to change this method name? You can add the new logic to seekOrSkipToNextColumn method directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok . I can see how can that be done.

NextState stateAfterSeekNextColumn = needToReturn(outResult);
if (stateAfterSeekNextColumn != null) {
return scannerContext.setScannerState(stateAfterSeekNextColumn).hasMoreValues();
Expand Down Expand Up @@ -786,6 +791,19 @@ private void updateMetricsStore(boolean memstoreRead) {
}
}

private void doSeekCol(Cell cell) throws IOException {
// we check when ever a seek_next_col happens did the seek really land in a new block.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this comment helps with understanding. (It doesn't for me, but could just be me...)
Rewrite it?

Questions to be answered by the comment:

  • What does this optimization do? Why next() instead of seekOrSkip...?
  • Is this the right layer to be doing this?
    • Why does this optimization have to be done here instead of down in the file scanner?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this optimization do? Why next() instead of seekOrSkip...?
Next is always good if we know we are going with the actual next block only. seekOrSkip involves loading the block and also doing a seek to the given key. Not only that the code to do that decision does lot of compares. That adds to the performance. Here we try to do a decision making with the fact that if the seekOrSkip has really landed in the actual next block only- we tend to take that as the pattern for the rest of the scan and go ahead with next and avoid all those loading of the block and seeking of the block.

Why does this optimization have to be done here instead of down in the file scanner?
The reason is that I felt that the trackers are deciding what the store scanner should be doing and hence asking to do a SEEK or NEXT. The storescanner layer is trying to do the actual smart work of deciding whether to really do a seek or next as per what the trackers are saying. Hence I added that logic at this layer.

// If the seek always lands in the same current block while trying to do a next,
// we tend to go with next() rather than seek() based on the 'seekToSameBlock'
// which is updated in the method 'seekOrSkipToNextColumn'
if (seekToSameBlock && bytesRead > switchToNextOnlyBytes) {
// forcefully make it to next only
this.heap.next();
} else {
seekOrSkipToNextColumn(cell);
}
}

/**
* If the top cell won't be flushed into disk, the new top cell may be
* changed after #reopenAfterFlush. Because the older top cell only exist
Expand Down Expand Up @@ -817,7 +835,13 @@ private void seekOrSkipToNextRow(Cell cell) throws IOException {

private void seekOrSkipToNextColumn(Cell cell) throws IOException {
if (!trySkipToNextColumn(cell)) {
boolean prevIndexKeyNull = (getNextIndexedKey() == null);
seekAsDirection(matcher.getKeyForNextColumn(cell));
if (prevIndexKeyNull) {
// even if one seek has lead to another block - reset to false.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be rewritten or removed. Ideally not removed, because you are trying to communicate something important for understanding how the code works. Try a rewrite?

// TODO : For SEEK_NEXT_ROW also?
seekToSameBlock = this.heap.isSeekToSameBlock();
}
}
}

Expand Down Expand Up @@ -1235,6 +1259,11 @@ public Cell getNextIndexedKey() {
return this.heap.getNextIndexedKey();
}

@Override
public boolean isSeekToSameBlock() {
return this.heap.isSeekToSameBlock();
}

@Override
public void shipped() throws IOException {
if (prevCell != null) {
Expand Down