HADOOP-19242. Aliyun oss add a redirection switch #6967
Conversation
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Thanks for the PR.
PTAL my comments.
@steveloughran Kindly ping. Could you also help to take a look ? Thanks.
| } | ||
| } | ||
|
|
||
| private void constructOssClient(Configuration conf) throws IOException { |
There was a problem hiding this comment.
Could we avoid repeating these codes?
There was a problem hiding this comment.
such as is done in AliyunOSSTestUtils
There was a problem hiding this comment.
OK，I will simplify the code.
| @@ -147,6 +147,13 @@ public void initialize(URI uri, Configuration conf, String user, | |||
| throw new IllegalArgumentException(msg); | |||
| } | |||
|
|
|||
| boolean redirectEnable = conf.getBoolean(REDIRECT_ENABLE_KEY, | |||
| REDIRECT_ENABLE_DEFAULT); | |||
| if (!redirectEnable) { | |||
There was a problem hiding this comment.
Could we just use clientConf.setRedirectEnable(redirectEnable); ?
Current implementation always assumes true is default for clientConf, right ?
There was a problem hiding this comment.
1 I will simplify the code to clientConf.setRedirectEnable(redirectEnable)
2 yes,the OSS SDK defaults the redirect setting to true
| REDIRECT_ENABLE_DEFAULT); | ||
| if (!redirectEnable) { | ||
| clientConf.setRedirectEnable(false); | ||
| LOG.info("oss redirectEnable closed"); |
There was a problem hiding this comment.
I will change the log level to debug in the next commit
hadoop-tools/hadoop-aliyun/src/main/java/org/apache/hadoop/fs/aliyun/oss/Constants.java
Show resolved
Hide resolved
|
|
||
| package org.apache.hadoop.fs.aliyun.oss; | ||
|
|
||
| import org.apache.commons.lang3.StringUtils; |
There was a problem hiding this comment.
nit: import ordering, especially keeping apache stuff on their own
There was a problem hiding this comment.
nit: import ordering, especially keeping apache stuff on their own
OK. I will optimize the import ordering
| } | ||
| } | ||
|
|
||
| private void constructOssClient(Configuration conf) throws IOException { |
There was a problem hiding this comment.
such as is done in AliyunOSSTestUtils
...tools/hadoop-aliyun/src/test/java/org/apache/hadoop/fs/aliyun/oss/TestAliyunOSSRedirect.java
Show resolved
Hide resolved
Currently, none of the OSS test cases have the ITest prefix. I'm not sure about the historical reasons, and I may need to discuss this with other colleagues on the Aliyun OSS team. Can this PR be considered separately from the changes to add the ITest prefix? |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran @masteryhx @zeekling hi，I have made the suggested changes to the code. Please take a look again. Thanks. |
|
This patch looks good. There are still a few checkstyle issues that haven't been fully resolved. Can these problems be further addressed? @zhouaoe |
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.fs.FSDataInputStream; | ||
| import com.aliyun.oss.common.auth.CredentialsProvider; |
There was a problem hiding this comment.
nit: not an apache import
There was a problem hiding this comment.
ok went through it, main area of change is in the tests where i
- reviewed object close() code everywhere
- suggested new parent class
- proposed move to assertj for meaningful failures
- more
the problem with asserTrue() is that when it is false, there is no meaning at all. AssertJ is really, really good at providing details on a failure. It's a bit hard to learn, but now is the time
| * When the redirection feature is available, it will access the | ||
| * redirected target file | ||
| */ | ||
| public class TestAliyunOSSRedirect { |
There was a problem hiding this comment.
I'm going to to suggest subclassing AbstractHadoopTestBase
This
- adds the test timeout, and supports a system property to extend it
- sets the thread name to the test method, different names for setup and teardown. this is really useful
Once you do that you can cut line 81; nothing else should change
There was a problem hiding this comment.
Thank you, steveloughran, for the review. I will carefully revise these comments and submit the improved code as soon as possible.
| } | ||
|
|
||
| testURI = URI.create(fsname); | ||
| assertTrue(testURI.getScheme().equals(Constants.FS_OSS)); |
There was a problem hiding this comment.
AssertJ assertions here please
Assertions.assertThat(testURI.getScheme())
.isEqualsTo(Constants.FS_OSS));
| } | ||
| String proxyUsername = conf.getTrimmed(PROXY_USERNAME_KEY); | ||
| String proxyPassword = conf.getTrimmed(PROXY_PASSWORD_KEY); | ||
| if ((proxyUsername == null) != (proxyPassword == null)) { |
There was a problem hiding this comment.
you can use org.apache.hadoop.util.Preconditions.checkArgument() with an xor statement
Preconditions.checkArgument(!(proxyUsername == null ^ proxyPassword == null),
"Proxy error: " + PROXY_USERNAME_KEY + " or " +
PROXY_PASSWORD_KEY + " set without the other.")
This is about the only place the ^ can be used in normal code, so something to consider just to tick it off your software developer career!
| } | ||
|
|
||
| String endPoint = conf.getTrimmed(ENDPOINT_KEY, ""); | ||
| if (StringUtils.isEmpty(endPoint)) { |
There was a problem hiding this comment.
Use Preconditions.checkArgument
| fs = AliyunOSSTestUtils.createTestFileSystem(conf); | ||
|
|
||
| Path srcFilePath = new Path(testRootPath, "redirect_test_src.txt"); | ||
| FSDataInputStream instream = this.fs.open(srcFilePath); |
There was a problem hiding this comment.
use try with resources
| fs = AliyunOSSTestUtils.createTestFileSystem(conf); | ||
|
|
||
| Path srcFilePath = new Path(testRootPath,"redirect_test_src.txt"); | ||
| assertThrows(IOException.class, () -> this.fs.open(srcFilePath)); |
There was a problem hiding this comment.
so if this doesn't throw, there's actually a new open stream. better to use our LambdaTestUtils.intercept() which is often more informative on failures, and close the stream.
intercept(IOException.class, () ->
fs.open(srcFilePath).close());
| String fsname = conf.getTrimmed( | ||
| TestAliyunOSSFileSystemContract.TEST_FS_OSS_NAME, ""); | ||
|
|
||
| if (StringUtils.isEmpty(fsname)) { |
There was a problem hiding this comment.
assertJ assume
Assumptions.assumeThat(fsName)
.describedAs(No test filesystem in "
+ TestAliyunOSSFileSystemContract.TEST_FS_OSS_NAME)
.isNotEmpty();
| @After | ||
| public void tearDown() throws Exception { | ||
| if (fs != null) { | ||
| fs.delete(new Path(testRootPath), true); |
There was a problem hiding this comment.
add fs.close(), so the filesystems aren't leaked
| boolean redirectEnable = conf.getBoolean(REDIRECT_ENABLE_KEY, | ||
| REDIRECT_ENABLE_DEFAULT); | ||
| clientConf.setRedirectEnable(redirectEnable); | ||
| LOG.debug("oss redirectEnable " + redirectEnable); |
There was a problem hiding this comment.
prefer using {} expansion sin the log
LOG.debug("oss redirectEnable {}", redirectEnable);
- saves building the string when debug is not enabled
- the toString() call on every argument is very robust, handles raised exceptions and more. Not an issue for boolean, but stull a good habit
Description of PR
How was this patch tested?
New unit tests; cloud store tests
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?