Skip to content
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

Add hive 4.0 image #218

Merged

Conversation

mayankvadariya
Copy link
Contributor

@mayankvadariya mayankvadariya commented Dec 9, 2024

Build custom hive4 image with access to S3 by using plain apache hive4 image. Plain hive image offers an advantage of not having to tweak multiple configuration. However, currently created image doesn't include yarn services which will be required to run queries concurrently. We may need to build a new image to accommodate yarn services as we run different tests using Hive4.

@cla-bot cla-bot bot added the cla-signed label Dec 9, 2024
@mayankvadariya mayankvadariya force-pushed the mayank/hive4-using-apache-hive-image branch 11 times, most recently from 9e19ed7 to 314e24d Compare December 9, 2024 19:22
@@ -35,6 +35,9 @@ jobs:
- image: hive3.1-hive
platforms: linux/amd64,linux/arm64
test: hive3.1-hive
- image: hive4.0-hive
platforms: linux/amd64
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't aded linux/arm64 platform as test image had been complaining about different arch

The requested image's platform (linux/arm64) does not match the detected host platform (linux/amd64/v3) and no specific platform was requested

https://github.com/trinodb/docker-images/actions/runs/12242033317/job/34148454814?pr=218#step:5:837

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a code comment I guess

Copy link
Member

@hashhar hashhar Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on ci this is expected, what happens when you build the image locally? if it builds then you should allow all platforms here - CI will use emulation to build the arm version of the image but then devs who have arm machines will get arm images instead of having to emulate amd64 images.

on ci the node running the build is not an arm runner hence the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya I realized about the runner spec. However, its just not about warning but image test is failing with arm. Please advise if there is a way to run only amd64 test and not arm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to merge without arm image.

The fix isn't to skip tests on arm but rather to find what is failing. The logs here are almost useless, I'll take a look at this and submit follow up PR.


FROM apache/hive:4.0.0

# TODO replace with aws sdk v2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we missing to go ahead now with aws sdk v2 ?
Can you pls link an issue on Hive that needs to be done to be able to use aws sdk v2?

Copy link
Contributor Author

@mayankvadariya mayankvadariya Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it but /tmp/root/hive.log doesn't provide enough information on what is causing sdk v2 to not work.

@findinpath
Copy link
Contributor

Add appropriate description explaining why do we need to customize the vanilla apache hive 4 docker image.

@findinpath findinpath requested a review from mdesmet December 9, 2024 21:30
@mayankvadariya mayankvadariya force-pushed the mayank/hive4-using-apache-hive-image branch from 314e24d to b90a41d Compare December 9, 2024 23:27
# See the License for the specific language governing permissions and
# limitations under the License.

FROM apache/hive:4.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the base image include some arch-specific code? if not we can make this multi-arch by inlining the base image.

I can take a look at this as follow-up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@anusudarsan anusudarsan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % @hashhar 's comment

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow-up regarding arm image test failures. Feel free to merge without arm image.

@mayankvadariya
Copy link
Contributor Author

mayankvadariya commented Dec 11, 2024

@hashhar thanks for offering to take care of arm build. Would you please merge this PR?

@ebyhr
Copy link
Member

ebyhr commented Dec 11, 2024

@mayankvadariya Could you fix Polaris failure? Otherwise, we can't release a new version.

@mayankvadariya
Copy link
Contributor Author

@mayankvadariya Could you fix Polaris failure? Otherwise, we can't release a new version.

@ebyhr created #219. Please review.

@ebyhr ebyhr merged commit b1503a8 into trinodb:master Dec 12, 2024
13 of 14 checks passed
@ebyhr
Copy link
Member

ebyhr commented Dec 12, 2024

Running a release workflow: https://github.com/trinodb/docker-images/actions/runs/12289019120

# See the License for the specific language governing permissions and
# limitations under the License.

FROM apache/hive:4.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use 4.0.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing speaks against it but I am not sure on what major/minor releases do we test.
If we want to use the latest one then, I can change it to 4.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants