HADOOP-19622: [ABFS][ReadAheadV2] Implement Read Buffer Manager V2 with improved aggressiveness #7832
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| // while waiting, so no one will be able to change any state. If this becomes more complex in the future, | ||
| // then the latch cane be removed and replaced with wait/notify whenever getInProgressList() is touched. | ||
| } catch (InterruptedException ex) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
In the catch block- we're interrupting the thread but not informing the caller. Is it expected?
There was a problem hiding this comment.
It will be thrown to caller.
| // getInProgressList(). So this latch is safe to be outside the synchronized block. | ||
| // Putting it in synchronized would result in a deadlock, since this thread would be holding the lock | ||
| // while waiting, so no one will be able to change any state. If this becomes more complex in the future, | ||
| // then the latch cane be removed and replaced with wait/notify whenever getInProgressList() is touched. |
| int cursor = (int) (position - buf.getOffset()); | ||
| int availableLengthInBuffer = buf.getLength() - cursor; | ||
| int lengthToCopy = Math.min(length, availableLengthInBuffer); | ||
| System.arraycopy(buf.getBuffer(), cursor, buffer, 0, lengthToCopy); |
There was a problem hiding this comment.
we can skip it for cases when lengthToCopy = 0
There was a problem hiding this comment.
For length 0 we won't even read.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| LOG.debug("issuing read ahead requestedOffset = {} requested size {}", | ||
| nextOffset, nextSize); | ||
| readBufferManager.queueReadAhead(this, nextOffset, (int) nextSize, | ||
| getReadBufferManager().queueReadAhead(this, nextOffset, (int) nextSize, |
There was a problem hiding this comment.
Needs changes in constructor to return an instance of readbuffermanager v2 if enabled
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
… into RBMV2_HADOOP-19622
This comment was marked as outdated.
This comment was marked as outdated.
============================================================
|
|
💔 -1 overall
This message was automatically generated. |
|
Thanks everyone for reviewing this. Spotbugs and Javadocs warnings are due to https://issues.apache.org/jira/browse/HADOOP-19731 |
5 participants
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-19622
Implementing ReadBufferManagerV2 as per the new design document.
Following capabilities are added to ReadBufferManager:
For more details on design doc please refer to the design doc attached to parent JIRA: https://issues.apache.org/jira/browse/HADOOP-19596
How was this patch tested?
TBA