-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-17871. S3A CSE: minor tuning #3412
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
Conversation
steveloughran
left a comment
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.
production code looks OK, apart from one query.
I'm worried about complications in testing.
I'm likely to have a test setup with KMS encryption,using the old values
<property>
<name>fs.s3a.bucket.hwdev-steve-london.server-side-encryption-algorithm</name>
<value>SSE-KMS</value>
</property>
<property>
<name>fs.s3a.bucket.stevel-london.server-side-encryption.key</name>
<value>${london.kms.key}</value>
</property>
I was thinking about how best I could retain the settings and still
test consistently across different versions.
I think I'm going to have to
- Set both sets of options, explicitly
- Comment both sets out when I want to disable it
What would be good is for all test cases which call removeBaseAndBucketOverrides() to remove encryption options to still remove the old options. This will stop any problems surfacing where the old option is still so the FS instance will be encrypted.
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/S3ATestUtils.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSSEConfiguration.java
Show resolved
Hide resolved
...rc/test/java/org/apache/hadoop/fs/s3a/auth/delegation/ITestSessionDelegationInFileystem.java
Show resolved
Hide resolved
...rc/test/java/org/apache/hadoop/fs/s3a/auth/delegation/ITestSessionDelegationInFileystem.java
Show resolved
Hide resolved
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/ITestS3AHugeFilesSSECDiskBlocks.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
Outdated
Show resolved
Hide resolved
...op-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AClientSideEncryption.java
Outdated
Show resolved
Hide resolved
steveloughran
left a comment
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.
all good except for some minor test tuning
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java
Outdated
Show resolved
Hide resolved
| fileSystem.initialize(fsURI, conf); | ||
| throw new Exception("Do not reach here"); | ||
| }); | ||
| Configuration conf = super.createConfiguration(); |
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; if we generate that error "unknown server side algorithm", should we change to "unknown s3 encryption algorithm"
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.
Seems like we are already returning "Unknown encryption algorithm " exception string in case of an unknown encryption algorithm. This test was just not running and hence, never failed.
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java
Outdated
Show resolved
Hide resolved
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java
Outdated
Show resolved
Hide resolved
...hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionAlgorithmValidation.java
Outdated
Show resolved
Hide resolved
|
@steveloughran, Just realized something in the test |
|
it looks like the test has never run. That is, it went in broken and has stayed that way. I don't see why we need to keep maintaining disabled tests. Either we turn it on (more grief) or we just remove entirely. For now: I'll go with your PR as is, so we can backport it for 3.3.2; but really we can/should delete the entire test suite. Funny: we'd never noticed until now. |
|
Yes, it's been like that since the test was created. Okay, let's maybe merge this in after yetus, and I can cherry pick into 3.3 with the rest of the commits. |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran can we ignore javac errors, which are regarding old keys being used in removeBucketOverides since they are deprecated now? some of the javac errors are not from this patch also. Checkstyle errors are in that disabled test. Guess, the indentation is still wrong somehow. |
|
In the last Yetus run, checkstyle says the correct indentation is level 6 not 8 and in this one, it says it should be 8, 10, or 12 and not 6. |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
left a comment
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.
+1 for the last patch; will wait for yetus to approve as the final due diligence
|
🎊 +1 overall
This message was automatically generated. |
This migrates the fs.s3a-server-side encryption configuration options to a name which covers client-side encryption too. fs.s3a.server-side-encryption-algorithm becomes fs.s3a.encryption.algorithm fs.s3a.server-side-encryption.key becomes fs.s3a.encryption.key The existing keys remain valid, simply deprecated and remapped to the new values. If you want server-side encryption options to be picked up regardless of hadoop versions, use the old keys. (the old key also works for CSE, though as no version of Hadoop with CSE support has shipped without this remapping, it's less relevant) Contributed by: Mehakmeet Singh
This migrates the fs.s3a-server-side encryption configuration options to a name which covers client-side encryption too. fs.s3a.server-side-encryption-algorithm becomes fs.s3a.encryption.algorithm fs.s3a.server-side-encryption.key becomes fs.s3a.encryption.key The existing keys remain valid, simply deprecated and remapped to the new values. If you want server-side encryption options to be picked up regardless of hadoop versions, use the old keys. (the old key also works for CSE, though as no version of Hadoop with CSE support has shipped without this remapping, it's less relevant) Contributed by: Mehakmeet Singh Change-Id: I51804b21b287dbce18864f0a6ad17126aba2b281
This migrates the fs.s3a-server-side encryption configuration options to a name which covers client-side encryption too. fs.s3a.server-side-encryption-algorithm becomes fs.s3a.encryption.algorithm fs.s3a.server-side-encryption.key becomes fs.s3a.encryption.key The existing keys remain valid, simply deprecated and remapped to the new values. If you want server-side encryption options to be picked up regardless of hadoop versions, use the old keys. (the old key also works for CSE, though as no version of Hadoop with CSE support has shipped without this remapping, it's less relevant) Contributed by: Mehakmeet Singh
Region: ap-south-1
mvn clean verify -Dparallel-tests -DtestsThreadCount=4 -DscaleUT:
IT:
IT-scale:
CSE-test: