HADOOP-17811: ABFS ExponentialRetryPolicy doesn't pick up configuration values #3221
Conversation
Allow ExponentialRetryPolicy to be fully configured from the hadoop configuration. Properties are already defined for this, so ensure that they are passed along when the retry policy is constructed and that documentation has been updated.
* Add spotbugs exclusion for IS2_INCONSISTENT_SYNC on NativeAzureFsInputStream.in to fix errant bug trigger introduced in PR apache#3149. * Refactor TestExponentialRetryPolicy to use a variable with the value offset rather than hard-coded numbers in order to satisfy checkstyle.
There was a problem hiding this comment.
Seems reasonable; made some minor suggestions.
One thing I like for config options is to be able to include the unit (time, capacity) in the option, e.g "500ms", "2min". Configuration.getTimeDuration() does this, including allowing a default time unit to retrofit the feature to existing properties.
Can we use that here? It may need a change in the ABFS config
Also please look at the policy in testing_azure.md and declare which abfs endpoint you tested against. No declaration: no review. Sorry, but we have to be this strict because we can't give yetus any credentials
| @@ -57,12 +61,38 @@ public void testDifferentMaxIORetryCount() throws Exception { | |||
| @Test | |||
| public void testDefaultMaxIORetryCount() throws Exception { | |||
| AbfsConfiguration abfsConfig = getAbfsConfig(); | |||
| Assert.assertTrue( | |||
| Assert.assertEquals( | |||
There was a problem hiding this comment.
If you use AssertJ.assertThat() you can use its describedAs for sprintf-style formatting. We're embracing that for new code and more complex assertions, so don't be afraid to use it here
There was a problem hiding this comment.
I only changed this line because my IDE was warning me about it--it's not otherwise relevant to my change. I looked at doing what you suggested and unfortunately the assertj version of assertThat cannot be statically imported since there are assertThat methods on the base class that the test extends. Given that, I think this change would make the code less readable.
...p-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestExponentialRetryPolicy.java
Outdated
Show resolved
Hide resolved
@steveloughran, thanks for taking a look! I addressed the suggestions that I thought made sense, and will push them in just a minute.
I like the idea, but it looks like it will be a more involved change since the configuration properties are read via annotation-driven code and updating that will be much more involved. I'm happy to take a look, but was thinking that should be a separate issue/PR.
I did what I could here in East US. The instructions aren't clear as to what must be done. I ran the runtests.sh script, according to the
I also had failures in ITestAzureBlobFileSystemLease.testFileSystemClose and ITestAzureBlobFileSystemLease.testTwoCreate. These fail when I run individually and also fail when I run the tests on trunk. I am also unable to get ITestAzureBlobFileSystemCheckAccess.testCheckAccessForAccountWithoutNS to pass (on either my branch or trunk) despite following the special instructions for that. Please let me know if there's anything else I need to run/test. Thanks. |
* Update doc formatting as requested to include backticks on class name * Fix some spelling errors in testing_azure.md * Add assertion failure messages for new test case
steveloughran
changed the title
Yeah, strange behavior. I rebuilt my storage accounts in a different tenant, and it's doing better now. I'm still seeing some weirdness in a few tests. ITestAzureBlobFileSystemLease I can get to pass if I don't use the runtests.sh script and set all of the right properties in azure-auth-keys.xml. I can't get ITestAbfsStreamStatistics to pass in the appendblob configuration (even on trunk). But other than those, everything else is running now. Thanks for merging! |
w.r.t ITestAbfsStreamStatistics, if there's no JIRA on that test failure, file one, with the stack trace. It'll inevitably be some counting mismatch Can you do the cherrypick and test of the branch-3.3 locally, let me know if it's all good there and I'll merge it in there too. |
@steveloughran my local cherry pick against branch-3.3 had no problems running tests in East US. There were two failures that ran fine individually: ITestAzureBlobFileSystemRandomRead.testValidateSeekBounds (HNS-SharedKey) and ITestAbfsListStatusRemoteIterator.testWithAbfsIteratorDisabledWithoutHasNext (NonHNS-SharedKey). |
…on values (apache#3221) Contributed by Brian Loss. (cherry picked from commit 1d03c69)
…on values (#3221) Contributed by Brian Loss. Change-Id: I5f24196d1d02de91336c3679abaf8d55cfaed746
…on values (apache#3221) Contributed by Brian Loss.
… configuration values (apache#3221) Contributed by Brian Loss. Change-Id: I5f24196d1d02de91336c3679abaf8d55cfaed746
Allow ExponentialRetryPolicy to be fully configured from the hadoop
configuration. Properties are already defined for this, so ensure that
they are passed along when the retry policy is constructed and that
documentation has been updated.