-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19645. [ABFS][ReadAheadV2] Improve Metrics for Read Calls to identify type of read done. #7837
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
HADOOP-19645. [ABFS][ReadAheadV2] Improve Metrics for Read Calls to identify type of read done. #7837
Changes from 6 commits
ea1572a
42ecdd0
224f712
9a87ad5
0d926b1
9bb6cdb
69ffec4
132893f
6345ec9
9059784
c787d3b
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 |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /** | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.fs.azurebfs.constants; | ||
|
|
||
| /** | ||
| * Enumeration for different types of read operations triggered by AbfsInputStream. | ||
| */ | ||
| public enum ReadType { | ||
| /** | ||
| * Synchronous read from the storage service. No optimization is being applied. | ||
| */ | ||
| DIRECT_READ("DR"), | ||
| /** | ||
| * Synchronous read from the storage service where optimization were considered but found disabled. | ||
| */ | ||
| NORMAL_READ("NR"), | ||
| /** | ||
| * Asynchronous read from the storage service for filling up cache. | ||
| */ | ||
| PREFETCH_READ("PR"), | ||
| /** | ||
| * Synchronous read from the storage service when nothing was found in cache. | ||
| */ | ||
| MISSEDCACHE_READ("MR"), | ||
| /** | ||
| * Synchronous read from the storage service for reading the footer of a file. | ||
| * Only triggered when footer read optimization kicks in. | ||
| */ | ||
| FOOTER_READ("FR"), | ||
| /** | ||
| * Synchronous read from the storage service for reading a small file fully. | ||
| * Only triggered when small file read optimization kicks in. | ||
| */ | ||
| SMALLFILE_READ("SR"), | ||
| /** | ||
| * None of the above read types were applicable. | ||
| */ | ||
| UNKNOWN_READ("UR"); | ||
|
|
||
| private final String readType; | ||
|
|
||
| ReadType(String readType) { | ||
| this.readType = readType; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return readType; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.hadoop.classification.VisibleForTesting; | ||
| import org.apache.hadoop.fs.PositionedReadable; | ||
| import org.apache.hadoop.fs.azurebfs.constants.ReadType; | ||
| import org.apache.hadoop.fs.impl.BackReference; | ||
| import org.apache.hadoop.util.Preconditions; | ||
|
|
||
|
|
@@ -165,6 +166,7 @@ public AbfsInputStream( | |
| this.tracingContext = new TracingContext(tracingContext); | ||
| this.tracingContext.setOperation(FSOperationType.READ); | ||
| this.tracingContext.setStreamID(inputStreamId); | ||
| this.tracingContext.setReadType(ReadType.UNKNOWN_READ); | ||
| this.context = abfsInputStreamContext; | ||
| readAheadBlockSize = abfsInputStreamContext.getReadAheadBlockSize(); | ||
| if (abfsReadFooterMetrics != null) { | ||
|
|
@@ -227,7 +229,9 @@ public int read(long position, byte[] buffer, int offset, int length) | |
| if (streamStatistics != null) { | ||
| streamStatistics.readOperationStarted(); | ||
| } | ||
| int bytesRead = readRemote(position, buffer, offset, length, tracingContext); | ||
| TracingContext tc = new TracingContext(tracingContext); | ||
| tc.setReadType(ReadType.DIRECT_READ); | ||
| int bytesRead = readRemote(position, buffer, offset, length, tc); | ||
| if (statistics != null) { | ||
| statistics.incrementBytesRead(bytesRead); | ||
| } | ||
|
|
@@ -345,6 +349,8 @@ private int readOneBlock(final byte[] b, final int off, final int len) throws IO | |
| buffer = new byte[bufferSize]; | ||
| } | ||
|
|
||
| // Reset Read Type back to normal and set again based on code flow. | ||
| tracingContext.setReadType(ReadType.NORMAL_READ); | ||
| if (alwaysReadBufferSize) { | ||
| bytesRead = readInternal(fCursor, buffer, 0, bufferSize, false); | ||
| } else { | ||
|
|
@@ -385,6 +391,7 @@ private int readFileCompletely(final byte[] b, final int off, final int len) | |
| // data need to be copied to user buffer from index bCursor, bCursor has | ||
| // to be the current fCusor | ||
| bCursor = (int) fCursor; | ||
| tracingContext.setReadType(ReadType.SMALLFILE_READ); | ||
| return optimisedRead(b, off, len, 0, contentLength); | ||
| } | ||
|
|
||
|
|
@@ -405,6 +412,7 @@ private int readLastBlock(final byte[] b, final int off, final int len) | |
| bCursor = (int) (fCursor - lastBlockStart); | ||
| // 0 if contentlength is < buffersize | ||
| long actualLenToRead = min(footerReadSize, contentLength); | ||
| tracingContext.setReadType(ReadType.FOOTER_READ); | ||
| return optimisedRead(b, off, len, lastBlockStart, actualLenToRead); | ||
| } | ||
|
|
||
|
|
@@ -428,6 +436,7 @@ private int optimisedRead(final byte[] b, final int off, final int len, | |
| } | ||
| } catch (IOException e) { | ||
| LOG.debug("Optimized read failed. Defaulting to readOneBlock {}", e); | ||
| tracingContext.setReadType(ReadType.NORMAL_READ); | ||
| restorePointerState(); | ||
| return readOneBlock(b, off, len); | ||
| } finally { | ||
|
|
@@ -442,6 +451,7 @@ private int optimisedRead(final byte[] b, final int off, final int len, | |
| // bCursor that means the user requested data has not been read. | ||
| if (fCursor < contentLength && bCursor > limit) { | ||
| restorePointerState(); | ||
| tracingContext.setReadType(ReadType.NORMAL_READ); | ||
|
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. Before readOneBlock we're setting TC as normal read both here and line 439. In readOneBlock method- we're setting TC again to normal read- do we need it twice?
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. Nice Catch, that seemed redundant, hence removed |
||
| return readOneBlock(b, off, len); | ||
| } | ||
| return copyToUserBuffer(b, off, len); | ||
|
|
@@ -520,6 +530,7 @@ private int readInternal(final long position, final byte[] b, final int offset, | |
| LOG.debug("read ahead enabled issuing readheads num = {}", numReadAheads); | ||
| TracingContext readAheadTracingContext = new TracingContext(tracingContext); | ||
| readAheadTracingContext.setPrimaryRequestID(); | ||
| readAheadTracingContext.setReadType(ReadType.PREFETCH_READ); | ||
| while (numReadAheads > 0 && nextOffset < contentLength) { | ||
| LOG.debug("issuing read ahead requestedOffset = {} requested size {}", | ||
| nextOffset, nextSize); | ||
|
|
@@ -544,7 +555,9 @@ private int readInternal(final long position, final byte[] b, final int offset, | |
| } | ||
|
|
||
| // got nothing from read-ahead, do our own read now | ||
| receivedBytes = readRemote(position, b, offset, length, new TracingContext(tracingContext)); | ||
| TracingContext tc = new TracingContext(tracingContext); | ||
| tc.setReadType(ReadType.MISSEDCACHE_READ); | ||
| receivedBytes = readRemote(position, b, offset, length, tc); | ||
| return receivedBytes; | ||
| } else { | ||
| LOG.debug("read ahead disabled, reading remote"); | ||
|
anujmodi2021 marked this conversation as resolved.
|
||
|
|
@@ -578,6 +591,7 @@ int readRemote(long position, byte[] b, int offset, int length, TracingContext t | |
| streamStatistics.remoteReadOperation(); | ||
| } | ||
| LOG.trace("Trigger client.read for path={} position={} offset={} length={}", path, position, offset, length); | ||
| tracingContext.setPosition(String.valueOf(position)); | ||
|
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. Is there a test to verify position is correctly added to tracing context? Position is a key identifier for read operations.
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. Thanks for the suggestion. I updated the current test to assert on position as well. |
||
| op = client.read(path, position, b, offset, length, | ||
| tolerateOobAppends ? "*" : eTag, cachedSasToken.get(), | ||
| contextEncryptionAdapter, tracingContext); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,9 +25,12 @@ | |
|
|
||
| import org.apache.hadoop.fs.azurebfs.constants.FSOperationType; | ||
| import org.apache.hadoop.fs.azurebfs.constants.HttpHeaderConfigurations; | ||
| import org.apache.hadoop.fs.azurebfs.constants.ReadType; | ||
| import org.apache.hadoop.fs.azurebfs.services.AbfsClient; | ||
| import org.apache.hadoop.fs.azurebfs.services.AbfsHttpOperation; | ||
|
|
||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.CHAR_HYPHEN; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.COLON; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.EMPTY_STRING; | ||
| import static org.apache.hadoop.fs.azurebfs.services.RetryReasonConstants.CONNECTION_TIMEOUT_ABBREVIATION; | ||
|
|
||
|
|
@@ -67,6 +70,7 @@ public class TracingContext { | |
| private String position = EMPTY_STRING; | ||
| private String metricResults = EMPTY_STRING; | ||
| private String metricHeader = EMPTY_STRING; | ||
| private ReadType readType = ReadType.UNKNOWN_READ; | ||
|
|
||
| /** | ||
| * If {@link #primaryRequestId} is null, this field shall be set equal | ||
|
|
@@ -77,8 +81,7 @@ public class TracingContext { | |
| * this field shall not be set. | ||
| */ | ||
| private String primaryRequestIdForRetry; | ||
|
|
||
| private Integer operatedBlobCount = null; | ||
| private Integer operatedBlobCount = 1; // Only relevant for rename-delete over blob endpoint where it will be explicitly set. | ||
|
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. why is it changed from null to 1 ?
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. Because it was coming out as null in ClientReqId. Having a null value does not looks good and can be prone to NPE if someone used this value anywhere. As to why set to 1, I thought for every operation this has to be 1. I am open to suggestions for a better default value but strongly feel null should be avoided. Thoughts?
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. But there was a null check before it was added to the header which would avoid the NPE
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 but we decided to keep the header schema fix and publishing this value as null does not look good in Client Request Id as it can be exposed to user. |
||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(AbfsClient.class); | ||
| public static final int MAX_CLIENT_CORRELATION_ID_LENGTH = 72; | ||
|
|
@@ -142,6 +145,7 @@ public TracingContext(TracingContext originalTracingContext) { | |
| this.listener = originalTracingContext.listener.getClone(); | ||
| } | ||
| this.metricResults = originalTracingContext.metricResults; | ||
| this.readType = originalTracingContext.readType; | ||
| } | ||
| public static String validateClientCorrelationID(String clientCorrelationID) { | ||
| if ((clientCorrelationID.length() > MAX_CLIENT_CORRELATION_ID_LENGTH) | ||
|
|
@@ -181,8 +185,24 @@ public void setListener(Listener listener) { | |
| } | ||
|
|
||
| /** | ||
| * Concatenate all identifiers separated by (:) into a string and set into | ||
| * Concatenate all components separated by (:) into a string and set into | ||
| * X_MS_CLIENT_REQUEST_ID header of the http operation | ||
| * Following are the components in order of concatenation: | ||
| * <ul> | ||
| * <li>version - not present for versions less than v1</li> | ||
| * <li>clientCorrelationId</li> | ||
| * <li>clientRequestId</li> | ||
| * <li>fileSystemId</li> | ||
| * <li>primaryRequestId</li> | ||
| * <li>streamId</li> | ||
| * <li>opType</li> | ||
| * <li>retryHeader - this contains retryCount, failureReason and retryPolicy underscore separated</li> | ||
| * <li>ingressHandler</li> | ||
| * <li>position of read/write in the remote file</li> | ||
| * <li>operatedBlobCount - number of blobs operated on by this request</li> | ||
| * <li>httpOperationHeader - suffix for network library used</li> | ||
| * <li>operationSpecificHeader - different operation types can publish info relevant to that operation</li> | ||
| * </ul> | ||
| * @param httpOperation AbfsHttpOperation instance to set header into | ||
| * connection | ||
| * @param previousFailure Failure seen before this API trigger on same operation | ||
|
|
@@ -193,32 +213,33 @@ public void setListener(Listener listener) { | |
| public void constructHeader(AbfsHttpOperation httpOperation, String previousFailure, String retryPolicyAbbreviation) { | ||
| clientRequestId = UUID.randomUUID().toString(); | ||
| switch (format) { | ||
| case ALL_ID_FORMAT: // Optional IDs (e.g. streamId) may be empty | ||
| header = | ||
| clientCorrelationID + ":" + clientRequestId + ":" + fileSystemID + ":" | ||
| + getPrimaryRequestIdForHeader(retryCount > 0) + ":" + streamID | ||
| + ":" + opType + ":" + retryCount; | ||
| header = addFailureReasons(header, previousFailure, retryPolicyAbbreviation); | ||
| if (!(ingressHandler.equals(EMPTY_STRING))) { | ||
| header += ":" + ingressHandler; | ||
| } | ||
| if (!(position.equals(EMPTY_STRING))) { | ||
| header += ":" + position; | ||
| } | ||
| if (operatedBlobCount != null) { | ||
| header += (":" + operatedBlobCount); | ||
| } | ||
| header += (":" + httpOperation.getTracingContextSuffix()); | ||
| metricHeader += !(metricResults.trim().isEmpty()) ? metricResults : ""; | ||
| case ALL_ID_FORMAT: | ||
| header = TracingHeaderVersion.V1.getVersion() + COLON | ||
|
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 use getCurrentVersion here ?
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. Fixed |
||
| + clientCorrelationID + COLON | ||
| + clientRequestId + COLON | ||
| + fileSystemID + COLON | ||
| + getPrimaryRequestIdForHeader(retryCount > 0) + COLON | ||
| + streamID + COLON | ||
| + opType + COLON | ||
| + getRetryHeader(previousFailure, retryPolicyAbbreviation) + COLON | ||
| + ingressHandler + COLON | ||
|
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. these empty string checks are needed
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. With empty checks we cannot have a fixed schema. We need the proper defined schema where each position after split is fixed for all the headers and analysis can be done easily without worrying about the position of info we need to analyse. |
||
| + position + COLON | ||
| + operatedBlobCount + COLON | ||
| + httpOperation.getTracingContextSuffix() + COLON | ||
| + getOperationSpecificHeader(opType); | ||
|
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 keep the op specific header before adding the HTTP client? It would get all req related info together and then network client.
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. Sounds Better, Taken |
||
|
|
||
| metricHeader += !(metricResults.trim().isEmpty()) ? metricResults : EMPTY_STRING; | ||
| break; | ||
| case TWO_ID_FORMAT: | ||
| header = clientCorrelationID + ":" + clientRequestId; | ||
| metricHeader += !(metricResults.trim().isEmpty()) ? metricResults : ""; | ||
| header = TracingHeaderVersion.V1.getVersion() + COLON | ||
|
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 getCurrentVersion ?
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. Fixed |
||
| + clientCorrelationID + COLON + clientRequestId; | ||
| metricHeader += !(metricResults.trim().isEmpty()) ? metricResults : EMPTY_STRING; | ||
| break; | ||
| default: | ||
| //case SINGLE_ID_FORMAT | ||
| header = clientRequestId; | ||
| metricHeader += !(metricResults.trim().isEmpty()) ? metricResults : ""; | ||
| header = TracingHeaderVersion.V1.getVersion() + COLON | ||
| + clientRequestId; | ||
| metricHeader += !(metricResults.trim().isEmpty()) ? metricResults : EMPTY_STRING; | ||
| } | ||
| if (listener != null) { //for testing | ||
| listener.callTracingHeaderValidator(header, format); | ||
|
|
@@ -234,7 +255,7 @@ public void constructHeader(AbfsHttpOperation httpOperation, String previousFail | |
| * of the x-ms-client-request-id header in case of retry of the same API-request. | ||
| */ | ||
| if (primaryRequestId.isEmpty() && previousFailure == null) { | ||
| String[] clientRequestIdParts = clientRequestId.split("-"); | ||
| String[] clientRequestIdParts = clientRequestId.split(String.valueOf(CHAR_HYPHEN)); | ||
| primaryRequestIdForRetry = clientRequestIdParts[ | ||
| clientRequestIdParts.length - 1]; | ||
| } | ||
|
|
@@ -265,6 +286,34 @@ private String addFailureReasons(final String header, | |
| return String.format("%s_%s", header, previousFailure); | ||
| } | ||
|
|
||
| private String getRetryHeader(final String previousFailure, String retryPolicyAbbreviation) { | ||
|
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. Please add javadoc to all newly added methods
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
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 remove the addFailureReasons method- it has no usage now
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 |
||
| String retryHeader = String.format("%d", retryCount); | ||
| if (previousFailure == null) { | ||
| return retryHeader; | ||
| } | ||
| if (CONNECTION_TIMEOUT_ABBREVIATION.equals(previousFailure) && retryPolicyAbbreviation != null) { | ||
| return String.format("%s_%s_%s", retryHeader, previousFailure, retryPolicyAbbreviation); | ||
| } | ||
| return String.format("%s_%s", retryHeader, previousFailure); | ||
| } | ||
|
|
||
| private String getOperationSpecificHeader(FSOperationType opType) { | ||
| // Similar header can be added for other operations in the future. | ||
| switch (opType) { | ||
| case READ: | ||
| return getReadSpecificHeader(); | ||
| default: | ||
| return EMPTY_STRING; // no operation specific header | ||
| } | ||
| } | ||
|
|
||
| private String getReadSpecificHeader() { | ||
| // More information on read can be added to this header in the future. | ||
| // As underscore separated values. | ||
| String readHeader = String.format("%s", readType.toString()); | ||
|
anujmodi2021 marked this conversation as resolved.
|
||
| return readHeader; | ||
| } | ||
|
|
||
| public void setOperatedBlobCount(Integer count) { | ||
| operatedBlobCount = count; | ||
| } | ||
|
|
@@ -322,4 +371,15 @@ public void setPosition(final String position) { | |
| listener.updatePosition(position); | ||
| } | ||
| } | ||
|
|
||
| public void setReadType(ReadType readType) { | ||
| this.readType = readType; | ||
| if (listener != null) { | ||
| listener.updateReadType(readType); | ||
| } | ||
| } | ||
|
|
||
| public ReadType getReadType() { | ||
| return readType; | ||
| } | ||
| } | ||
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.
We already have CHAR_HYPHEN defined for this.
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