HADOOP-18889. S3A v2 SDK third party support #6141
Merged
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
|
💔 -1 overall
This message was automatically generated. |
1. handle getBucketLocation() unsupported 2. append / to marker tool searches if the first call fails. 3. downgrade change control errors a bit better. TODO: useful error text/docs 4. handle unknown storage classes. 5. Docs on how to bind to and test third party stores, including google gcs. Not doing the region logic; this is other test failures. Exception translation/resilience * new AWSUnsupportedFeatureException ex for unsupported/unavailable errors * handle 501 method unimplemented the same * all ex > 500 mapped to the AWSStatus500Exception * precondition errors handled a bit better * GCS throttle exception also recognised. * New class ErrorHandling to move our error handling logic too, initially somewhere for new stuff * ErrorHandling.maybeTranslateNetworkException() scans down an exception chain and if the inner one is of a known type - UnknownHostException - NoRouteToHostException - ConnectException then a new exception of that specific type is created, with the top level ex its cause. This is done to retain the whole stack chain. * reduce the number of retries within the AWS SDK * and those of our normal set. The reduction in retries is because its clear when you try to create a bucket which doesn't resolve that the time for even an UnknownHostException to eventually fail over 90s, which then hit the s3a retry code. - Reducing the sdk retries means these escalate to our code better. - Cutting back on our own retries makes it a bit more responsive for most real deployments. - maybeTranslateNetworkException() and s3a retry policy means that unknown host exception is recognised and fails fast. Lots of changes to the test code: * more switches to turn off test suites (acls, v1list, ...) * more resilience to different test failures * pull up skip() checks into config create phase, rather than waiting for fs to be instantiated (faster; keeps the fs cache clean) * tuning for new exceptions Change-Id: I2c30dc41971a4d303376f53871b514156f68012e
ahmarsuhail
requested changes
Oct 3, 2023
| this.invoker = storeContext.getInvoker(); | ||
| this.auditSpan = storeContext.getActiveAuditSpan(); | ||
| String p; | ||
| if (prefix == null) { |
There was a problem hiding this comment.
would prefer to move this logic out of the constructor. We do something similar in S3AFileSystem.listMultipartUploads. Maybe move to a method in S3AUtils and use that in both places.
Do you know why we were doing the prefix logic in S3AFileSystem.listMultipartUploads and not here?
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Show resolved
Hide resolved
| @@ -2888,6 +2898,7 @@ protected S3ListResult listObjects(S3ListRequest request, | |||
| trackDurationOfOperation(trackerFactory, | |||
| OBJECT_LIST_REQUEST, | |||
| () -> { | |||
| checkNotClosed(); // this listing is done in the thread pool, it may actually be closed | |||
There was a problem hiding this comment.
is the concern here that since the listing is done async, by the time it gets here, S3AFS could have been closed? If yes, we could make the comment clearer:
// this listing is done in the thread pool, filesystem could be closed
also nit: move the comment up a line, above the checkNotClosed()
There was a problem hiding this comment.
exactly: i managed to create a stack trace here when closing an fs client spinning on UnknownHost
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ARetryPolicy.java
Show resolved
Hide resolved
| import java.net.NoRouteToHostException; | ||
| import java.net.UnknownHostException; | ||
|
|
||
| /** |
There was a problem hiding this comment.
is the goal to eventually move all translation logic here?
There was a problem hiding this comment.
yes...a big chunk of S3AUtils is now just that mapping, fault extraction etc and isolation is good
There was a problem hiding this comment.
update: already had a class ErrorTranslation. thats's where things are going
|
|
||
| # Working with Third-party S3 Stores | ||
|
|
||
| The S3A connector works well worth a third-party S3 stores if the following requirements are met: |
There was a problem hiding this comment.
nit: typo, works well with
|
|
||
| The S3A connector works well worth a third-party S3 stores if the following requirements are met: | ||
|
|
||
| * It correctly implements the core S3 REST API, including support for uploads clothes and the V2 listing API. |
There was a problem hiding this comment.
typo on this line, remove clothes ?
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/AWSStatus500Exception.java
Show resolved
Hide resolved
| .cause(cause) | ||
| .build(); | ||
| } | ||
|
|
There was a problem hiding this comment.
can also add a similar test for ConnectException
steveloughran
force-pushed
the
s3/HADOOP-18889-third-party
branch
from
0edc728 to
5042f66
Compare
Change-Id: If0d14307d623d90b9c8d827852ffd6c3ea9137e4
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
- GCS raises 404 on a delete of a file which doesn't exist: fix that - removed that checkNotClosed() call in listings, as it breaks directory delete on exit. - docs updated with full set of options to talk to google gcs. Test failures * fix tests which got their stats wrong if single delete was used over batch deletes * content encoding tests can be disabled * trying to improve ITestCustomSigner but it still fails with GCS. * rejection of getDefaultEncoding() * handle region/endpoint test which only work with aws creds/regions/... Change-Id: I31a5964635e4a8675ec18f2e97139be8bb75d90e
|
new commit now works properly with GCS except for some known test failures! Docs cover the details...the key thing is s3a can handle it if you don't have the gcs-connector. No idea about performance diffs. I know gcs does some things with caching parquet footers etc, but s3a offers vector io. also i've somehow got multipart playing up. will look at. |
|
Tested: google cloud somewhere |
|
💔 -1 overall
This message was automatically generated. |
Error translation uses reflection to create IOE of the right type. All IOEs at the bottom of an AWS stack chain are regenerated. Troubleshooting doc updated with the new classnames; cutting out stack traces rather than bother recreating them. Pulled the multipart param stuff, which can be addressed in a separate JIRA. Where I also think we should move listUploads() to S3AInternals as it is very internal. But did tweak ITestS3AMultipartUtils while debugging this... leaving that in as better assertions are always good. + checkstyles Change-Id: Iba325cc98a95d11911fe41546f91a912a1d9aca5
|
latest iteration tested: s3 london at -Dscale and -Dprefetch *Error translation uses reflection to create IOE of the right type.
Pulled the multipart param stuff, which can be addressed in a separate JIRA. |
|
💔 -1 overall
This message was automatically generated. |
ahmarsuhail
approved these changes
Oct 9, 2023
| @@ -69,8 +72,14 @@ public void testNoBucketProbing() throws Exception { | |||
| assertTrue("getFileStatus on root should always return a directory", | |||
| fs.getFileStatus(root).isDirectory()); | |||
|
|
|||
| expectUnknownStore( | |||
| () -> fs.listStatus(root)); | |||
| try { | |||
There was a problem hiding this comment.
I didn't understand why we're doing this..why will this listStatus call fail with a third party store?
There was a problem hiding this comment.
the region lookup talks to eu-west to get th region of the bucket, and if the credentials aren't valid then the request is rejected with AccessDenied.
| s3client.deleteObject(b -> b.bucket(bucket).key(key)); | ||
| return "deleted " + key; | ||
| }); | ||
| } catch (IOException ignored) { |
There was a problem hiding this comment.
why are we swallowing exceptions here?
There was a problem hiding this comment.
google gcs raises 404 if the object doesn't exist.
There was a problem hiding this comment.
note we didn't see this running against the v1 sdk, but that may be test coverage rather than sdk changes
* Explicitly declare SocketException as connectivity failure * Log at debug whenever retry policies looked up * Reorder exceptions to alphabetical order, with commentary * Review use of the Invoke.retry() method. * Convert timeout config options to millis in constructing s3 clients... that didn't help with connection setup issues against the stores. All invocations are currently declared as idempotent, so there's no need to differentiate between the idempotent/non-idempotent calls. However, things may change in future so having different policies makes sense. When S3Guard was used we probably did have some considered non idempotent. Change-Id: I4f4898347e29d4bb0040e7835a2d635543d39d1f
|
failure of test run with -Dparallel-tests -DtestsThreadCount=10 -Dscale normally I run with count=8; wonder if this is a factor. but the huge stuff is done at the end, sequentially. or its a leftover of me cancelling a previous run. in which case i should add a new cleanup |
This was needed to fix ITestS3AHugeMagicCommits once a test had been interrupted...after which it kept getting worse. +fix a typo in Statistic string description. cleanup is a bit ugly. we really need an interface/impl for multipart operations within the fs. Change-Id: I9a639b10752ad266c5bfb08efdc0b4c0930da608
|
fixed the test cleanup; one failure is |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
the javadoc warning is (a) mistaken (b) complaining about an existing entry Change-Id: Ifbbf486cc58d5f896fa8f4a5a38889351bd8fcbe
|
💔 -1 overall
This message was automatically generated. |
...i understand now. @see must come last in the javadocs Change-Id: If7464a7d13bbd29fd5a000b2068a2aa536df2d66
|
once javadocs is happy i will merge this |
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: I9353a3bd7e8a8044b90f091ea923e5ae53a90de9
|
🎊 +1 overall
This message was automatically generated. |
|
going to cut out the bits related to connection settings; needs its own review. specifically, i want the settings <property>
<name>fs.s3a.connection.timeout</name>
<value>25000</value>
</property>
<property>
<name>fs.s3a.connection.establish.timeout</name>
<value>5000</value>
</property>
to map to millis, and i want tests to show this |
steveloughran
force-pushed
the
s3/HADOOP-18889-third-party
branch
from
13fbaf8 to
97992b0
Compare
|
rolled back one commit; it's been through yetus so will merge |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
This was referenced Oct 13, 2023
ahmarsuhail
pushed a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Nov 27, 2023
Tune AWS v2 SDK changes based on testing with third party stores including GCS. Contains HADOOP-18889. S3A v2 SDK error translations and troubleshooting docs * Changes needed to work with multiple third party stores * New third_party_stores document on how to bind to and test third party stores, including google gcs (which works!) * Troubleshooting docs mostly updated for v2 SDK Exception translation/resilience * New AWSUnsupportedFeatureException for unsupported/unavailable errors * Handle 501 method unimplemented as one of these * Error codes > 500 mapped to the AWSStatus500Exception if no explicit handler. * Precondition errors handled a bit better * GCS throttle exception also recognized. * GCS raises 404 on a delete of a file which doesn't exist: swallow it. * Error translation uses reflection to create IOE of the right type. All IOEs at the bottom of an AWS stack chain are regenerated. then a new exception of that specific type is created, with the top level ex its cause. This is done to retain the whole stack chain. * Reduce the number of retries within the AWS SDK * And those of s3a code. * S3ARetryPolicy explicitly declare SocketException as connectivity failure but subclasses BindException * SocketTimeoutException also considered connectivity * Log at debug whenever retry policies looked up * Reorder exceptions to alphabetical order, with commentary * Review use of the Invoke.retry() method The reduction in retries is because its clear when you try to create a bucket which doesn't resolve that the time for even an UnknownHostException to eventually fail over 90s, which then hit the s3a retry code. - Reducing the SDK retries means these escalate to our code better. - Cutting back on our own retries makes it a bit more responsive for most real deployments. - maybeTranslateNetworkException() and s3a retry policy means that unknown host exception is recognised and fails fast. Contributed by Steve Loughran
ahmarsuhail
pushed a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Dec 5, 2023
Tune AWS v2 SDK changes based on testing with third party stores including GCS. Contains HADOOP-18889. S3A v2 SDK error translations and troubleshooting docs * Changes needed to work with multiple third party stores * New third_party_stores document on how to bind to and test third party stores, including google gcs (which works!) * Troubleshooting docs mostly updated for v2 SDK Exception translation/resilience * New AWSUnsupportedFeatureException for unsupported/unavailable errors * Handle 501 method unimplemented as one of these * Error codes > 500 mapped to the AWSStatus500Exception if no explicit handler. * Precondition errors handled a bit better * GCS throttle exception also recognized. * GCS raises 404 on a delete of a file which doesn't exist: swallow it. * Error translation uses reflection to create IOE of the right type. All IOEs at the bottom of an AWS stack chain are regenerated. then a new exception of that specific type is created, with the top level ex its cause. This is done to retain the whole stack chain. * Reduce the number of retries within the AWS SDK * And those of s3a code. * S3ARetryPolicy explicitly declare SocketException as connectivity failure but subclasses BindException * SocketTimeoutException also considered connectivity * Log at debug whenever retry policies looked up * Reorder exceptions to alphabetical order, with commentary * Review use of the Invoke.retry() method The reduction in retries is because its clear when you try to create a bucket which doesn't resolve that the time for even an UnknownHostException to eventually fail over 90s, which then hit the s3a retry code. - Reducing the SDK retries means these escalate to our code better. - Cutting back on our own retries makes it a bit more responsive for most real deployments. - maybeTranslateNetworkException() and s3a retry policy means that unknown host exception is recognised and fails fast. Contributed by Steve Loughran
ahmarsuhail
pushed a commit
to ahmarsuhail/hadoop
that referenced
this pull request
Dec 5, 2023
Tune AWS v2 SDK changes based on testing with third party stores including GCS. Contains HADOOP-18889. S3A v2 SDK error translations and troubleshooting docs * Changes needed to work with multiple third party stores * New third_party_stores document on how to bind to and test third party stores, including google gcs (which works!) * Troubleshooting docs mostly updated for v2 SDK Exception translation/resilience * New AWSUnsupportedFeatureException for unsupported/unavailable errors * Handle 501 method unimplemented as one of these * Error codes > 500 mapped to the AWSStatus500Exception if no explicit handler. * Precondition errors handled a bit better * GCS throttle exception also recognized. * GCS raises 404 on a delete of a file which doesn't exist: swallow it. * Error translation uses reflection to create IOE of the right type. All IOEs at the bottom of an AWS stack chain are regenerated. then a new exception of that specific type is created, with the top level ex its cause. This is done to retain the whole stack chain. * Reduce the number of retries within the AWS SDK * And those of s3a code. * S3ARetryPolicy explicitly declare SocketException as connectivity failure but subclasses BindException * SocketTimeoutException also considered connectivity * Log at debug whenever retry policies looked up * Reorder exceptions to alphabetical order, with commentary * Review use of the Invoke.retry() method The reduction in retries is because its clear when you try to create a bucket which doesn't resolve that the time for even an UnknownHostException to eventually fail over 90s, which then hit the s3a retry code. - Reducing the SDK retries means these escalate to our code better. - Cutting back on our own retries makes it a bit more responsive for most real deployments. - maybeTranslateNetworkException() and s3a retry policy means that unknown host exception is recognised and fails fast. Contributed by Steve Loughran
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
Tune AWS v2 SDK changes based on testing with third party stores including GCS. Contains HADOOP-18889. S3A v2 SDK error translations and troubleshooting docs * Changes needed to work with multiple third party stores * New third_party_stores document on how to bind to and test third party stores, including google gcs (which works!) * Troubleshooting docs mostly updated for v2 SDK Exception translation/resilience * New AWSUnsupportedFeatureException for unsupported/unavailable errors * Handle 501 method unimplemented as one of these * Error codes > 500 mapped to the AWSStatus500Exception if no explicit handler. * Precondition errors handled a bit better * GCS throttle exception also recognized. * GCS raises 404 on a delete of a file which doesn't exist: swallow it. * Error translation uses reflection to create IOE of the right type. All IOEs at the bottom of an AWS stack chain are regenerated. then a new exception of that specific type is created, with the top level ex its cause. This is done to retain the whole stack chain. * Reduce the number of retries within the AWS SDK * And those of s3a code. * S3ARetryPolicy explicitly declare SocketException as connectivity failure but subclasses BindException * SocketTimeoutException also considered connectivity * Log at debug whenever retry policies looked up * Reorder exceptions to alphabetical order, with commentary * Review use of the Invoke.retry() method The reduction in retries is because its clear when you try to create a bucket which doesn't resolve that the time for even an UnknownHostException to eventually fail over 90s, which then hit the s3a retry code. - Reducing the SDK retries means these escalate to our code better. - Cutting back on our own retries makes it a bit more responsive for most real deployments. - maybeTranslateNetworkException() and s3a retry policy means that unknown host exception is recognised and fails fast. Contributed by Steve Loughran
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Not doing the region logic; this is other test failures.
Exception translation/resilience
Then a new exception of that specific type is created, with the top level ex its cause.
This is done to retain the whole stack chain.
The reduction in retries is because its clear when you try to create a bucket
which doesn't resolve that the time for even an UnknownHostException to
eventually fail over 90s, which then hit the s3a retry code.
UnknownHostException is recognised and fails fast.
Lots of changes to the test code:
How was this patch tested?
UnknownHostException needed to be radically improved.
Gave me the stack traces for the docs too
Docs cover google gcs binding; I've not tested that yet, but the settings came from
a colleague who has connected the v1 sdk to it.
A full test run against GCS should be something we require here as it supports
a different subset of the s3 apis (no bulk delete, v1 list only...)
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?