HADOOP-18706: S3ABlockOutputStream recovery, and downgrade syncable will call flush rather than no-op. #5771
Conversation
… the s3a recovery changes
unit tests
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran Can you take a look at this new PR when you get a chance? |
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
commented; most on the tests. sorry...but I spend more time waiting for tests or debugging failures than anything else, so I'm very fussy there.
| * Buffer blocks to disk. | ||
| */ | ||
| static class DiskBlockFactory extends BlockFactory { | ||
| private static final String ESCAPED_FORWARD_SLASH = "EFS"; |
There was a problem hiding this comment.
think I might prefer something more distinguishable from text, e.g "FS" and "BS" so its easier to read in a dir listing
| { | ||
| // avoid validating multiple times. | ||
| // if the jvm running is version 9+ then defer to java.io.File validation implementation | ||
| if(Float.parseFloat(System.getProperty("java.class.version")) >= 53) { |
There was a problem hiding this comment.
this should go in org.apache.hadoop.util.Shell; there's already something similar. will need a test somehow.
| int maxRandomSuffixLen = 19; // Long.toUnsignedString(Long.MAX_VALUE).length() | ||
|
|
||
| String name; | ||
| int nameMax = 255; // unable to access the underlying FS directly, so assume 255 |
There was a problem hiding this comment.
make a constant, e.g ASSUMED_MAX_FILENAME
| int nameMax = 255; // unable to access the underlying FS directly, so assume 255 | ||
| int excess = prefixLength + maxRandomSuffixLen + suffixLength - nameMax; | ||
|
|
||
| // shorten the prefix length if the file name exceeds 255 chars |
There was a problem hiding this comment.
and replace explicit size with "too long"
| // shorten the prefix length if the file name exceeds 255 chars | ||
| if (excess > 0) { | ||
| // Attempt to shorten the prefix length to no less than 3 | ||
| prefixLength = shortenSubName(prefixLength, excess, 3); |
There was a problem hiding this comment.
again, make a constant and use in both places
| mock(S3ADataBlocks.BlockFactory.class); | ||
| long blockSize = Constants.DEFAULT_MULTIPART_SIZE; | ||
| WriteOperationHelper oHelper = mock(WriteOperationHelper.class); | ||
| AuditSpan auditSpan = mock(AuditSpan.class); |
There was a problem hiding this comment.
you can just use org.apache.hadoop.fs.s3a.audit.impl.NoopSpan here; one less thing to mock.
| import org.junit.Test; | ||
| import org.junit.rules.Timeout; | ||
|
|
||
| import java.io.File; |
There was a problem hiding this comment.
nit: ordering...these should come first.
|
|
||
| @Before | ||
| public void init() throws IOException { | ||
| Files.createDirectories(TEMP_DIR.toPath()); |
There was a problem hiding this comment.
use S3ATestUtils code here for handling of parallel test cases and ease of future maintenance
Configuration conf = new Configuration(false)
S3ATestUtils.prepareTestConfiguration(conf)
conf.get(BUFFER_DIR)
| public void testSafeCreateTempFile() throws Throwable { | ||
| // fitting name isn't changed | ||
| File noChangesRequired = S3AFileSystem.safeCreateTempFile("noChangesRequired", ".tmp", TEMP_DIR); | ||
| assertTrue(noChangesRequired.exists()); |
There was a problem hiding this comment.
bad news, use AssertJ. more verbose but the assertions it raised include details about the failure. which is what I insist on, sorry. If a yetus build fails, i want more than a line number. if this isn't done automatically, used .describedAs() which takes String.format() string + varargs list
| assertTrue(noChangesRequiredName.endsWith(".tmp")); | ||
|
|
||
| // a long prefix should be truncated | ||
| File excessivelyLongPrefix = S3AFileSystem.safeCreateTempFile(longStr, ".tmp", TEMP_DIR); |
There was a problem hiding this comment.
each of these should be split into their own test method so they can fail independently. I know, we ignore that in ITests, but that is because they're integration tests with longer setup overhead
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
3 participants
Description of PR
This PR improves the ability to recovery partial S3A uploads.
Changed the handleSyncableInvocation() to call flush() after warning that the syncable API isn't supported. This mirrors the downgradeSyncable behavior of BufferedIOStatisticsOutputStream and RawLocalFileSystem.
Changed the DiskBlock temporary file names to include the S3 key to allow partial uploads to be recovered.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?