-
Notifications
You must be signed in to change notification settings - Fork 237
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.
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
Add AUTO_RANDOM mode #1242
base: branch-2.2.x
Are you sure you want to change the base?
Add AUTO_RANDOM mode #1242
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,9 @@ | |
import java.nio.channels.ReadableByteChannel; | ||
import java.nio.channels.SeekableByteChannel; | ||
import java.util.ArrayList; | ||
import java.util.LinkedList; | ||
import java.util.List; | ||
import java.util.ListIterator; | ||
import javax.annotation.Nullable; | ||
|
||
/** Provides seekable read access to GCS via java-storage library. */ | ||
|
@@ -210,14 +212,41 @@ | |
// in-place seeks. | ||
private byte[] skipBuffer = null; | ||
private ReadableByteChannel byteChannel = null; | ||
// Keeps track of distance between last 2 consecutive request. | ||
private LimitedFifoQueue<Long> requestDistance = new LimitedFifoQueue<Long>(2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: final. Also at other places.
singhravidutt marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we create this only for AUTO_RANDOM? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can but it adds multiple if calls in regular path. Given it's limited to just 2 long I am not too much worried about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyhow, it wasn't a big headache. Removed it and also made it configurable. |
||
// Keeps track of last index of last served Request. | ||
private long servedRequestLastIndex = -1; | ||
private boolean randomAccess; | ||
private boolean sequentialAccess; | ||
singhravidutt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* A FIFO queue.*/ | ||
singhravidutt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public class LimitedFifoQueue<E> extends LinkedList<E> { | ||
singhravidutt marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Composition is more appropriate than inheritance in this case. |
||
private int limit; | ||
|
||
public LimitedFifoQueue(int limit) { | ||
this.limit = limit; | ||
} | ||
|
||
@Override | ||
public boolean add(E o) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can vectorized IO call this concurrently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, VectoredRead don't use GcsReadChannel concurrently using multiple threads. GcsReadChannel class in not thread safe so is the FifoQueue. |
||
super.add(o); | ||
while (size() > limit) { | ||
// removed the head of linkedList. | ||
singhravidutt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
super.remove(); | ||
singhravidutt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return true; | ||
} | ||
} | ||
|
||
public ContentReadChannel( | ||
GoogleCloudStorageReadOptions readOptions, StorageResourceId resourceId) { | ||
this.blobId = | ||
BlobId.of( | ||
resourceId.getBucketName(), resourceId.getObjectName(), resourceId.getGenerationId()); | ||
this.randomAccess = readOptions.getFadvise() == Fadvise.RANDOM; | ||
this.randomAccess = | ||
readOptions.getFadvise() == Fadvise.RANDOM | ||
|| readOptions.getFadvise() == Fadvise.AUTO_RANDOM; | ||
this.sequentialAccess = !this.randomAccess; | ||
} | ||
|
||
public int readContent(ByteBuffer dst) throws IOException { | ||
|
@@ -304,6 +333,7 @@ | |
int partialBytes = partiallyReadBytes(remainingBeforeRead, dst); | ||
totalBytesRead += partialBytes; | ||
currentPosition += partialBytes; | ||
contentChannelCurrentPosition += partialBytes; | ||
logger.atFine().log( | ||
"Closing contentChannel after %s exception for '%s'.", e.getMessage(), resourceId); | ||
closeContentChannel(); | ||
|
@@ -321,12 +351,22 @@ | |
return partialReadBytes; | ||
} | ||
|
||
private boolean shouldDetectSequentialAccess() { | ||
return !gzipEncoded && !sequentialAccess && readOptions.getFadvise() == Fadvise.AUTO_RANDOM; | ||
} | ||
|
||
private boolean shouldDetectRandomAccess() { | ||
return !gzipEncoded && !randomAccess && readOptions.getFadvise() == Fadvise.AUTO; | ||
} | ||
|
||
private void setRandomAccess() { | ||
randomAccess = true; | ||
sequentialAccess = false; | ||
} | ||
|
||
private void setSequentialAccess() { | ||
sequentialAccess = true; | ||
randomAccess = false; | ||
} | ||
|
||
private ReadableByteChannel openByteChannel(long bytesToRead) throws IOException { | ||
|
@@ -341,6 +381,9 @@ | |
return serveFooterContent(); | ||
} | ||
|
||
// Should be updated only if content is not served from cached footer | ||
updateAccessPattern(); | ||
|
||
setChannelBoundaries(bytesToRead); | ||
|
||
ReadableByteChannel readableByteChannel = | ||
|
@@ -426,8 +469,14 @@ | |
if (gzipEncoded) { | ||
return objectSize; | ||
} | ||
|
||
long endPosition = objectSize; | ||
|
||
if (sequentialAccess) { | ||
endPosition = objectSize; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this line required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not required. |
||
if (readOptions.getFadvise() == Fadvise.AUTO_RANDOM) { | ||
endPosition = min(startPosition + readOptions.getBlockSize(), objectSize); | ||
} | ||
} | ||
if (randomAccess) { | ||
singhravidutt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// opening a channel for whole object doesn't make sense as anyhow it will not be utilized | ||
// for further reads. | ||
|
@@ -451,6 +500,7 @@ | |
"Got an exception on contentChannel.close() for '%s'; ignoring it.", resourceId); | ||
} finally { | ||
byteChannel = null; | ||
servedRequestLastIndex = contentChannelCurrentPosition; | ||
reset(); | ||
} | ||
} | ||
|
@@ -521,34 +571,70 @@ | |
if (isInRangeSeek()) { | ||
skipInPlace(); | ||
} else { | ||
if (isRandomAccessPattern()) { | ||
setRandomAccess(); | ||
} | ||
// close existing contentChannel as requested bytes can't be served from current | ||
// contentChannel; | ||
closeContentChannel(); | ||
} | ||
} | ||
|
||
private void updateAccessPattern() { | ||
if (readOptions.getFadvise() == Fadvise.AUTO_RANDOM) { | ||
if (isSequentialAccessPattern()) { | ||
setSequentialAccess(); | ||
} | ||
} else if (readOptions.getFadvise() == Fadvise.AUTO) { | ||
if (isRandomAccessPattern()) { | ||
setRandomAccess(); | ||
} | ||
} | ||
} | ||
|
||
private boolean isSequentialAccessPattern() { | ||
if (servedRequestLastIndex != -1) { | ||
requestDistance.add(currentPosition - servedRequestLastIndex); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is bit of a code smell. A "get" method updating the state. Is there a way to avoid it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not updating the sate of content but it's internal state about how to access data. This is the feature we are offering with |
||
} | ||
|
||
if (!shouldDetectSequentialAccess()) { | ||
return false; | ||
} | ||
|
||
if (requestDistance.size() < 2) { | ||
return false; | ||
} | ||
|
||
boolean sequentialRead = true; | ||
// if more than two reads qualifies for sequential access pattern | ||
ListIterator<Long> iterator = requestDistance.listIterator(); | ||
while (iterator.hasNext()) { | ||
Long distance = iterator.next(); | ||
if (distance < 0 || distance > readOptions.DEFAULT_INPLACE_SEEK_LIMIT) { | ||
sequentialRead = false; | ||
break; | ||
Check warning on line 612 in gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientReadChannel.java Codecov / codecov/patchgcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageClientReadChannel.java#L611-L612
|
||
} | ||
} | ||
return sequentialRead; | ||
} | ||
|
||
private boolean isRandomAccessPattern() { | ||
if (!shouldDetectRandomAccess()) { | ||
return false; | ||
} | ||
if (currentPosition < contentChannelCurrentPosition) { | ||
if (servedRequestLastIndex == -1) { | ||
return false; | ||
} | ||
|
||
if (currentPosition < servedRequestLastIndex) { | ||
logger.atFine().log( | ||
"Detected backward read from %s to %s position, switching to random IO for '%s'", | ||
contentChannelCurrentPosition, currentPosition, resourceId); | ||
servedRequestLastIndex, currentPosition, resourceId); | ||
return true; | ||
} | ||
if (contentChannelCurrentPosition >= 0 | ||
&& contentChannelCurrentPosition + readOptions.getInplaceSeekLimit() < currentPosition) { | ||
if (servedRequestLastIndex >= 0 | ||
&& servedRequestLastIndex + readOptions.getInplaceSeekLimit() < currentPosition) { | ||
logger.atFine().log( | ||
"Detected forward read from %s to %s position over %s threshold," | ||
+ " switching to random IO for '%s'", | ||
contentChannelCurrentPosition, | ||
currentPosition, | ||
readOptions.getInplaceSeekLimit(), | ||
resourceId); | ||
servedRequestLastIndex, currentPosition, readOptions.getInplaceSeekLimit(), resourceId); | ||
return true; | ||
} | ||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,8 @@ public abstract class GoogleCloudStorageReadOptions { | |
public enum Fadvise { | ||
AUTO, | ||
RANDOM, | ||
SEQUENTIAL | ||
SEQUENTIAL, | ||
AUTO_RANDOM | ||
} | ||
|
||
public static final int DEFAULT_BACKOFF_INITIAL_INTERVAL_MILLIS = 200; | ||
|
@@ -43,6 +44,7 @@ public enum Fadvise { | |
public static final boolean DEFAULT_FAST_FAIL_ON_NOT_FOUND = true; | ||
public static final boolean DEFAULT_SUPPORT_GZIP_ENCODING = true; | ||
public static final long DEFAULT_INPLACE_SEEK_LIMIT = 8 * 1024 * 1024; | ||
public static final long BLOCK_SIZE = 64 * 1024 * 1024; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we take the connector block_size? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we define it's own constants. Defaults getting picked in [GoogleHadoopFileSystemConfiguration.java] (https://github.com/GoogleCloudDataproc/hadoop-connectors/pull/1242/files#diff-f06c91b66e47300ff6c940ca14f152898b99e6e48033502fd4c1dd69c07f0c68) is using the connector BLOCK_SIZE. |
||
public static final Fadvise DEFAULT_FADVISE = Fadvise.SEQUENTIAL; | ||
public static final int DEFAULT_MIN_RANGE_REQUEST_SIZE = 2 * 1024 * 1024; | ||
public static final boolean GRPC_CHECKSUMS_ENABLED_DEFAULT = false; | ||
|
@@ -75,6 +77,7 @@ public static Builder builder() { | |
.setGrpcReadMessageTimeoutMillis(DEFAULT_GRPC_READ_MESSAGE_TIMEOUT_MILLIS) | ||
.setTraceLogEnabled(TRACE_LOGGING_ENABLED_DEFAULT) | ||
.setTraceLogTimeThreshold(0L) | ||
.setBlockSize(BLOCK_SIZE) | ||
.setTraceLogExcludeProperties(ImmutableSet.of()); | ||
} | ||
|
||
|
@@ -133,6 +136,8 @@ public static Builder builder() { | |
/** See {@link Builder#setTraceLogTimeThreshold(long)} . */ | ||
public abstract long getTraceLogTimeThreshold(); | ||
|
||
public abstract long getBlockSize(); | ||
|
||
/** Mutable builder for GoogleCloudStorageReadOptions. */ | ||
@AutoValue.Builder | ||
public abstract static class Builder { | ||
|
@@ -200,6 +205,9 @@ public abstract static class Builder { | |
* <ul> | ||
* <li>{@code AUTO} - automatically switches to {@code RANDOM} mode if backward read or | ||
* forward read for more than {@link #setInplaceSeekLimit} bytes is detected. | ||
* <li>{@code AUTO_RANDOM} - Uses {@code RANDOM} to start with and automatically switches to | ||
* {@code SEQUENTIAL} mode if more than 2 requests fall within {@link | ||
* #setInplaceSeekLimit} limits. | ||
* <li>{@code RANDOM} - sends HTTP requests with {@code Range} header set to greater of | ||
* provided reade buffer by user. | ||
* <li>{@code SEQUENTIAL} - sends HTTP requests with unbounded {@code Range} header. | ||
|
@@ -242,6 +250,8 @@ public abstract static class Builder { | |
/** Sets the property for gRPC read message timeout in milliseconds. */ | ||
public abstract Builder setGrpcReadMessageTimeoutMillis(long grpcMessageTimeout); | ||
|
||
public abstract Builder setBlockSize(long blockSize); | ||
|
||
abstract GoogleCloudStorageReadOptions autoBuild(); | ||
|
||
public GoogleCloudStorageReadOptions build() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is scope for improvement in the documentation. I did not get the gist of this flag by reading the documentation.
It is explaining WHAT the feature is doing. If you can also add WHEN this flag makes, sense, it would be useful the future readers of the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated