-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
REST: Docker file for REST Fixture #11283
Conversation
ed2ba10
to
56ee5ee
Compare
b497524
to
e8a7a36
Compare
d039972
to
65be33f
Compare
65be33f
to
d8b98e3
Compare
@@ -985,6 +985,15 @@ project(':iceberg-open-api') { | |||
exclude group: 'org.apache.commons', module: 'commons-configuration2' | |||
exclude group: 'org.apache.hadoop.thirdparty', module: 'hadoop-shaded-protobuf_3_7' | |||
exclude group: 'org.eclipse.jetty' | |||
exclude group: 'com.google.re2j', module: 're2j' |
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.
Excluded some more which are unrelated to 'Configuration' class.
These were not included in the license too.
e0c357e
to
a69b02f
Compare
a69b02f
to
8c89087
Compare
@Fokko: Thanks for the review. When I exclude some dependencies from hadoop-common (like hadoop auth), it failed at runtime. I fixed and double checked now. Everything works fine. Steps to verify.
|
8c89087
to
15baa13
Compare
Hey @ajantha-bhat Thanks for taking a stab at this again. My main goal is to use this docker image to replace PyIceberg, Iceberg-Rust, and Iceberg-Go. These repositories still rely on a third-pary container that we want to get rid of (I believe you also raised this earlier). I tried this, but failed because it didn't come with the AWS Runtime:
This is because we store the metadata in Minio, so the metadata is easily accessible outside of the container as well. How do you feel about adding the S3 runtime? Steps to run the tests: git clone [email protected]:apache/iceberg-python.git
cd iceberg-python Apply patch as described in #11283 (review):
And run the tests: make install
make test-integration Tail the logs using: docker compose -f dev/docker-compose-integration.yml logs -f |
@Fokko: I have also added GCP and Azure runtime dependency.
|
@ajantha-bhat Thanks for adding the runtime dependencies! 🙌 Yes, that looks like it will be fixed in apache/iceberg-python#1321 (review). I'll do some final checks, but I think this is ready 👍 |
The patch fixes it indeed 👍
|
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.
Did some final checks on the licenses, looks all good 👍 Thanks @ajantha-bhat for fixing this, @kevinjqliu, @findepi, @mrcnc for the review and thanks @bryanck for the help around the licenses generation.
I'm doing a new pass on license and content (Cat A deps). |
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.
LGTM! Built locally and ran the image against iceberg-python
, iceberg-go
, and iceberg-rust
integration tests, all passed.
Here are some other references to tabulario/iceberg-rest
https://grep.app/search?q=tabulario/iceberg-rest&filter[repo.pattern][0]=apache
@jbonofre: Do you have any more comments for this? |
@@ -81,6 +81,9 @@ Copyright 2002-2024 The Apache Software Foundation | |||
Apache Commons Lang | |||
Copyright 2001-2023 The Apache Software Foundation | |||
|
|||
Apache Commons Configuration |
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.
Are you sure about this ?
I checked the NOTICE
file in commons-configuration, and I see:
Apache Commons Configuration
Copyright 2001-2013 The Apache Software Foundation
This product includes software developed at
The Apache Software Foundation (http://www.apache.org/).
As reminder, the purpose of ALv2 4.d, is to include NOTICE
content (as it is) in the NOTICE
file.
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.
I am sure.
https://github.com/apache/commons-configuration/blob/master/NOTICE.txt
This product includes software developed at
The Apache Software Foundation (http://www.apache.org/).
This line is there at the beginning of the section
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.
I added this for each one instead of section. So, notice looks as it is.
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.
OK, that's the NOTICE
from main
branch. I checked in the commons-configuration distribution of the version used in Iceberg.
That's why I was not sure.
@ajantha-bhat @Fokko quick question for you guys. We can use a single DockerHub repo ( |
@jbonofre I'm strongly in favor of having separate repositories so we can separate the different containers nicely. |
@jbonofre: Separate repo is fine for me. |
Thanks @ajantha-bhat for working on this, and thanks @bryanck, @mrcnc, @kevinjqliu, @jbonofre and @findepi for reviewing 🎉 |
depends on #11279