HADOOP-17191. ABFS: Run tests with all AuthTypes #2202
Closed
+1,234
−174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Driver test results using accounts in Central India [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 |
bilaharith
force-pushed
the
allauthtests
branch
5 times, most recently
from
53a7f00 to
79b9109
Compare
bilaharith
force-pushed
the
allauthtests
branch
from
79b9109 to
f3fa582
Compare
bilaharith
force-pushed
the
allauthtests
branch
from
c29d4ec to
fdf44b6
Compare
steveloughran
requested changes
Aug 22, 2020
There was a problem hiding this comment.
This is a big change to the test set up.
My ability to review this is hampered by the fact there is no explanation in the JIRA or PR as to what it is directly trying to do.
Proposed
- a paragraph in both outlining what your goal is
- details in testing_azure.md, including detailed instructions for people writing new test as to what they are expected to do.
Now is it is now a requirement that people running tests have multiple authentication mechanisms? Because we been having problems recently with test to have different expectations from some of us testers. Will this help there or would it make things worse?
Various tests are fiddling with test timeouts. Either things have got very slow are you been changing them so you can debug stuff. If you do have to multiply what there is, why not make this a single constant?
finally, my comments on imports are predictable. Save time by guessing what I will say.
| @@ -214,6 +214,8 @@ private boolean executeHttpOperation(final int retryCount) throws AzureBlobFileS | |||
| httpOperation = new AbfsHttpOperation(url, method, requestHeaders); | |||
| incrementCounter(AbfsStatistic.CONNECTIONS_MADE, 1); | |||
|
|
|||
| LOG.error("{}.{}", client.getAuthType(), url); | |||
| @@ -23,9 +23,12 @@ | |||
| */ | |||
| public final class TestConfigurationKeys { | |||
| public static final String FS_AZURE_ACCOUNT_NAME = "fs.azure.account.name"; | |||
| public static final String FS_AZURE_ABFS_HNS_ACCOUNT_NAME = "fs.azure.hns.abfs.account.name"; | |||
| import org.apache.hadoop.fs.Path; | ||
| import org.apache.hadoop.fs.azurebfs.constants.FileSystemUriSchemes; | ||
| import org.apache.hadoop.fs.azurebfs.utils.UriUtils; | ||
| import org.apache.hadoop.fs.contract.AbstractBondedFSContract; | ||
|
|
||
| import java.io.IOException; |
| protected Configuration createConfiguration() { | ||
| return this.binding.getRawConfiguration(); | ||
| protected int getTestTimeoutMillis() { | ||
| return 3 * 180000; |
There was a problem hiding this comment.
why the change? and use 9 * 60_000 for readability
| */ | ||
| @InterfaceAudience.Private | ||
| @InterfaceStability.Evolving | ||
| package org.apache.hadoop.fs.azurebfs.rules; |
There was a problem hiding this comment.
add a javadoc to describe package
| @@ -25,13 +25,19 @@ | |||
| import java.util.UUID; | |||
| import java.util.concurrent.Callable; | |||
|
|
|||
| import org.apache.hadoop.fs.azurebfs.constants.TestConfigurationKeys; | |||
| @@ -50,20 +56,24 @@ | |||
| import org.apache.hadoop.fs.permission.FsPermission; | |||
| import org.apache.hadoop.io.IOUtils; | |||
|
|
|||
| import static org.junit.Assume.assumeTrue; | |||
There was a problem hiding this comment.
avoid import reordering
| @@ -54,6 +55,10 @@ public ITestAbfsClient() throws Exception { | |||
| super(); | |||
| } | |||
|
|
|||
| protected int getTestTimeoutMillis() { | |||
| return 3 * TEST_TIMEOUT; | |||
There was a problem hiding this comment.
really that long? Why the hard coding
| import java.io.FileNotFoundException; | ||
| import java.util.List; | ||
| import java.util.UUID; | ||
|
|
||
| import org.junit.Assume; | ||
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
| import com.google.common.collect.Lists; |
There was a problem hiding this comment.
import reordering. Once in, best to leave alone unless you enjoy more merge pain than normal.
|
💔 -1 overall
This message was automatically generated. |
Thanks @steveloughran for the comments. I have still kept this PR as draft(work in progress). The comments will be addressed once the same is ready to review. |
steveloughran
added
fs/azure
|
ok...labelled as WiP. Thanks for the docs, makes it a lot clearer |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Driver test results using accounts in Canary region [INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 [ERROR] Errors: [WARNING] Tests run: 207, Failures: 0, Errors: 0, Skipped: 16 |
|
🎊 +1 overall
This message was automatically generated. |
|
why closed? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ADLS Gen 2 supports accounts with and without hierarchical namespace support.
ABFS driver supports various authorization mechanisms like OAuth, SharedKey,
Shared Access Signature. The integration tests need to be executed against
accounts with and without hierarchical namespace support using various
authorization mechanisms.
The aforementioned combinations of tests are ran automatically. Currently,
tests are ran against accounts with and without hierarchical name space
support. Authorisation mechanisms supported are OAuth and SharedKey. The
combinations to run the tests against should be configured in the
azure-auth-keys.xml using the property
fs.azure.test.combinations. Eachcombination has to be separated with a
|. The first config to mention inshould be the account type (HNS and NonHNS). Second one being the
authorisation type (OAuth, SharedKey). The configuration values has to be
separated with
-. An example value would beHNS-SharedKey|HNS-OAuth|NonHNS-SharedKey. Account names for both HNS andNonHNS has to be specified within the configuration file using the keys
fs.azure.hns.abfs.account.nameandfs.azure.nonhns.abfs.account.name.How to write test cases to run against all the configured combinations
A test written in a test class is run with all the combinations configured if
the class implements the AbfsTestable interface and a rule instance of type
AbfsTestsRule is part of the class. In case if a particular test case needs
to run with only a certain combinations the same can be mentioned with the
method level annotation AbfsConfigsToTest. If a hadoop contract test class
needs to exclude certain auth types, the same cannot be done with the
AbfsConfigsToTest annotation, as the tests are present in the hadoop-common
module. The same can be achieved by implementing the excludeAuthTypes method
of the AbfsTestable interface.