You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "shouwangyw (via GitHub)" <gi...@apache.org> on 2024/10/29 00:56:17 UTC
[PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
shouwangyw opened a new pull request, #3306:
URL: https://github.com/apache/amoro/pull/3306
## Why are the changes needed?
Close #3301.
## Brief change log
- Add OSS storage type, support OSS for iceberg in InternalCatalog
## How was this patch tested?
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] Run test locally before making a pull request
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2443102355
Thank you for your contribution. As far as I know, the current S3 protocol can also use Ali Cloud OSS object storage. Is iceberg storage support added here? By the way, we should make OSS optional at compile time
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2456313198
you can use s3 protocol to read it
cc @shouwangyw
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2444076155
> Thank you for your contribution. As far as I know, the current S3 protocol can also use Ali Cloud OSS object storage. Is iceberg storage support added here? By the way, we should make OSS optional at compile time
I don't understand how to use Ali Cloud OSS object storage with the S3 protocol？
This PR has added Ali Cloud OSS related dependencies
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2456313785
CI is Failing
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2458635386
> > you can use s3 protocol to read it
> > cc @czy006
>
> But this might not be intuitive
Of course this is a compromise. I would like to ask if we can have a compilation switch to compile the OSS part when we needed? As object storage support increases, compiling all by default will increase Amoro's binary installation package
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on code in PR #3306:
URL: https://github.com/apache/amoro/pull/3306#discussion_r1850514692
##########
http/RestCatalogLocalTest.http:
##########
@@ -0,0 +1,54 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+### get catalog config
+GET http://localhost:1630/api/iceberg/rest/v1/config?warehouse=iceberg
Review Comment:
This may not be suitable for unit tests or integration tests, as it relies on the local environment. I added a readme file to explain it.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2451329568
OSS support can be correctly added through the front end
<img width="1315" alt="image" src="https://github.com/user-attachments/assets/836c9149-4572-4c57-a4fe-f0f5b85b8626">
I have completed the test in my local environment, able to obtain relevant data correctly
<img width="1536" alt="image" src="https://github.com/user-attachments/assets/f92c599d-cd46-4165-ac70-27f2d38299ea">
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2456543161
> CI is Failing
I have fixed the ci
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on code in PR #3306:
URL: https://github.com/apache/amoro/pull/3306#discussion_r1832143925
##########
README.md:
##########
@@ -117,6 +117,7 @@ Amoro is built using Maven with JDK 8 and JDK 17(only for `amoro-format-mixed/am
* Build and skip tests: `mvn clean package -DskipTests`
* Build and skip dashboard: `mvn clean package -Pskip-dashboard-build`
* Build and disable disk storage, RocksDB will NOT be introduced to avoid memory overflow: `mvn clean package -DskipTests -Pno-extented-disk-storage`
+* Build and disable aliyun sdk: `mvn clean package -DskipTests -Pno-aliyun-sdk`
Review Comment:
I recommend not compiling **Aliyun OSS** by default, if the user wants to use it can use **-Paliyun-oss-sdk**
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on code in PR #3306:
URL: https://github.com/apache/amoro/pull/3306#discussion_r1840100943
##########
http/RestCatalogLocalTest.http:
##########
@@ -0,0 +1,54 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+### get catalog config
+GET http://localhost:1630/api/iceberg/rest/v1/config?warehouse=iceberg
Review Comment:
I am not sure what is the usage of this new file?
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2461518499
## [Codecov](https://app.codecov.io/gh/apache/amoro/pull/3306?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 22.54%. Comparing base [(`f13f3f3`)](https://app.codecov.io/gh/apache/amoro/commit/f13f3f340127e3447918e014766337b6b3bb58f6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`99e0eba`)](https://app.codecov.io/gh/apache/amoro/commit/99e0eba7acbda1fc688bf3a630ef34b2ee5658de?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 7 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (f13f3f3) and HEAD (99e0eba). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (f13f3f3) | HEAD (99e0eba) |
>|------|------|------|
>|core|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3306 +/- ##
============================================
- Coverage 30.17% 22.54% -7.63%
+ Complexity 3840 2312 -1528
============================================
Files 580 397 -183
Lines 48020 38201 -9819
Branches 6207 5437 -770
============================================
- Hits 14488 8611 -5877
+ Misses 32542 28863 -3679
+ Partials 990 727 -263
```
| [Flag](https://app.codecov.io/gh/apache/amoro/pull/3306/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [core](https://app.codecov.io/gh/apache/amoro/pull/3306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [trino](https://app.codecov.io/gh/apache/amoro/pull/3306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `22.54% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/amoro/pull/3306?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2458661017
> > > you can use s3 protocol to read it
> > > cc @czy006
> >
> >
> > But this might not be intuitive
>
> Of course this is a compromise. I would like to ask if we can have a compilation switch to compile the OSS part when we needed? As object storage support increases, compiling all by default will increase Amoro's binary installation package
yes, I can limit the package through scope provided
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on code in PR #3306:
URL: https://github.com/apache/amoro/pull/3306#discussion_r1840174628
##########
http/RestCatalogLocalTest.http:
##########
@@ -0,0 +1,54 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+### get catalog config
+GET http://localhost:1630/api/iceberg/rest/v1/config?warehouse=iceberg
Review Comment:
This is for easy testing of HTTP interfaces in a local environment, and the 'HTTP Client' plugin can be installed in IDEA.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "shouwangyw (via GitHub)" <gi...@apache.org>.
shouwangyw commented on PR #3306:
URL: https://github.com/apache/amoro/pull/3306#issuecomment-2456322216
> you can use s3 protocol to read it
>
> cc @shouwangyw
But this might not be intuitive
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong merged PR #3306:
URL: https://github.com/apache/amoro/pull/3306
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-3301] Support OSS for iceberg in InternalCatalog [amoro]
Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on code in PR #3306:
URL: https://github.com/apache/amoro/pull/3306#discussion_r1841499349
##########
http/RestCatalogLocalTest.http:
##########
@@ -0,0 +1,54 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+### get catalog config
+GET http://localhost:1630/api/iceberg/rest/v1/config?warehouse=iceberg
Review Comment:
So is this used for local testing? I'm not sure if this file is necessary for other developers. If it is, we might need to add a README to explain how to use it.
Additionally, I'm not sure if using the IDEA plugin is a reasonable way to test the REST API. Perhaps we should integrate them into unit tests or 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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org