HADOOP-18399. S3A Prefetch - SingleFilePerBlockCache to use LocalDirAllocator #5054
Conversation
This file contains hidden or 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
|
Tested against
and
|
|
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HADOOP-18399-trunk
branch
from
416ce37 to
e94d0ad
Compare
|
@steveloughran, requesting your review |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Oct 26, 2022
|
|
||
| private final PrefetchingStatistics prefetchingStatistics; | ||
|
|
||
| private static LocalDirAllocator localDirAllocator; |
There was a problem hiding this comment.
shouldn't be static, as that would be shared across all fs instances, even those of different users.
better: constructor takes an allocator, which is created in s3a caching block manager.
| * @throws IOException if there is an error writing the given block. | ||
| */ | ||
| void put(int blockNumber, ByteBuffer buffer) throws IOException; | ||
| void put(int blockNumber, ByteBuffer buffer, Configuration conf, String bufferDirConf) |
There was a problem hiding this comment.
the allocator should be per instance for the cache; and so no need to pass in binding info here
virajjasani
force-pushed
the
HADOOP-18399-trunk
branch
2 times, most recently
from
1dc0b6e to
e5ca459
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Addressed the above comments. |
|
@steveloughran could you please take another look? |
|
Re-tested the full test suit with and without prefetch against us-west-2, test results look good. |
|
FYI @mehakmeet @mukund-thakur if you have some bandwidth to review this PR |
|
Sorry for the late reply. We are really busy with other important stuff like data correctness issue in Azure connector and many others internally. So I can't prioritize this as prefetching is not an active feature. |
|
We'd love to take a look but this week is pretty busy for our team. I should have more time next week, will see if I can do earlier but I want to be realistic :') |
|
No worries, thank you! |
steveloughran
requested changes
Nov 25, 2022
|
|
||
| this.ops = new BlockOperations(); | ||
| this.ops.setDebug(false); | ||
| this.conf = conf; |
There was a problem hiding this comment.
add requireNonNull() checks here
There was a problem hiding this comment.
After applying this change, a minor tests that intercepts (asserts) some arg based Exceptions, failed.
Fixed in the latest revision.
| createObjectAttributes(path, fileStatus), | ||
| createInputStreamCallbacks(auditSpan), | ||
| inputStreamStats)); | ||
| inputStreamStats, getConf())); |
There was a problem hiding this comment.
put on a new line for consistency
|
|
||
| @Override | ||
| protected Path getCacheFilePath() throws IOException { | ||
| protected Path getCacheFilePath(Configuration conf, LocalDirAllocator localDirAllocator) |
There was a problem hiding this comment.
think this should be indented more
|
|
||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(ITestS3APrefetchingInputStream.class); | ||
| private static final Logger LOG = LoggerFactory.getLogger(ITestS3APrefetchingInputStream.class); |
There was a problem hiding this comment.
not needed; just revert to make patch smaller. unless you really, really want this
| } | ||
|
|
||
| @Test | ||
| public void testCacheFileExistence() throws Throwable { |
There was a problem hiding this comment.
is there any cleanup of these files after? i know a maven clean will do it, but these may be be big files and create problems on docker container runs. best to do an rm of the dir, or at least the files, afterwards
There was a problem hiding this comment.
Sounds reasonable to let the test clean it up rather than rely on mvn cleanup. Done.
There was a problem hiding this comment.
Sorry, I had to remove this change in the latest revision.
The problem could arise with the file cleanup: we would not know which exact cache file was created by this test, meaning what if another test (with -Dprefetch option) running in parallel also creates a new cache file and we remove it as part of this test?
Edit: This is now taken care of in the latest revision.
There was a problem hiding this comment.
Update: this is now taken care of with the latest commit. The test has been included with sequential-integration-tests.
virajjasani
force-pushed
the
HADOOP-18399-trunk
branch
2 times, most recently
from
0922a44 to
399ad71
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HADOOP-18399-trunk
branch
3 times, most recently
from
ee06d3f to
c9331a2
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
@mehakmeet I have addressed all comments. Could you please take a look? |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
mvninstall and shadedclient errors above are related to this latest commit 72b0122 |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Apr 14, 2023
There was a problem hiding this comment.
commented. main changes
- somehow get that allocator from s3afs all the way down
- tests to properly isolate temp dirs used including cleanup; all test suites which use
prepareTestConfiguration()have this already, so: use its defined buffer dir, and call the method in new unit tests
|
|
||
| private final PrefetchingStatistics prefetchingStatistics; | ||
|
|
||
| // File attributes attached to any intermediate temporary file created during index creation. |
.../hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/SingleFilePerBlockCache.java
Show resolved
Hide resolved
.../hadoop-common/src/main/java/org/apache/hadoop/fs/impl/prefetch/SingleFilePerBlockCache.java
Show resolved
Hide resolved
| LOG.debug("Creating in caching input stream for {}", context.getPath()); | ||
| final String bufferDir = | ||
| conf.get(BUFFER_DIR) != null ? BUFFER_DIR : HADOOP_TMP_DIR; | ||
| final LocalDirAllocator localDirAllocator = |
There was a problem hiding this comment.
these are pretty expensive. s3afs creates one in directoryAllocator on demand...that code could be pulled out from createTmpFileForWrite() and also passed down here.
There was a problem hiding this comment.
i think i missed this one in the first place, should have passed down S3AFS dir allocator only.
Done.
|
|
||
| testFile = new Path(testFileUri); | ||
| prefetchBlockSize = conf.getInt(PREFETCH_BLOCK_SIZE_KEY, PREFETCH_BLOCK_DEFAULT_SIZE); | ||
| fs = new S3AFileSystem(); |
There was a problem hiding this comment.
superclass setup creates an fs already; no need for another one.
| @Override | ||
| public synchronized void teardown() throws Exception { | ||
| super.teardown(); | ||
| File tmpFileDir = new File("target/build/test"); |
There was a problem hiding this comment.
ok, here is why the tests are stamping on each other.
if you look at S3ATestUtils.prepareTestConfiguration() you can see how it patches test configs with custom temp dirs. extract that from the fs used and delete it only
| private static final class BlockManagerForTesting | ||
| extends S3ACachingBlockManager { | ||
|
|
||
| private static final Configuration CONF = new Configuration(); |
There was a problem hiding this comment.
use prepareTestConfiguration() to isolate the temp dir. this always sets BUFFER_DIR so no need for the checks on L142
| S3AInputStreamStatistics stats = | ||
| readContext.getS3AStatisticsContext().newInputStreamStatistics(); | ||
|
|
||
| Configuration conf = new Configuration(); |
There was a problem hiding this comment.
same comments as above on prepareTestConfiguration()
|
|
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
4 tasks
|
Re-run against us-west-2: |
virajjasani
commented
Apr 17, 2023
| assertEquals(0, cache.size()); | ||
| assertFalse(cache.containsBlock(0)); | ||
| cache.put(0, buffer1); | ||
| cache.put(0, buffer1, CONF, new LocalDirAllocator(HADOOP_TMP_DIR)); |
There was a problem hiding this comment.
For Test* classes, using BUFFER_DIR is not helpful as they don't use S3ATestUtils#prepareTestConfiguration.
Hence, using HADOOP_TMP_DIR for Test* classes.
steveloughran
changed the title
asfgit
pushed a commit
that referenced
this pull request
Apr 26, 2023
…llocator (#5054) Contributed by Viraj Jasani
asfgit
pushed a commit
that referenced
this pull request
Apr 28, 2023
…llocator (#5054) Contributed by Viraj Jasani
ferdelyi
pushed a commit
to ferdelyi/hadoop
that referenced
this pull request
May 26, 2023
…llocator (apache#5054) Contributed by Viraj Jasani
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
No description provided.