-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-18960: [ABFS] Making Contract tests run in sequential and Other Test Fixes #7104
base: trunk
Are you sure you want to change the base?
Conversation
:::: AGGREGATED TEST RESULT :::: ============================================================
|
@steveloughran @mukund-thakur |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all test tuning and I'll assume it works.
some minor nits, especially on cutting all the this.
terms.
they aren't the normal style in this project
@@ -201,4 +183,20 @@ public void testAuthFailException() throws Exception { | |||
.describedAs("Incorrect error message: " + errorDesc) | |||
.contains("Auth failure: "); | |||
} | |||
|
|||
private Configuration getEntries(final int numOfRetries) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: javadoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
@@ -65,8 +67,8 @@ public void setup() throws Exception { | |||
TestConfigurationKeys.FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT, false); | |||
Assume.assumeTrue(isHNSEnabled); | |||
loadConfiguredFileSystem(); | |||
this.getConfiguration().set(ConfigurationKeys.FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, TEST_AUTHZ_CLASS); | |||
this.getConfiguration().set(ConfigurationKeys.FS_AZURE_ACCOUNT_AUTH_TYPE_PROPERTY_NAME, "SAS"); | |||
this.getConfiguration().set(FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, TEST_AUTHZ_CLASS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cut the this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@@ -68,6 +68,9 @@ protected Configuration createConfiguration() { | |||
protected AbstractFSContract createContract(final Configuration conf) { | |||
conf.setInt(AZURE_READ_AHEAD_RANGE, MIN_BUFFER_SIZE); | |||
conf.setInt(AZURE_READ_BUFFER_SIZE, MIN_BUFFER_SIZE); | |||
// Disabling cache to make sure new configs are picked up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pull out into an azure test utils class, just as `S3ATestUtils.disableFilesystemCaching()does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestions. Taken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already had a AbfsTestUtils class defined. Added there itself.
I noticed that class has other methods without any usage. Not sure how but was wondering should they be removed or not.
config.set(AZURE_CUSTOM_TOKEN_FETCH_RETRY_COUNT, Integer.toString( | ||
numOfRetries)); | ||
// Stop filesystem creation as it will lead to calls to store. | ||
config.set(AZURE_CREATE_REMOTE_FILESYSTEM_DURING_INITIALIZATION, "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setInteger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken everywhere applicable
numOfRetries)); | ||
// Stop filesystem creation as it will lead to calls to store. | ||
config.set(AZURE_CREATE_REMOTE_FILESYSTEM_DURING_INITIALIZATION, "false"); | ||
config.set(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, config.getBoolean( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setBoolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some cosmetic changes. Overall good to go
testConfig.set(ConfigurationKeys.FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, TEST_ERR_AUTHZ_CLASS); | ||
Configuration testConfig = new Configuration(this.getConfiguration().getRawConfiguration()); | ||
testConfig.set(FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, TEST_ERR_AUTHZ_CLASS); | ||
testConfig.set(MOCK_SASTOKENPROVIDER_RETURN_EMPTY_SAS_TOKEN, "true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setBoolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken
testConfig.set(FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, TEST_ERR_AUTHZ_CLASS); | ||
testConfig.set(MOCK_SASTOKENPROVIDER_RETURN_EMPTY_SAS_TOKEN, "true"); | ||
// Setting IS_HNS_ENABLED to avoid the exception thrown by the HNS check. | ||
testConfig.set(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, this.getIsNamespaceEnabled(fs) + ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to append "" at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was needed to conver boolean to string. But with setBoolean not needed.
@@ -68,6 +68,9 @@ protected Configuration createConfiguration() { | |||
protected AbstractFSContract createContract(final Configuration conf) { | |||
conf.setInt(AZURE_READ_AHEAD_RANGE, MIN_BUFFER_SIZE); | |||
conf.setInt(AZURE_READ_BUFFER_SIZE, MIN_BUFFER_SIZE); | |||
// Disabling cache to make sure new configs are picked up. | |||
conf.set("fs.abfss.impl.disable.cache", "true"); | |||
conf.set("fs.abfs.impl.disable.cache", "true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setBoolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apart from that little ordering; code is ready.
Now, we just need to get through yetus. I didn't like it last time. why not do a git merge to pull all of trunk in and push up again, to make sure all is in sync.
@@ -21,6 +21,7 @@ | |||
import com.microsoft.azure.storage.blob.CloudBlobClient; | |||
import com.microsoft.azure.storage.blob.CloudBlobContainer; | |||
|
|||
import org.apache.hadoop.conf.Configuration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: import ordering
💔 -1 overall
This message was automatically generated. |
Thanks for suggestions @steveloughran |
Description of PR
Target Jira: https://issues.apache.org/jira/browse/HADOOP-18960
The execution of contract-tests in
abfs
works as perexecutionId = integration-test-abfs-parallel-classes
of the pom. The tests would work in different jvms, and at a given instance multiple such jvms could be there, depending on ${testsThreadCount}. The problem is that all the test jvms for contract-test use the same container for test runs which is defined byfs.contract.test.fs.abfs
. Due to this, one jvm root-contract-runs can influence other jvm's root-contract-runs. This leads to CI failures for hadoop-azure package.Hence making these tests run in parallel so that they don't interfere with each other.
This PR also fixes some test failures reported by Jira: https://issues.apache.org/jira/browse/HADOOP-19106
How was this patch tested?
Test Suite Run Result pasted in comments below.
The failing tests are known and due to some missing configs. They are fixed in #6847
For code changes: