-
Notifications
You must be signed in to change notification settings - Fork 52
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 image for hive3 and hadoop3 #13
Conversation
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.
My understanding is that most of the files were created as a copy from hdp2.6 and then applying certain changes. Now, these changes are not visible.
Could you please make 2 separate commits? One that is plain copy and second which changes the file contents as necessary?
prestodev/centos7-oj8/README.md
Outdated
|
||
## Oracle license | ||
|
||
By using this image, you accept the Oracle Binary Code License Agreement for Java SE available here: |
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.
Doesn't apply, the image uses openjdk
prestodev/centos7-oj8/README.md
Outdated
@@ -0,0 +1,13 @@ | |||
# centos7-oj8 [![][layers-badge]][layers-link] [![][version-badge]][dockerhub-link] | |||
|
|||
[layers-badge]: https://images.microbadger.com/badges/image/prestodev/centos7-oj8.svg |
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.
@kokosing i'd like to abandon this READMEs. I don't find them useful.
WDYT?
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 noticed the link are dead as of now but wasnt sure if there are plan to get them working so extended the readmes from existing ones.
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.
@stagraqubole i understand. You did the reasonable thing. Let's wait for @kokosing 's opinion before dropping this.
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 agree. Especially, if they have outdated information then they do more harm than good.
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 will remove these then.
prestodev/centos7-oj8/Dockerfile
Outdated
\ | ||
# install supervisor | ||
yum --enablerepo=extras install -y setuptools epel-release && \ | ||
sed -i 's/https/http/g' /etc/yum.repos.d/epel.repo && \ |
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.
Should not be needed. Why so?
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.
will double check.
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.
Thanks @stagraqubole for working on this!
By any chance, did you also prepare kerberized images?
They would be required when we want to run all the tests on hdp3.
I didnt prepare kerberized image, my requirement was limited to running tests using hive3.
Thanks @stagraqubole for working on this! By any chance, did you also prepare kerberized images? |
Makes sense, I will separate it out. |
5c90577
to
2a46baa
Compare
Updates as per the comments @findepi |
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.
Nice piece of work!
prestodev/centos7-oj8/README.md
Outdated
@@ -0,0 +1,13 @@ | |||
# centos7-oj8 [![][layers-badge]][layers-link] [![][version-badge]][dockerhub-link] | |||
|
|||
[layers-badge]: https://images.microbadger.com/badges/image/prestodev/centos7-oj8.svg |
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 agree. Especially, if they have outdated information then they do more harm than good.
prestodev/hdp3-hive3/Dockerfile
Outdated
@@ -59,7 +66,7 @@ RUN chmod 755 /root && chmod 700 /root/.ssh | |||
RUN passwd --unlock root | |||
|
|||
# HDFS ports | |||
EXPOSE 1004 1006 8020 50010 50020 50070 50075 50470 | |||
EXPOSE 1004 1006 8020 9866 9867 9870 9864 50470 |
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.
What happened to these ports? Is 50070 no longer exposed? Was it migrated somehow? I used to use it in other versions.
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.
Port have changed. I used https://issues.apache.org/jira/browse/HDFS-9427 and updated the ports that were in use.
0dec365
to
8584ce4
Compare
Handled your comments @kokosing |
Ah... I forgot about tests. Please see |
@kokosing I see that the test.sh file already has the test queries and can run for any profile which has compose file defined. Do you want me to make changes to ensure that it runs on hdp3-hive3 (like having compose file for hdp3-hive3 etc) or something more? |
Ran |
I also noticed that in test.sh, |
The expectation is to return the error code and do cleanup, hence the |
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.
Partial review
(until prestodev/hdp3-hive3/files/etc/supervisord.d/mysql-metastore.conf
)
|
||
<property> | ||
<name>hive.support.concurrency</name> | ||
<value>true</value> |
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.
What is this for?
(I think we'll never run any concurrent workload in hive directly.)
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.
This is a requirement for using DbTxnManager and DbTxnManager is a requirement for ACID.
@@ -1,5 +1,5 @@ | |||
[program:mysql-metastore] | |||
command=/usr/bin/pidproxy /var/run/mysqld/mysqld.pid /usr/bin/mysqld_safe | |||
command=/bin/bash -c "(chown -R mysql:mysql /var/lib/mysql) && (/usr/bin/pidproxy /var/run/mysqld/mysqld.pid /usr/bin/mysqld_safe)" |
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 guess this is not sufficient to chown
once in Dockerfile. Did you try? i wouldn't be surprised -- i think i encountered such a problem recently somewhere else.
no need for ( ... )
, please remove them
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 wasnt able to get it running with chown
in Dockerfile. Tried again by placing another instance of it after full setup but still there are failures when container starts.
@findepi handled your latest comments. Only the part about mysql vs mariadb is left. |
Can you please post a docker image under different repository name to docker hub for now? I would like to merge this PR and the one in Presto together. Notice that merging this PR is not enough, we still need to do a release. |
I have done that, PR is open in presto (with a bit outdated docker image): trinodb/trino#1034 I will update it with the latest image. |
Super. Thanks! |
This is done. Image is at shubhamtagra/hdp3.1-hive:1 |
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.
Looks good overall, a bunch of comments.
I'm testing this locally now.
I manually tested locally the metastore part of the image
It worked like a charm. Good job! |
i will take over this PR & merge it, if you don't mind. |
Thanks @stagraqubole, this is merged as 51c48e9 |
Images based on hdp3.
I have use image created via this change for Hive ACID tests in trinodb/trino#1034