-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19795: ABFS. GetPathStatus Optimization on OpenFileForRead #8212
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
Changes from 13 commits
bb27ec4
16253aa
68f905c
bb42e3c
94d2336
46b3e18
3560cc5
f3b6b57
c53a82f
ba7131c
621c89e
b935f8f
e13fd56
b173e07
1cf2643
4c184d8
63f7ac1
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 |
|---|---|---|
|
|
@@ -152,6 +152,7 @@ | |
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.CHAR_STAR; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.CHAR_UNDERSCORE; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.DIRECTORY; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.EMPTY_STRING; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.FILE; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.ROOT_PATH; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.SINGLE_WHITE_SPACE; | ||
|
|
@@ -564,7 +565,7 @@ public Hashtable<String, String> getPathStatus(final Path path, | |
|
|
||
| /** | ||
| * Creates an object of {@link ContextEncryptionAdapter} | ||
| * from a file path. It calls {@link org.apache.hadoop.fs.azurebfs.services.AbfsClient | ||
| * from a file path. It calls {@link org.apache.hadoop.fs.azurebfs.services.AbfsClient | ||
| * #getPathStatus(String, boolean, TracingContext, EncryptionAdapter)} method to get | ||
| * contextValue (x-ms-encryption-context) from the server. The contextValue is passed | ||
| * to the constructor of EncryptionAdapter to create the required object of | ||
|
|
@@ -878,6 +879,20 @@ public AbfsInputStream openFileForRead(final Path path, | |
| tracingContext); | ||
| } | ||
|
|
||
| /** | ||
| * Creates an exception indicating that openFileForRead was called on a directory. | ||
| * | ||
| * @return AbfsRestOperationException with PATH_NOT_FOUND error code and a message | ||
| * indicating that openFileForRead must be used with files and not directories. | ||
| */ | ||
| private AbfsRestOperationException openFileForReadDirectoryException() { | ||
| return new AbfsRestOperationException( | ||
| AzureServiceErrorCode.PATH_NOT_FOUND.getStatusCode(), | ||
| AzureServiceErrorCode.PATH_NOT_FOUND.getErrorCode(), | ||
| "openFileForRead must be used with files and not directories", | ||
| null); | ||
| } | ||
|
|
||
| public AbfsInputStream openFileForRead(Path path, | ||
| final Optional<OpenFileParameters> parameters, | ||
| final FileSystem.Statistics statistics, TracingContext tracingContext) | ||
|
|
@@ -890,66 +905,79 @@ public AbfsInputStream openFileForRead(Path path, | |
| FileStatus fileStatus = parameters.map(OpenFileParameters::getStatus) | ||
| .orElse(null); | ||
| String relativePath = getRelativePath(path); | ||
| String resourceType, eTag; | ||
| long contentLength; | ||
| String resourceType, eTag = EMPTY_STRING; | ||
| long contentLength = 0; | ||
| ContextEncryptionAdapter contextEncryptionAdapter = NoContextEncryptionAdapter.getInstance(); | ||
| /* | ||
| * GetPathStatus API has to be called in case of: | ||
| * 1. fileStatus is null or not an object of VersionedFileStatus: as eTag | ||
| * would not be there in the fileStatus object. | ||
| * 1. restrictGpsOnOpenFile config is disabled AND fileStatus is null or not | ||
| * an object of VersionedFileStatus: as eTag would not be there in the fileStatus object. | ||
| * 2. fileStatus is an object of VersionedFileStatus and the object doesn't | ||
| * have encryptionContext field when client's encryptionType is | ||
| * ENCRYPTION_CONTEXT. | ||
| */ | ||
| if ((fileStatus instanceof VersionedFileStatus) && ( | ||
| getClient().getEncryptionType() != EncryptionType.ENCRYPTION_CONTEXT | ||
| || ((VersionedFileStatus) fileStatus).getEncryptionContext() | ||
| != null)) { | ||
| getClient().getEncryptionType() != EncryptionType.ENCRYPTION_CONTEXT | ||
|
Contributor
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. additional space changes can be reverted |
||
| || ((VersionedFileStatus) fileStatus).getEncryptionContext() | ||
| != null)) { | ||
| path = path.makeQualified(this.uri, path); | ||
| Preconditions.checkArgument(fileStatus.getPath().equals(path), | ||
| String.format( | ||
| "Filestatus path [%s] does not match with given path [%s]", | ||
| fileStatus.getPath(), path)); | ||
| String.format( | ||
| "Filestatus path [%s] does not match with given path [%s]", | ||
| fileStatus.getPath(), path)); | ||
| resourceType = fileStatus.isFile() ? FILE : DIRECTORY; | ||
| contentLength = fileStatus.getLen(); | ||
| eTag = ((VersionedFileStatus) fileStatus).getVersion(); | ||
| final String encryptionContext | ||
| = ((VersionedFileStatus) fileStatus).getEncryptionContext(); | ||
| = ((VersionedFileStatus) fileStatus).getEncryptionContext(); | ||
| if (getClient().getEncryptionType() == EncryptionType.ENCRYPTION_CONTEXT) { | ||
| contextEncryptionAdapter = new ContextProviderEncryptionAdapter( | ||
| getClient().getEncryptionContextProvider(), getRelativePath(path), | ||
| encryptionContext.getBytes(StandardCharsets.UTF_8)); | ||
| getClient().getEncryptionContextProvider(), getRelativePath(path), | ||
| encryptionContext.getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| } else { | ||
| if (parseIsDirectory(resourceType)) { | ||
|
Contributor
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 be moved to common part as is getting checked in both the cases
Contributor
Author
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. Taken |
||
| throw openFileForReadDirectoryException(); | ||
| } | ||
| } | ||
| /* | ||
| * If file created with ENCRYPTION_CONTEXT, irrespective of whether isRestrictGpsOnOpenFile config is enabled or not, | ||
| * GetPathStatus API has to be called to get the encryptionContext from the response header | ||
| */ | ||
| else if (getClient().getEncryptionType() == EncryptionType.ENCRYPTION_CONTEXT | ||
| || !getAbfsConfiguration().shouldRestrictGpsOnOpenFile()) { | ||
|
|
||
| AbfsHttpOperation op = getClient().getPathStatus(relativePath, false, | ||
| tracingContext, null).getResult(); | ||
| resourceType = getClient().checkIsDir(op) ? DIRECTORY : FILE; | ||
| contentLength = extractContentLength(op); | ||
| eTag = op.getResponseHeader(HttpHeaderConfigurations.ETAG); | ||
| tracingContext, null).getResult(); | ||
| /* | ||
| * For file created with ENCRYPTION_CONTEXT, client shall receive | ||
| * encryptionContext from header field: X_MS_ENCRYPTION_CONTEXT. | ||
| */ | ||
| if (getClient().getEncryptionType() == EncryptionType.ENCRYPTION_CONTEXT) { | ||
| final String fileEncryptionContext = op.getResponseHeader( | ||
| HttpHeaderConfigurations.X_MS_ENCRYPTION_CONTEXT); | ||
| X_MS_ENCRYPTION_CONTEXT); | ||
| if (fileEncryptionContext == null) { | ||
| LOG.debug("EncryptionContext missing in GetPathStatus response"); | ||
| throw new PathIOException(path.toString(), | ||
| "EncryptionContext not present in GetPathStatus response headers"); | ||
| "EncryptionContext not present in GetPathStatus response headers"); | ||
| } | ||
| contextEncryptionAdapter = new ContextProviderEncryptionAdapter( | ||
| getClient().getEncryptionContextProvider(), getRelativePath(path), | ||
| fileEncryptionContext.getBytes(StandardCharsets.UTF_8)); | ||
| getClient().getEncryptionContextProvider(), getRelativePath(path), | ||
|
Contributor
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. same as above |
||
| fileEncryptionContext.getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| } | ||
| resourceType = getClient().checkIsDir(op) ? DIRECTORY : FILE; | ||
| contentLength = extractContentLength(op); | ||
| eTag = op.getResponseHeader(HttpHeaderConfigurations.ETAG); | ||
|
|
||
| if (parseIsDirectory(resourceType)) { | ||
| throw new AbfsRestOperationException( | ||
| AzureServiceErrorCode.PATH_NOT_FOUND.getStatusCode(), | ||
| AzureServiceErrorCode.PATH_NOT_FOUND.getErrorCode(), | ||
| "openFileForRead must be used with files and not directories", | ||
| null); | ||
| if (parseIsDirectory(resourceType)) { | ||
| throw openFileForReadDirectoryException(); | ||
| } | ||
| } | ||
| /* The only remaining case is: | ||
| * - restrictGpsOnOpenFile config is enabled with null FileStatus and encryptionType not as ENCRYPTION_CONTEXT | ||
| * In this case, we don't need to call GetPathStatus API. | ||
| */ | ||
| else { | ||
|
Contributor
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. Won't this lead to going ahead and opening the stream without checks? Do we fail later for this case ?
Contributor
Author
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. Yes, the checks with this config would be happening after the first read then. If the read fails there then we throw the appropriate exception |
||
| // do nothing | ||
| } | ||
|
|
||
| perfInfo.registerSuccess(true); | ||
|
|
@@ -1015,6 +1043,7 @@ AZURE_FOOTER_READ_BUFFER_SIZE, getAbfsConfiguration().getFooterReadBufferSize()) | |
| .withStreamStatistics(new AbfsInputStreamStatisticsImpl()) | ||
| .withShouldReadBufferSizeAlways(getAbfsConfiguration().shouldReadBufferSizeAlways()) | ||
| .withReadAheadBlockSize(getAbfsConfiguration().getReadAheadBlockSize()) | ||
| .shouldRestrictGpsOnOpenFile(getAbfsConfiguration().shouldRestrictGpsOnOpenFile()) | ||
| .withBufferedPreadDisabled(bufferedPreadDisabled) | ||
| .withEncryptionAdapter(contextEncryptionAdapter) | ||
| .withAbfsBackRef(fsBackRef) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,8 @@ public enum AzureServiceErrorCode { | |
| INVALID_APPEND_OPERATION("InvalidAppendOperation", HttpURLConnection.HTTP_CONFLICT, null), | ||
| UNAUTHORIZED_BLOB_OVERWRITE("UnauthorizedBlobOverwrite", HttpURLConnection.HTTP_FORBIDDEN, | ||
| "This request is not authorized to perform blob overwrites."), | ||
| INVALID_RANGE("InvalidRange", 416, | ||
|
Contributor
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. 416 should come from a constant defined in HttpURLConnection class
Contributor
Author
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 dont have a 416 defined in HttpURLConnection
Contributor
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 define it in our constants class then
Contributor
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. Yes, let's define in AbfsHttpConstants |
||
| "The range specified is invalid for the current size of the resource."), | ||
| UNKNOWN(null, -1, null); | ||
|
|
||
| private final String errorCode; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,7 +68,7 @@ protected int readOneBlock(final byte[] b, final int off, final int len) throws | |
| // If buffer is empty, then fill the buffer. | ||
| if (getBCursor() == getLimit()) { | ||
| // If EOF, then return -1 | ||
| if (getFCursor() >= getContentLength()) { | ||
| if (!(shouldRestrictGpsOnOpenFile() && isFirstRead()) && getFCursor() >= getContentLength()) { | ||
|
Contributor
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 decouple Currently, if both
Contributor
Author
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. As discussed, for first reads we have other validation in place |
||
| return -1; | ||
| } | ||
|
|
||
|
|
@@ -83,7 +83,11 @@ protected int readOneBlock(final byte[] b, final int off, final int len) throws | |
|
|
||
| // Reset Read Type back to normal and set again based on code flow. | ||
| getTracingContext().setReadType(ReadType.NORMAL_READ); | ||
| if (shouldAlwaysReadBufferSize()) { | ||
| if(shouldRestrictGpsOnOpenFile() && isFirstRead()) { | ||
|
Contributor
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: space after if
Contributor
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. add a comment for this condition as well
Contributor
Author
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. Added |
||
| LOG.debug("RestrictGpsOnOpenFile is enabled. Skip readahead for first read."); | ||
| bytesRead = readInternal(getFCursor(), getBuffer(), 0, getBufferSize(), true); | ||
| } | ||
| else if (shouldAlwaysReadBufferSize()) { | ||
| bytesRead = readInternal(getFCursor(), getBuffer(), 0, getBufferSize(), false); | ||
| } else { | ||
| // Enable readAhead when reading sequentially | ||
|
|
||
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.
Define this error String in AbfsErrors file
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.
Taken