-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-12315. Speed up some Freon integration tests #7870
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
Changes from all commits
ac92f68
cc6c46e
9a7b2c3
f98997a
7c8219a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,8 @@ | |||||
| import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_FS_LISTING_PAGE_SIZE; | ||||||
| import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_REPLICATION; | ||||||
| import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_REPLICATION_TYPE; | ||||||
| import static org.apache.hadoop.ozone.OzoneConsts.OZONE_OFS_URI_SCHEME; | ||||||
| import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_SCHEME; | ||||||
| import static org.assertj.core.api.Assertions.assertThat; | ||||||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||||||
|
|
||||||
|
|
@@ -33,17 +35,30 @@ | |||||
| import org.apache.hadoop.fs.Path; | ||||||
| import org.apache.hadoop.fs.RemoteIterator; | ||||||
| import org.apache.hadoop.fs.contract.ContractTestUtils; | ||||||
| import org.apache.hadoop.hdds.conf.ConfigurationTarget; | ||||||
| import org.apache.hadoop.hdds.conf.OzoneConfiguration; | ||||||
| import org.apache.ratis.util.Preconditions; | ||||||
|
|
||||||
| /** | ||||||
| * Common test cases for Ozone file systems. | ||||||
| */ | ||||||
| final class OzoneFileSystemTests { | ||||||
| public final class OzoneFileSystemTests { | ||||||
|
|
||||||
| private OzoneFileSystemTests() { | ||||||
| // no instances | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Set file system listing page size. Also disable the file system cache to | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not forget to remove this space :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I use two spaces intentionally. |
||||||
| * ensure new {@link FileSystem} instance reflects the updated page size. | ||||||
| */ | ||||||
| public static void setPageSize(ConfigurationTarget conf, int pageSize) { | ||||||
| Preconditions.assertTrue(pageSize > 0, () -> "pageSize=" + pageSize + " <= 0"); | ||||||
| conf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize); | ||||||
| conf.setBoolean(String.format("fs.%s.impl.disable.cache", OZONE_URI_SCHEME), true); | ||||||
| conf.setBoolean(String.format("fs.%s.impl.disable.cache", OZONE_OFS_URI_SCHEME), true); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Tests listStatusIterator operation on directory with different | ||||||
| * numbers of child directories. | ||||||
|
|
@@ -60,10 +75,8 @@ public static void listStatusIteratorOnPageSize(OzoneConfiguration conf, | |||||
| pageSize + pageSize | ||||||
| }; | ||||||
| OzoneConfiguration config = new OzoneConfiguration(conf); | ||||||
| config.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize); | ||||||
| setPageSize(config, pageSize); | ||||||
| URI uri = FileSystem.getDefaultUri(config); | ||||||
| config.setBoolean( | ||||||
| String.format("fs.%s.impl.disable.cache", uri.getScheme()), true); | ||||||
| try (FileSystem subject = FileSystem.get(uri, config)) { | ||||||
| Path dir = new Path(Objects.requireNonNull(rootPath), | ||||||
| "listStatusIterator"); | ||||||
|
|
||||||
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.
Can we rename this class to something like OzoneFileSystemTestUtils? Right now it feels like it is a test case rather than collection of utils.
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.
There is lot of duplication between
AbstractOzoneFileSystemTestandAbstractRootedOzoneFileSystemTest. This class is intended as a place where we would move those common test cases, not as a utility. So far onlylistStatusIteratorOnPageSizewas moved in HDDS-9328.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.
Thanks for clarification!
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.
Created HDDS-12355.
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.
Can you add TODO with Jira link?
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.
We use Jira for tasks, not TODO items in the code. If someone reports a bug or an improvement idea, we don't go and edit code just to mention those.
Also, the item we discussed is not related to the changes being done in the PR, which touches this class only superficially. If I was working on a series of tasks, it would be OK to mention a follow-up task in a comment to help remember items or details about the follow-up.