bhattmanish98 commented on code in PR #8050:
URL: https://github.com/apache/hadoop/pull/8050#discussion_r2485436621
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java:
##########
@@ -393,6 +393,14 @@ public final class FileSystemConfigurations {
public static final boolean DEFAULT_FS_AZURE_ENABLE_CREATE_BLOB_IDEMPOTENCY
= true;
+ public static final boolean
DEFAULT_FS_AZURE_ENABLE_PREFETCH_REQUEST_PRIORITY = true;
+
+ // The default traffic request priority is 3 (from service)
+ // The lowest priority a request can get is 7
+ public static final int DEFAULT_FS_AZURE_REQUEST_PRIORITY_VALUE_7 = 7;
Review Comment:
You don't need to add 7 in the variable name.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java:
##########
@@ -63,6 +63,7 @@
import org.apache.hadoop.fs.azurebfs.constants.FSOperationType;
import org.apache.hadoop.fs.azurebfs.constants.HttpHeaderConfigurations;
import org.apache.hadoop.fs.azurebfs.constants.HttpQueryParams;
+import org.apache.hadoop.fs.azurebfs.constants.ReadType;
Review Comment:
Unused import
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStream.java:
##########
@@ -590,7 +590,7 @@ int readRemote(long position, byte[] b, int offset, int
length, TracingContext t
tracingContext.setPosition(String.valueOf(position));
op = client.read(path, position, b, offset, length,
tolerateOobAppends ? "*" : eTag, cachedSasToken.get(),
- contextEncryptionAdapter, tracingContext);
+ contextEncryptionAdapter, new TracingContext(tracingContext));
Review Comment:
I don't think we need this isolation here. Can you give an example where it
may fail without this change?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java:
##########
@@ -282,6 +282,18 @@ public final class ConfigurationKeys {
*/
public static final String FS_AZURE_ENABLE_READAHEAD_V2_DYNAMIC_SCALING =
"fs.azure.enable.readahead.v2.dynamic.scaling";
+ /**
+ * Enable or disable request priority for prefetch requests
+ * Value: {@value}.
+ */
+ public static final String FS_AZURE_ENABLE_PREFETCH_REQUEST_PRIORITY =
"fs.azure.enable.prefetch.request.priority";
+
+ /**
Review Comment:
Correction in Java doc required
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/ReadBufferManagerV2.java:
##########
@@ -280,7 +280,7 @@ public void queueReadAhead(final AbfsInputStream stream,
buffer.setRequestedLength(requestedLength);
buffer.setStatus(ReadBufferStatus.NOT_AVAILABLE);
buffer.setLatch(new CountDownLatch(1));
- buffer.setTracingContext(tracingContext);
+ buffer.setTracingContext(new TracingContext(tracingContext));
Review Comment:
Same as above
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##########
@@ -638,6 +638,15 @@ public class AbfsConfiguration{
DefaultValue = DEFAULT_FS_AZURE_TAIL_LATENCY_MAX_RETRY_COUNT)
private int tailLatencyMaxRetryCount;
+ @BooleanConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_ENABLE_PREFETCH_REQUEST_PRIORITY,
+ DefaultValue = DEFAULT_FS_AZURE_ENABLE_PREFETCH_REQUEST_PRIORITY)
+ private boolean enablePrefetchRequestPriority;
+
+ @IntegerConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_PREFETCH_REQUEST_PRIORITY_VALUE,
+ MinValue = DEFAULT_FS_AZURE_STANDARD_REQUEST_PRIORITY_VALUE,
Review Comment:
I guess we should define a separate variable for min
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -1399,6 +1402,15 @@ protected void
addCheckSumHeaderForWrite(List<AbfsHttpHeader> requestHeaders,
}
}
+ protected void addRequestPriorityForPrefetch(List<AbfsHttpHeader>
requestHeaders,
Review Comment:
Java doc missing
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -158,6 +160,7 @@ public abstract class AbfsClient implements Closeable {
public static final String HUNDRED_CONTINUE_USER_AGENT = SINGLE_WHITE_SPACE
+ HUNDRED_CONTINUE + SEMICOLON;
public static final String ABFS_CLIENT_TIMER_THREAD_NAME =
"abfs-timer-client";
public static final String FNS_BLOB_USER_AGENT_IDENTIFIER = "FNS";
+ private static final String PREFETCH_TRAFFIC_PRIORITY_HEADER_VALUE = "7";
Review Comment:
Can't we use DEFAULT_FS_AZURE_REQUEST_PRIORITY_VALUE here?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java:
##########
@@ -56,6 +56,7 @@
import org.apache.hadoop.fs.azurebfs.constants.AbfsServiceType;
import org.apache.hadoop.fs.azurebfs.constants.HttpHeaderConfigurations;
import org.apache.hadoop.fs.azurebfs.constants.HttpQueryParams;
+import org.apache.hadoop.fs.azurebfs.constants.ReadType;
Review Comment:
Unused import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
