Add fadvise:AUTO_RANDOM mode #1243
Conversation
|
/gcbrun |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## branch-2.2.x #1243 +/- ##
==================================================
- Coverage 80.84% 80.74% -0.10%
- Complexity 2418 2433 +15
==================================================
Files 167 168 +1
Lines 10815 10871 +56
Branches 1197 1208 +11
==================================================
+ Hits 8743 8778 +35
- Misses 1544 1557 +13
- Partials 528 536 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return; | ||
| } | ||
| if (readOptions.getFadvise() == Fadvise.AUTO_RANDOM) { | ||
| if (isSequentialAccessPattern(currentPosition)) { |
There was a problem hiding this comment.
will it only switch from sequential -> random once and not go back to sequential? Is this intentional?
There was a problem hiding this comment.
Yeah, it is as per the request as mentioned in the document AUTO_RANDOM starts assuming "RANDOM" file access and adapts to "SEQUENTIAL" if read pattern is so. Once it's adapted it will not flip again.
gcsio/src/main/java/com/google/cloud/hadoop/gcsio/AdaptiveFileAccessPattern.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| public AdaptiveFileAccessPattern( |
There was a problem hiding this comment.
I think this class is not a "Pattern", but rather a "PatternTracker". Why not name this accordingly.
There was a problem hiding this comment.
We don't track the pattern of access but decide on pattern which is derived by tracking the requests. I agrees that it's also not a "Pattern". Will change it to AccessPatternManager or something on those lines.
gcsio/src/main/java/com/google/cloud/hadoop/gcsio/AdaptiveFileAccessPattern.java
Outdated
Show resolved
Hide resolved
| consecutiveRequestsDistances.add(currentPosition - lastServedIndex); | ||
| } | ||
|
|
||
| if (!shouldDetectSequentialAccess()) { |
There was a problem hiding this comment.
Why not have this as the first check?
There was a problem hiding this comment.
It will help populate the distance accurately. I do agree that it will not be used if shouldDetectSequentialAccess returns true. I find it to be helpful in maintaining the correct state.
gcsio/src/main/java/com/google/cloud/hadoop/gcsio/AdaptiveFileAccessPattern.java
Outdated
Show resolved
Hide resolved
| this.randomAccess = readOptions.getFadvise() == Fadvise.RANDOM; | ||
| this.fileAccessPattern = new AdaptiveFileAccessPattern(resourceId, readOptions); | ||
| if (gzipEncoded) { | ||
| fileAccessPattern.overrideAccessPattern(false); |
There was a problem hiding this comment.
Why not have this as part of constructor. That way we can make the overRideAccessPattern final. Even better have a dummy noop FileAccessPattern for this case.
There was a problem hiding this comment.
It is part of the constructor.
Only reason fileAccessPattern is not final is because we are cleaning up the state maintained in it during close.
There was a problem hiding this comment.
Have a set method breaks the encapsulation. I think in this case it can be easily avoided.
There was a problem hiding this comment.
I understand.
We have two scenarios in one we will know the gzip info before hand and in other we will get to know about it at later point hence the fn:overrideAccessPattern.
gcsio/src/main/java/com/google/cloud/hadoop/gcsio/AdaptiveFileAccessPattern.java
Outdated
Show resolved
Hide resolved
gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageReadChannel.java
Show resolved
Hide resolved
No description provided.