anujmodi2021 commented on code in PR #6314: URL: https://github.com/apache/hadoop/pull/6314#discussion_r1521326382
########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java: ########## @@ -160,6 +162,7 @@ public class AzureBlobFileSystem extends FileSystem /** Storing full path uri for better logging. */ private URI fullPathUri; + private AzureBlobFileSystem metricFs = null; Review Comment: I guess its not used anymore?? ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java: ########## @@ -108,6 +108,8 @@ public final class FileSystemConfigurations { public static final boolean DEFAULT_ENABLE_FLUSH = true; public static final boolean DEFAULT_DISABLE_OUTPUTSTREAM_FLUSH = true; public static final boolean DEFAULT_ENABLE_AUTOTHROTTLING = true; + public static final int DEFAULT_METRIC_IDLE_TIMEOUT_MS = 60 * 1000; Review Comment: Use 60_000 for both the values ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java: ########## @@ -190,6 +196,8 @@ public final class ConfigurationKeys { * character constraints are not satisfied. **/ public static final String FS_AZURE_CLIENT_CORRELATIONID = "fs.azure.client.correlationid"; public static final String FS_AZURE_TRACINGHEADER_FORMAT = "fs.azure.tracingheader.format"; + public static final String FS_AZURE_TRACINGMETRICHEADER_FORMAT = "fs.azure.tracingmetricheader.format"; Review Comment: One of these only needed ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java: ########## @@ -2211,4 +2211,20 @@ private void populateRenameRecoveryStatistics( abfsCounters.incrementCounter(METADATA_INCOMPLETE_RENAME_FAILURES, 1); } } + + /** + * Sends a metric using the provided TracingContext. + * + * @param tracingContextMetric The TracingContext used for sending the metric. + * @throws AzureBlobFileSystemException If an error occurs while sending the metric. + * + * <p> + * This method retrieves filesystem properties using the specified TracingContext. The metrics are sent + * via this call in the header x-ms-feclient-metrics. As part of next iteration this additional call will be removed. + * </p> + */ + public void sendMetric(TracingContext tracingContextMetric) Review Comment: Not used, can be removed ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java: ########## @@ -1655,7 +1694,7 @@ void setIsNamespaceEnabled(final Boolean isNamespaceEnabled) { * Getter for abfsCounters from AbfsClient. * @return AbfsCounters instance. */ - protected AbfsCounters getAbfsCounters() { + public AbfsCounters getAbfsCounters() { Review Comment: Is it needs to be protected only for usage in tests? If so may be we can move the tests in same package (services package inside test) ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/contracts/services/AzureServiceErrorCode.java: ########## @@ -99,24 +104,22 @@ public static AzureServiceErrorCode getAzureServiceCode(int httpStatusCode, Stri return azureServiceErrorCode; } } - return UNKNOWN; } - public static AzureServiceErrorCode getAzureServiceCode(int httpStatusCode, String errorCode, final String errorMessage) { + public static AzureServiceErrorCode getAzureServiceCode(int httpStatusCode, String errorCode, String errorMessage) { Review Comment: Why was final removed from errorMessageString?? Also why do we need to split the error message? ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java: ########## @@ -850,6 +867,18 @@ public TracingHeaderFormat getTracingHeaderFormat() { return getEnum(FS_AZURE_TRACINGHEADER_FORMAT, TracingHeaderFormat.ALL_ID_FORMAT); } + /** + * Enum config to allow user to pick format of x-ms-client-request-id header + * @return tracingContextFormat config if valid, else default ALL_ID_FORMAT + */ + public TracingHeaderFormat getTracingMetricHeaderFormat() { Review Comment: Not used, can be removed ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java: ########## @@ -72,7 +72,7 @@ public final class FileSystemConfigurations { public static final boolean DEFAULT_AZURE_ENABLE_SMALL_WRITE_OPTIMIZATION = false; public static final int DEFAULT_READ_BUFFER_SIZE = 4 * ONE_MB; // 4 MB public static final boolean DEFAULT_READ_SMALL_FILES_COMPLETELY = false; - public static final boolean DEFAULT_OPTIMIZE_FOOTER_READ = true; + public static final boolean DEFAULT_OPTIMIZE_FOOTER_READ = false; Review Comment: We should keep it true only in the PR. To run the test maybe you can temporarily make it false, or apply the easy fix for the bug. ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java: ########## @@ -115,6 +124,14 @@ public class AbfsClient implements Closeable { private AccessTokenProvider tokenProvider; private SASTokenProvider sasTokenProvider; private final AbfsCounters abfsCounters; + private final Timer timer; + private String abfsMetricUrl = null; + private boolean isMetricConfigurationChecked = false; + private boolean metricCollectionEnabled = false; Review Comment: nit: rename to `isMetricCollectionEnabled` ########## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java: ########## @@ -115,6 +124,14 @@ public class AbfsClient implements Closeable { private AccessTokenProvider tokenProvider; private SASTokenProvider sasTokenProvider; private final AbfsCounters abfsCounters; + private final Timer timer; + private String abfsMetricUrl = null; + private boolean isMetricConfigurationChecked = false; + private boolean metricCollectionEnabled = false; + private final MetricFormat metricFormat; + private final AtomicBoolean metricCollectionStopped; Review Comment: nit: rename to `isMetricCollectionStopped` -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org