[
https://issues.apache.org/jira/browse/HADOOP-17290?focusedWorklogId=614331&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-614331
]
ASF GitHub Bot logged work on HADOOP-17290:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 24/Jun/21 05:39
Start Date: 24/Jun/21 05:39
Worklog Time Spent: 10m
Work Description: anoopsjohn commented on a change in pull request #2520:
URL: https://github.com/apache/hadoop/pull/2520#discussion_r657346332
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
##########
@@ -1071,7 +1155,10 @@ private boolean fileSystemExists() throws IOException {
LOG.debug(
"AzureBlobFileSystem.fileSystemExists uri: {}", uri);
try {
- abfsStore.getFilesystemProperties();
+ TracingContext tracingContext = new TracingContext(clientCorrelationID,
+ fileSystemID, HdfsOperationConstants.GET_FILESTATUS,
Review comment:
GET_FILESTATUS op?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
##########
@@ -264,6 +266,10 @@
DefaultValue = DEFAULT_VALUE_UNKNOWN)
private String clusterType;
+ @StringConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_CLIENT_CORRELATIONID,
+ DefaultValue = EMPTY_STRING)
+ private String clientCorrelationID;
Review comment:
clientCorrelationId ? To be similar as 'userAgentId' etc? And the
getter also
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
##########
@@ -335,7 +361,10 @@ public boolean rename(final Path src, final Path dst)
throws IOException {
}
// Non-HNS account need to check dst status on driver side.
- if (!abfsStore.getIsNamespaceEnabled() && dstFileStatus == null) {
+ TracingContext tracingContext = new TracingContext(clientCorrelationID,
+ fileSystemID, HdfsOperationConstants.RENAME, true,
tracingContextFormat,
+ listener);
+ if (!abfsStore.getIsNamespaceEnabled(tracingContext) && dstFileStatus ==
null) {
Review comment:
Within tryGetFileStatus() there is call to getFileStatus. We should be
using this context created here.
tryGetFileStatus() been called by createNonRecursive API also.
Have to handle these.
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
##########
@@ -1049,8 +1130,11 @@ public boolean exists(Path f) throws IOException {
throws IOException {
LOG.debug("AzureBlobFileSystem.listStatusIterator path : {}", path);
if (abfsStore.getAbfsConfiguration().enableAbfsListIterator()) {
+ TracingContext tracingContext = new TracingContext(clientCorrelationID,
+ fileSystemID, HdfsOperationConstants.LISTSTATUS, true,
+ tracingContextFormat, listener);
AbfsListStatusRemoteIterator abfsLsItr =
- new AbfsListStatusRemoteIterator(getFileStatus(path), abfsStore);
+ new AbfsListStatusRemoteIterator(getFileStatus(path), abfsStore,
tracingContext);
Review comment:
Here is a call to getFileStatus(Path) but as part of LISTSTATUS op. So
we should the context created above , during getFileStatus also right?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsInputStream.java
##########
@@ -451,15 +472,15 @@ 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);
+ receivedBytes = readRemote(position, b, offset, length, new
TracingContext(tracingContext));
return receivedBytes;
} else {
LOG.debug("read ahead disabled, reading remote");
- return readRemote(position, b, offset, length);
+ return readRemote(position, b, offset, length, new
TracingContext(tracingContext));
}
}
- int readRemote(long position, byte[] b, int offset, int length) throws
IOException {
+ int readRemote(long position, byte[] b, int offset, int length,
TracingContext tracingContext) throws IOException {
Review comment:
Why passing 'tracingContext' when its set as instance member?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java
##########
@@ -109,6 +109,12 @@
* Default value of this config is true. **/
public static final String FS_AZURE_DISABLE_OUTPUTSTREAM_FLUSH =
"fs.azure.disable.outputstream.flush";
public static final String FS_AZURE_USER_AGENT_PREFIX_KEY =
"fs.azure.user.agent.prefix";
+ /**
+ * The client correlation ID provided over config that will be added to
+ * x-ms-client-request-Id header. Defaults to empty string if the length and
+ * character constraints are not satisfied. **/
+ public static final String FS_AZURE_CLIENT_CORRELATIONID =
"fs.azure.client.correlationid";
+ public static final String FS_AZURE_TRACINGCONTEXT_FORMAT =
"fs.azure.tracingcontext.format";
Review comment:
This is the tracing header format right? Will that be a better name?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsHttpOperation.java
##########
@@ -166,6 +166,11 @@ public String getResponseHeader(String httpHeader) {
return connection.getHeaderField(httpHeader);
}
+ @VisibleForTesting
+ public String getRequestHeader(String httpHeader) {
+ return connection.getRequestProperties().get(httpHeader).toString();
Review comment:
Only used for tests? Should be returning List only?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java
##########
@@ -221,17 +226,27 @@ private void completeExecute() throws
AzureBlobFileSystemException {
LOG.trace("{} REST operation complete", operationType);
}
+ private void updateClientRequestHeader(AbfsHttpOperation httpOperation,
+ TracingContext tracingContext) {
+ tracingContext.generateClientRequestID();
Review comment:
Can avoid this way of explicit call which for sure should get executed
before setting of this header.
Can we have a better method name than toString() for generation of required
header value? This has to consider the format and generate.
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
##########
@@ -1049,8 +1130,11 @@ public boolean exists(Path f) throws IOException {
throws IOException {
LOG.debug("AzureBlobFileSystem.listStatusIterator path : {}", path);
if (abfsStore.getAbfsConfiguration().enableAbfsListIterator()) {
+ TracingContext tracingContext = new TracingContext(clientCorrelationID,
+ fileSystemID, HdfsOperationConstants.LISTSTATUS, true,
+ tracingContextFormat, listener);
AbfsListStatusRemoteIterator abfsLsItr =
- new AbfsListStatusRemoteIterator(getFileStatus(path), abfsStore);
+ new AbfsListStatusRemoteIterator(getFileStatus(path), abfsStore,
tracingContext);
return RemoteIterators.typeCastingRemoteIterator(abfsLsItr);
} else {
return super.listStatusIterator(path);
Review comment:
Even this call to FileSystem#listStatusIterator() will create ADL gen2
calls right? So should there be way for having a single context(above created)
to be used for call from there too?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
##########
@@ -1283,6 +1373,11 @@ public String getCanonicalServiceName() {
return this.statistics;
}
+ @VisibleForTesting
+ void setListenerOperation(String operation) {
+ listener.setOperation(operation);
Review comment:
What is this operation been set on Listener?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/HdfsOperationConstants.java
##########
@@ -0,0 +1,50 @@
+/**
+ * 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;
+
+public final class HdfsOperationConstants {
Review comment:
Can we avoid the hdfs in this ? Its after all Hadoop common FS API types
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
##########
@@ -111,10 +116,14 @@
private Path workingDir;
private AzureBlobFileSystemStore abfsStore;
private boolean isClosed;
+ private final String fileSystemID = UUID.randomUUID().toString();
Review comment:
All places ID to Id?
##########
File path:
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/HdfsOperationConstants.java
##########
@@ -0,0 +1,50 @@
+/**
+ * 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;
+
+public final class HdfsOperationConstants {
Review comment:
Can be Enum?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 614331)
Time Spent: 4h 10m (was: 4h)
> ABFS: Add Identifiers to Client Request Header
> ----------------------------------------------
>
> Key: HADOOP-17290
> URL: https://issues.apache.org/jira/browse/HADOOP-17290
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/azure
> Affects Versions: 3.3.0
> Reporter: Sumangala Patki
> Assignee: Sumangala Patki
> Priority: Major
> Labels: abfsactive, pull-request-available
> Time Spent: 4h 10m
> Remaining Estimate: 0h
>
> Adding unique values to the client request header to assist in correlating
> requests
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
