[GitHub] [hadoop] saxenapranav commented on a diff in pull request #5446: HADOOP-18640: [ABFS] Enabling Client-side Backoff only for new requests
saxenapranav commented on code in PR #5446: URL: https://github.com/apache/hadoop/pull/5446#discussion_r1130357920 ## hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java: ## @@ -268,10 +269,62 @@ DefaultValue = DEFAULT_ACCOUNT_OPERATION_IDLE_TIMEOUT_MS) private int accountOperationIdleTimeout; + /** + * Analysis Period for client-side throttling + */ Review Comment: nit: spacing. -- 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
[GitHub] [hadoop] saxenapranav commented on a diff in pull request #5446: HADOOP-18640: [ABFS] Enabling Client-side Backoff only for new requests
saxenapranav commented on code in PR #5446:
URL: https://github.com/apache/hadoop/pull/5446#discussion_r1130357512
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##
@@ -222,6 +224,10 @@ AbfsThrottlingIntercept getIntercept() {
return intercept;
}
+ boolean shouldThrottleRetries() {
+return throttleRetries;
+ }
+
Review Comment:
Either of two is fine.
if we keep in abfsClient, it will be stored account level, and we dont need
to check anything till abfsClient object is alived. In abfsRestOp, new field
will be created as new object is created for each api call.
if we keep it in abfsRestOperation, it is something which is actually
requried in abfsRestOperation. Though it doesn't matter. You may please resolve
this comment.
--
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
[GitHub] [hadoop] saxenapranav commented on a diff in pull request #5446: HADOOP-18640: [ABFS] Enabling Client-side Backoff only for new requests
saxenapranav commented on code in PR #5446:
URL: https://github.com/apache/hadoop/pull/5446#discussion_r1127368834
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -285,6 +299,154 @@ public void testAbfsConfigConstructor() throws Exception {
Assert.assertEquals("Delta backoff interval was not set as expected.",
expectedDeltaBackoff, policy.getDeltaBackoff());
}
+// public void testClientBackoffOnlyNewRequest() throws IOException {
+ @Test
+ public void testClientBackoffOnlyNewWriteRequest() throws IOException,
InterruptedException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = fs.getAbfsStore().getClient();
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+URL dummyUrl = client.createRequestUrl("/", "");
+String dummyMethod = HTTP_METHOD_PUT;
+
+AbfsRestOperationType testOperationType = AbfsRestOperationType.Append;
+
+AbfsRestOperation restOp = new AbfsRestOperation(testOperationType,
client, dummyMethod, dummyUrl, new ArrayList<>());
+
+Long writeThrottleStatBefore =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+Thread.sleep(1);
+boolean appliedBackoff = restOp.applyThrottlingBackoff(0,
testOperationType, counters);
+assertEquals(true, appliedBackoff);
+Long writeThrottleStatAfter =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+assertEquals(new Long(writeThrottleStatBefore+1), writeThrottleStatAfter);
+
+
+writeThrottleStatBefore =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+appliedBackoff = restOp.applyThrottlingBackoff(1, testOperationType,
counters);
+assertEquals(false, appliedBackoff);
+writeThrottleStatAfter =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+assertEquals(writeThrottleStatBefore, writeThrottleStatAfter);
+ }
+
+ @Test
+ public void testClientBackoffOnlyNewReadRequest() throws IOException,
InterruptedException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = fs.getAbfsStore().getClient();
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+URL dummyUrl = client.createRequestUrl("/", "");
+String dummyMethod = AbfsHttpConstants.HTTP_METHOD_GET;
+
+AbfsRestOperationType testOperationType = AbfsRestOperationType.ReadFile;
+
+AbfsRestOperation restOp = new AbfsRestOperation(testOperationType,
client, dummyMethod, dummyUrl, new ArrayList<>());
+
+Long readThrottleStatBefore =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+Thread.sleep(1);
+boolean appliedBackoff = restOp.applyThrottlingBackoff(0,
testOperationType, counters);
+assertEquals(true, appliedBackoff);
+Long readThrottleStatAfter =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+assertEquals(new Long(readThrottleStatBefore+1), readThrottleStatAfter);
+
+
+readThrottleStatBefore =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+appliedBackoff = restOp.applyThrottlingBackoff(1, testOperationType,
counters);
+assertEquals(false, appliedBackoff);
+readThrottleStatAfter =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+assertEquals(readThrottleStatBefore, readThrottleStatAfter);
+ }
+
+ @Test
+ public void testReadThrottleNewRequest() throws IOException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = Mockito.spy(fs.getAbfsStore().getClient());
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+AbfsThrottlingIntercept intercept =
Mockito.mock(AbfsThrottlingIntercept.class);
+
Mockito.doNothing().when(intercept).sendingRequest(Mockito.any(AbfsRestOperationType.class),
Mockito.any(AbfsCounters.class));
+Mockito.doReturn(intercept).when(client).getIntercept();
+
+// setting up the spy AbfsRestOperation class for read
+final List requestHeaders = client.createDefaultHeaders();
+
+final AbfsUriQueryBuilder abfsUriQueryBuilder =
client.createDefaultUriQueryBuilder();
+
+final URL url = client.createRequestUrl("/dummyReadFile",
abfsUriQueryBuilder.toString());
+final AbfsRestOperation mockRestOp = Mockito.spy(new AbfsRestOperation(
+AbfsRestOperationType.ReadFile,
+client,
+HTTP_METHOD_GET,
+url,
+requestHeaders));
+
+// setting up mock behavior for the AbfsHttpOperation class
+AbfsHttpOperation mockHttpOp =
[GitHub] [hadoop] saxenapranav commented on a diff in pull request #5446: HADOOP-18640: [ABFS] Enabling Client-side Backoff only for new requests
saxenapranav commented on code in PR #5446:
URL: https://github.com/apache/hadoop/pull/5446#discussion_r1127328623
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##
@@ -268,10 +269,62 @@
DefaultValue = DEFAULT_ACCOUNT_OPERATION_IDLE_TIMEOUT_MS)
private int accountOperationIdleTimeout;
+ /**
+ * Analysis Period for client-side throttling
+ */
@IntegerConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_ANALYSIS_PERIOD,
DefaultValue = DEFAULT_ANALYSIS_PERIOD_MS)
private int analysisPeriod;
+ /**
+ * Lower limit of acceptable error percentage
+ */
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_MIN_ACCEPTABLE_ERROR_PERCENTAGE,
+ DefaultValue = DEFAULT_MIN_ACCEPTABLE_ERROR_PERCENTAGE)
+ private double minAcceptableErrorPercentage;
+
+ /**
+ * Maximum equilibrium error percentage
+ */
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_MAX_EQUILIBRIUM_ERROR_PERCENTAGE,
+ DefaultValue = DEFAULT_MAX_EQUILIBRIUM_ERROR_PERCENTAGE)
+ private double maxEquilibriumErrorPercentage;
+
+ /**
+ * Rapid sleep decrease factor to increase throughput
+ */
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_RAPID_SLEEP_DECREASE_FACTOR,
+ DefaultValue = DEFAULT_RAPID_SLEEP_DECREASE_FACTOR)
+ private double rapidSleepDecreaseFactor;
+
+ /**
+ * Rapid sleep decrease transition period in milliseconds
+ */
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_RAPID_SLEEP_DECREASE_TRANSITION_MS,
+ DefaultValue = DEFAULT_RAPID_SLEEP_DECREASE_TRANSITION_PERIOD_MS)
+ private double rapidSleepDecreaseTransitionPeriodMs;
+
+ /**
+ * Sleep decrease factor to increase throughput
+ */
Review Comment:
fix spacing in all javadocs wherever required.
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -20,28 +20,42 @@
import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR;
+import static java.net.HttpURLConnection.HTTP_OK;
+import static
org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_METHOD_GET;
+import static
org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_METHOD_PUT;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_BACKOFF_INTERVAL;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MAX_BACKOFF_INTERVAL;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MAX_IO_RETRIES;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.AZURE_MIN_BACKOFF_INTERVAL;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_LEVEL_THROTTLING_ENABLED;
import static
org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ENABLE_AUTOTHROTTLING;
import static
org.apache.hadoop.fs.azurebfs.constants.FileSystemConfigurations.MIN_BUFFER_SIZE;
+import static
org.apache.hadoop.fs.azurebfs.constants.HttpHeaderConfigurations.IF_MATCH;
+import static
org.apache.hadoop.fs.azurebfs.constants.HttpHeaderConfigurations.RANGE;
import static
org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_ABFS_ACCOUNT1_NAME;
import static
org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.FS_AZURE_ACCOUNT_NAME;
import static
org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys.TEST_CONFIGURATION_FILE_NAME;
import static org.junit.Assume.assumeTrue;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
Review Comment:
fix wildcard. You can switch off wildcard imports on IDE so in future it
doesnt come.
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -285,6 +299,154 @@ public void testAbfsConfigConstructor() throws Exception {
Assert.assertEquals("Delta backoff interval was not set as expected.",
expectedDeltaBackoff, policy.getDeltaBackoff());
}
+// public void testClientBackoffOnlyNewRequest() throws IOException {
+ @Test
+ public void testClientBackoffOnlyNewWriteRequest() throws IOException,
InterruptedException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = fs.getAbfsStore().getClient();
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+URL dummyUrl = client.createRequestUrl("/", "");
+String dummyMethod = HTTP_METHOD_PUT;
+
+AbfsRestOperationType testOperationType = AbfsRestOperationType.Append;
+
+AbfsRestOperation restOp = new AbfsRestOperation(testOperationType,
client, dummyMethod, dummyUrl, new ArrayList<>());
+
+Long
[GitHub] [hadoop] saxenapranav commented on a diff in pull request #5446: HADOOP-18640: [ABFS] Enabling Client-side Backoff only for new requests
saxenapranav commented on code in PR #5446:
URL: https://github.com/apache/hadoop/pull/5446#discussion_r1126166721
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -285,6 +299,154 @@ public void testAbfsConfigConstructor() throws Exception {
Assert.assertEquals("Delta backoff interval was not set as expected.",
expectedDeltaBackoff, policy.getDeltaBackoff());
}
+// public void testClientBackoffOnlyNewRequest() throws IOException {
+ @Test
+ public void testClientBackoffOnlyNewWriteRequest() throws IOException,
InterruptedException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = fs.getAbfsStore().getClient();
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+URL dummyUrl = client.createRequestUrl("/", "");
+String dummyMethod = HTTP_METHOD_PUT;
+
+AbfsRestOperationType testOperationType = AbfsRestOperationType.Append;
+
+AbfsRestOperation restOp = new AbfsRestOperation(testOperationType,
client, dummyMethod, dummyUrl, new ArrayList<>());
+
+Long writeThrottleStatBefore =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+Thread.sleep(1);
+boolean appliedBackoff = restOp.applyThrottlingBackoff(0,
testOperationType, counters);
+assertEquals(true, appliedBackoff);
+Long writeThrottleStatAfter =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+assertEquals(new Long(writeThrottleStatBefore+1), writeThrottleStatAfter);
+
+
+writeThrottleStatBefore =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+appliedBackoff = restOp.applyThrottlingBackoff(1, testOperationType,
counters);
+assertEquals(false, appliedBackoff);
+writeThrottleStatAfter =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+assertEquals(writeThrottleStatBefore, writeThrottleStatAfter);
+ }
+
+ @Test
+ public void testClientBackoffOnlyNewReadRequest() throws IOException,
InterruptedException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = fs.getAbfsStore().getClient();
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+URL dummyUrl = client.createRequestUrl("/", "");
+String dummyMethod = AbfsHttpConstants.HTTP_METHOD_GET;
+
+AbfsRestOperationType testOperationType = AbfsRestOperationType.ReadFile;
+
+AbfsRestOperation restOp = new AbfsRestOperation(testOperationType,
client, dummyMethod, dummyUrl, new ArrayList<>());
+
+Long readThrottleStatBefore =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+Thread.sleep(1);
+boolean appliedBackoff = restOp.applyThrottlingBackoff(0,
testOperationType, counters);
+assertEquals(true, appliedBackoff);
+Long readThrottleStatAfter =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+assertEquals(new Long(readThrottleStatBefore+1), readThrottleStatAfter);
+
+
+readThrottleStatBefore =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+appliedBackoff = restOp.applyThrottlingBackoff(1, testOperationType,
counters);
+assertEquals(false, appliedBackoff);
+readThrottleStatAfter =
counters.toMap().get(AbfsStatistic.READ_THROTTLES.getStatName());
+assertEquals(readThrottleStatBefore, readThrottleStatAfter);
+ }
+
+ @Test
+ public void testReadThrottleNewRequest() throws IOException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = Mockito.spy(fs.getAbfsStore().getClient());
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+AbfsThrottlingIntercept intercept =
Mockito.mock(AbfsThrottlingIntercept.class);
+
Mockito.doNothing().when(intercept).sendingRequest(Mockito.any(AbfsRestOperationType.class),
Mockito.any(AbfsCounters.class));
+Mockito.doReturn(intercept).when(client).getIntercept();
+
+// setting up the spy AbfsRestOperation class for read
+final List requestHeaders = client.createDefaultHeaders();
+
+final AbfsUriQueryBuilder abfsUriQueryBuilder =
client.createDefaultUriQueryBuilder();
+
+final URL url = client.createRequestUrl("/dummyReadFile",
abfsUriQueryBuilder.toString());
+final AbfsRestOperation mockRestOp = Mockito.spy(new AbfsRestOperation(
+AbfsRestOperationType.ReadFile,
+client,
+HTTP_METHOD_GET,
+url,
+requestHeaders));
+
+// setting up mock behavior for the AbfsHttpOperation class
+AbfsHttpOperation mockHttpOp =
[GitHub] [hadoop] saxenapranav commented on a diff in pull request #5446: HADOOP-18640: [ABFS] Enabling Client-side Backoff only for new requests
saxenapranav commented on code in PR #5446:
URL: https://github.com/apache/hadoop/pull/5446#discussion_r1125941455
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java:
##
@@ -50,6 +50,14 @@ public final class FileSystemConfigurations {
public static final int
DEFAULT_AZURE_OAUTH_TOKEN_FETCH_RETRY_MAX_BACKOFF_INTERVAL = SIXTY_SECONDS;
public static final int DEFAULT_AZURE_OAUTH_TOKEN_FETCH_RETRY_DELTA_BACKOFF
= 2;
+ // Throttling Analysis defaults.
+ public static final double DEFAULT_MIN_ACCEPTABLE_ERROR_PERCENTAGE = .1;
Review Comment:
nit:`0.1`
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -285,6 +299,154 @@ public void testAbfsConfigConstructor() throws Exception {
Assert.assertEquals("Delta backoff interval was not set as expected.",
expectedDeltaBackoff, policy.getDeltaBackoff());
}
+// public void testClientBackoffOnlyNewRequest() throws IOException {
+ @Test
+ public void testClientBackoffOnlyNewWriteRequest() throws IOException,
InterruptedException {
+AzureBlobFileSystem fs = getFileSystem();
+AbfsClient client = fs.getAbfsStore().getClient();
+AbfsConfiguration configuration = client.getAbfsConfiguration();
+Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+AbfsCounters counters = client.getAbfsCounters();
+
+URL dummyUrl = client.createRequestUrl("/", "");
+String dummyMethod = HTTP_METHOD_PUT;
+
+AbfsRestOperationType testOperationType = AbfsRestOperationType.Append;
+
+AbfsRestOperation restOp = new AbfsRestOperation(testOperationType,
client, dummyMethod, dummyUrl, new ArrayList<>());
+
+Long writeThrottleStatBefore =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+Thread.sleep(1);
+boolean appliedBackoff = restOp.applyThrottlingBackoff(0,
testOperationType, counters);
+assertEquals(true, appliedBackoff);
+Long writeThrottleStatAfter =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+assertEquals(new Long(writeThrottleStatBefore+1), writeThrottleStatAfter);
+
+
+writeThrottleStatBefore =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+appliedBackoff = restOp.applyThrottlingBackoff(1, testOperationType,
counters);
+assertEquals(false, appliedBackoff);
+writeThrottleStatAfter =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+assertEquals(writeThrottleStatBefore, writeThrottleStatAfter);
+ }
Review Comment:
what is happening in throttlingIntercept is out of scope for the pr. its
not testing what the change we want to have. What its touching is that if the
if-else block is working correct or not. even if we want to to test it, lets
have intercept mocked and just check if `intercept.sendingRequest` is being
called or not.
What i want to say is that, lets not get into whats happening inside
throttlingIntercept.
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java:
##
@@ -50,6 +50,14 @@ public final class FileSystemConfigurations {
public static final int
DEFAULT_AZURE_OAUTH_TOKEN_FETCH_RETRY_MAX_BACKOFF_INTERVAL = SIXTY_SECONDS;
public static final int DEFAULT_AZURE_OAUTH_TOKEN_FETCH_RETRY_DELTA_BACKOFF
= 2;
+ // Throttling Analysis defaults.
+ public static final double DEFAULT_MIN_ACCEPTABLE_ERROR_PERCENTAGE = .1;
Review Comment:
please check other double values in default configs added.
nit comment.
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java:
##
@@ -334,6 +337,25 @@ private boolean executeHttpOperation(final int retryCount,
return true;
}
+ /**
+ * Makes a call for client side throttling based on
+ * the request count.
+ * @param operationType operation type of current request
+ * @param abfsCounters AbfsCounters instance
+ */
+ @VisibleForTesting
+ boolean applyThrottlingBackoff(int retryCount, AbfsRestOperationType
operationType, AbfsCounters abfsCounters) {
+if (retryCount == 0) {
+ intercept.sendingRequest(operationType, abfsCounters);
+ return true;
+}
+return false;
+ }
+
+ public AbfsHttpOperation createHttpOperationInstance() throws IOException {
Review Comment:
dont have it public. have it like:
```
AbfsHttpOperation createHttpOperationInstance() throws IOException {
```
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##
@@ -268,10 +269,55 @@
DefaultValue = DEFAULT_ACCOUNT_OPERATION_IDLE_TIMEOUT_MS)
private int accountOperationIdleTimeout;
+ /*
+ Analysis Period for client-side throttling
+ */
@IntegerConfigurationValidatorAnnotation(ConfigurationKey =
[GitHub] [hadoop] saxenapranav commented on a diff in pull request #5446: HADOOP-18640: [ABFS] Enabling Client-side Backoff only for new requests
saxenapranav commented on code in PR #5446:
URL: https://github.com/apache/hadoop/pull/5446#discussion_r1122694142
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##
@@ -272,6 +273,30 @@
DefaultValue = DEFAULT_ANALYSIS_PERIOD_MS)
private int analysisPeriod;
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_MIN_ACCEPTABLE_ERROR_PERCENTAGE,
+ DefaultValue = DEFAULT_MIN_ACCEPTABLE_ERROR_PERCENTAGE)
+ private double minAcceptableErrorPercentage;
+
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_MAX_EQUILIBRIUM_ERROR_PERCENTAGE,
+ DefaultValue = DEFAULT_MAX_EQUILIBRIUM_ERROR_PERCENTAGE)
+ private double maxEquilibriumErrorPercentage;
+
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_RAPID_SLEEP_DECREASE_FACTOR,
+ DefaultValue = DEFAULT_RAPID_SLEEP_DECREASE_FACTOR)
+ private double rapidSleepDecreaseFactor;
+
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_RAPID_SLEEP_DECREASE_TRANSITION_MS,
+ DefaultValue = DEFAULT_RAPID_SLEEP_DECREASE_TRANSITION_PERIOD_MS)
+ private double rapidSleepDecreaseTransitionPeriodMs;
+
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_SLEEP_DECREASE_FACTOR,
+ DefaultValue = DEFAULT_SLEEP_DECREASE_FACTOR)
+ private double sleepDecreaseFactor;
+
+ @DoubleConfigurationValidatorAnnotation(ConfigurationKey =
FS_AZURE_SLEEP_INCREASE_FACTOR,
+ DefaultValue = DEFAULT_SLEEP_INCREASE_FACTOR)
+ private double sleepIncreaseFactor;
Review Comment:
lets add documentation of what each field means.
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -285,6 +293,67 @@ public void testAbfsConfigConstructor() throws Exception {
Assert.assertEquals("Delta backoff interval was not set as expected.",
expectedDeltaBackoff, policy.getDeltaBackoff());
}
+// public void testClientBackoffOnlyNewRequest() throws IOException {
+@Test
+public void testClientBackoffOnlyNewWriteRequest() throws IOException,
InterruptedException {
+ AzureBlobFileSystem fs = getFileSystem();
+ AbfsClient client = fs.getAbfsStore().getClient();
+ AbfsConfiguration configuration = client.getAbfsConfiguration();
+ Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+ AbfsCounters counters = client.getAbfsCounters();
+
+ URL dummyUrl = client.createRequestUrl("/", "");
+ String dummyMethod = AbfsHttpConstants.HTTP_METHOD_PUT;
+
+ AbfsRestOperationType testOperationType = AbfsRestOperationType.Append;
+
+ AbfsRestOperation restOp = new AbfsRestOperation(testOperationType, client,
dummyMethod, dummyUrl, new ArrayList<>());
+
+ Long writeThrottleStatBefore =
counters.toMap().get(AbfsStatistic.WRITE_THROTTLES.getStatName());
+ Thread.sleep(1);
Review Comment:
why sleep is there?
##
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsRestOperation.java:
##
@@ -334,6 +337,21 @@ private boolean executeHttpOperation(final int retryCount,
return true;
}
+ /**
+ * Makes a call for client side throttling based on
+ * the request count.
+ * @param operationType operation type of current request
+ * @param abfsCounters AbfsCounters instance
+ */
+ @VisibleForTesting
+ boolean applyThrottlingBackoff(int retryCount, AbfsRestOperationType
operationType, AbfsCounters abfsCounters) {
+if (retryCount == 0) {
+ intercept.sendingRequest(operationType, abfsCounters);
+ return true;
+}
+return false;
+ }
+
Review Comment:
once test is refactored. lets keep
```
if (retryCount == 0) {
intercept.sendingRequest(operationType, abfsCounters);
return true;
}
```
inline to line 286.
##
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java:
##
@@ -285,6 +293,67 @@ public void testAbfsConfigConstructor() throws Exception {
Assert.assertEquals("Delta backoff interval was not set as expected.",
expectedDeltaBackoff, policy.getDeltaBackoff());
}
+// public void testClientBackoffOnlyNewRequest() throws IOException {
+@Test
+public void testClientBackoffOnlyNewWriteRequest() throws IOException,
InterruptedException {
+ AzureBlobFileSystem fs = getFileSystem();
+ AbfsClient client = fs.getAbfsStore().getClient();
+ AbfsConfiguration configuration = client.getAbfsConfiguration();
+ Assume.assumeTrue(configuration.isAutoThrottlingEnabled());
+ AbfsCounters counters = client.getAbfsCounters();
+
+ URL dummyUrl = client.createRequestUrl("/", "");
+ String dummyMethod = AbfsHttpConstants.HTTP_METHOD_PUT;
+
+ AbfsRestOperationType testOperationType = AbfsRestOperationType.Append;
+
+ AbfsRestOperation restOp = new AbfsRestOperation(testOperationType, client,
