You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/12/08 10:51:04 UTC
[GitHub] [iceberg] openinx opened a new pull request #3687: Aliyun: Add OSS integration test rule
openinx opened a new pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687
This PR addressed all the commented issues from the original one: https://github.com/apache/iceberg/pull/3596
I've tested the integration tests successfully in my local:
```bash
export ALIYUN_TEST_ACCESS_KEY_ID=******
export ALIYUN_TEST_ACCESS_KEY_SECRET=******
export ALIYUN_TEST_OSS_TEST_RULE_CLASS=org.apache.iceberg.aliyun.oss.OSSIntegrationTestRule
export ALIYUN_TEST_OSS_WAREHOUSE=oss://iceberg-test/ververica-iceberg-integration-tests/
export ALIYUN_TEST_OSS_ENDPOINT=oss-cn-hangzhou.aliyuncs.com
➜ apache-iceberg git:(aliyun-it-rule) ./gradlew iceberg-aliyun:build -x javadoc -Pquick=true
BUILD SUCCESSFUL in 53s
19 actionable tasks: 4 executed, 15 up-to-date
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on pull request #3687: Aliyun: Add OSS integration test rule
Posted by GitBox <gi...@apache.org>.
openinx commented on pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687#issuecomment-989404670
Get this merged, Thanks @jackye1995 for the reviewing !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #3687: Aliyun: Add OSS integration test rule
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687#discussion_r765366846
##########
File path: aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java
##########
@@ -69,6 +70,9 @@ public void after() {
@Test
public void testBuckets() {
+ Assume.assumeTrue("Aliyun integration test cannot delete existing bucket from test environment.",
+ OSS_TEST_RULE.getClass() == AliyunOSSMockRule.class);
Review comment:
> In AWS we just assumes the bucket exists, and create a random prefix as the root for all tests.
The aliyun integration tests is using the same approach as you said. This suite case was designed to guarantee that the Aliyun Mock OSS Application will have the same semantics as the production aliyun OSS services. If both AliyunOSSMockRule and OSSIntegrationTestRule are successfully applied to this case, then we can ensure that all the unit tests which are built on top of the AliyunOSSMockRule are correctly designed.
As we iceberg FileIO won't depend any bucket creation or deletion interfaces , plus the dangerous bucket deletion behavior. So Here I disabled the buckets tests for integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx merged pull request #3687: Aliyun: Add OSS integration test rule
Posted by GitBox <gi...@apache.org>.
openinx merged pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #3687: Aliyun: Add OSS integration test rule
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687#discussion_r764837542
##########
File path: aliyun/src/test/java/org/apache/iceberg/aliyun/AliyunTestUtility.java
##########
@@ -17,25 +17,36 @@
* under the License.
*/
-package org.apache.iceberg.aliyun.oss;
+package org.apache.iceberg.aliyun;
Review comment:
Move this utility class out of the `org.apache.iceberg.aliyun.oss` because the utility will also be used for other aliyun services , such as the future aliyun dlf catalog etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] openinx commented on a change in pull request #3687: Aliyun: Add OSS integration test rule
Posted by GitBox <gi...@apache.org>.
openinx commented on a change in pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687#discussion_r764761651
##########
File path: aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java
##########
@@ -69,6 +70,9 @@ public void after() {
@Test
public void testBuckets() {
+ Assume.assumeTrue("Aliyun integration test cannot delete existing bucket from test environment.",
+ OSS_TEST_RULE.getClass() == AliyunOSSMockRule.class);
Review comment:
Add this sentence because we don't want to run this test cases for integration test, because it will delete the bucket from test environment, while the test environment may have other objects that were used by other tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3687: Aliyun: Add OSS integration test rule
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687#discussion_r765268155
##########
File path: aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java
##########
@@ -69,6 +70,9 @@ public void after() {
@Test
public void testBuckets() {
+ Assume.assumeTrue("Aliyun integration test cannot delete existing bucket from test environment.",
+ OSS_TEST_RULE.getClass() == AliyunOSSMockRule.class);
Review comment:
In AWS we just assumes the bucket exists, and create a random prefix as the root for all tests. By doing that we don't deal with bucket deletion, which is quite dangerous as you say. Would that be better also for Aliyun?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #3687: Aliyun: Add OSS integration test rule
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3687:
URL: https://github.com/apache/iceberg/pull/3687#discussion_r765368114
##########
File path: aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java
##########
@@ -69,6 +70,9 @@ public void after() {
@Test
public void testBuckets() {
+ Assume.assumeTrue("Aliyun integration test cannot delete existing bucket from test environment.",
+ OSS_TEST_RULE.getClass() == AliyunOSSMockRule.class);
Review comment:
I see, that makes sense to me, thanks for the explanation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org