TEZ-4682: [Cloud] Tez AM docker image#456
TEZ-4682: [Cloud] Tez AM docker image#456Aggarwal-Raghav wants to merge 10 commits intoapache:masterfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
@abstractdog , I was able to start DagAppMaster with ZK on local. Attaching logs for the container docker_logs.txt But this PR has lot of open items and I need some advice on the following:
|
Aggarwal-Raghav
force-pushed
the
TEZ-4682
branch
from
ffceca5 to
856875a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
very good, very good, let me check this in detail sometime this week, here are some pointers in the meantime, responding your questions:
there is no Yarn NodeManager in a k8s environment, so the reader of the entrypoint.sh should see a clear code distinguishing between needed env vars and legacy/backward-compatible env vars, that's what should be handled with care in my opinion
|
|
Thanks for the pointers @abstractdog .
Few additional things:
|
Aggarwal-Raghav
force-pushed
the
TEZ-4682
branch
from
856875a to
bfebf9e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Aggarwal-Raghav
force-pushed
the
TEZ-4682
branch
from
c371557 to
9efce66
Compare
Using tez.local.mode=true, solves this as it will use |
This comment was marked as outdated.
This comment was marked as outdated.
Aggarwal-Raghav
changed the title
Aggarwal-Raghav
force-pushed
the
TEZ-4682
branch
from
9efce66 to
551075b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@abstractdog , Can you please help with review? |
let me get back to this next week |
sure |
This comment was marked as outdated.
This comment was marked as outdated.
tez-dist/src/docker/README.md
Outdated
| mvn clean install -DskipTests -Pdocker,tools | ||
| ``` | ||
|
|
||
| 2. Install zookeeper in mac by: |
There was a problem hiding this comment.
can you add ubuntu steps? we might be so kind to let linux users' life be easier
There was a problem hiding this comment.
UPDATE: can we use a dockerized zookeeper instead? install ZK on the host machine looks against this whole cloud/docker initiative (also, in case of problems or ZK nodes messed up, deleting and restarting a container feels easier and cleaner to me)
tez-dist/src/docker/tez.env
Outdated
|
|
||
| # Tez AM Container Environment Configuration | ||
|
|
||
| HADOOP_USER_NAME=tez |
There was a problem hiding this comment.
nitpicking, can you order the env vars here the same as they are ordered in the entrypoint script?
tez-dist/pom.xml
Outdated
| <argument>${project.basedir}/src/docker/build-docker.sh</argument> | ||
| <argument>-hadoop</argument> | ||
| <argument>${hadoop.version}</argument> | ||
| <argument>-tez</argument> | ||
| <argument>${project.version}</argument> | ||
| <argument>-repo</argument> | ||
| <argument>apache</argument> |
There was a problem hiding this comment.
can you make it similar to what I can see in Hive? much less verbose, e.g.
<arguments>
<argument>.... .sh</argument>
<argument>-hadoop ${hadoop.version}</argument>
<argument>-tez ${tez.version}</argument>
</arguments>
|
Thanks for the thorough review @abstractdog . I'll address the review comments shortly, you can continue the review in the meantime. I hope you are able to build the image and start tez am standalone process 😅 I still believe we can get rid of hadoop tarball dependency completely as the hadoop dependent required jars are already part of tez tarball. It might unnecessary increase docker image size. Also please suggest should I use eclipse-temurin:21.0.3_9-jre-ubi9-minimal or jdk, in case we want to take jstack or other java debugging tools, jkd image is required. |
|
tez-dist/src/docker/build-docker.sh
Outdated
| # HADOOP FETCH LOGIC # | ||
| ###################### | ||
| HADOOP_FILE_NAME="hadoop-$HADOOP_VERSION.tar.gz" | ||
| HADOOP_URL=${HADOOP_URL:-"https://archive.apache.org/dist/hadoop/core/hadoop-$HADOOP_VERSION/$HADOOP_FILE_NAME"} |
There was a problem hiding this comment.
what about using this first:
https://dlcdn.apache.org/hadoop/common/hadoop-3.4.2/hadoop-3.4.2.tar.gz
and then fall back to archive
archive.apache.org is crazy slow for me at the moment (not the first time), maybe it would worth discovering dlcdn.apache.org
Let me check this in-depth over this weekend and I'll post my analysis/PR under the JIRA. Hoping its not too late 😅 Can you please check TEZ-4686 as well. With this tez-am docker image + standalone program or (tez master + hive master) the stacktrace is observed (under attachment section). I have raised a draft PR for this |
I believe this patch can be merged without TEZ-4686: with the current WIP patch of TEZ-4686, there is an issue which has to be resolved from both hive and tez side instead, I'll describe it in detail there |
|
|
In YARN the AM was started using these opts in my env. Based on this I have update the java opts to use
|
|
💔 -1 overall
This message was automatically generated. |
Aggarwal-Raghav
force-pushed
the
TEZ-4682
branch
from
2971a0b to
e360466
Compare
|
@abstractdog , I have made some changes in this:
|
|
|
||
| # Tez AM Container Environment Configuration | ||
|
|
||
| CONTAINER_ID=container_1700000000000_0001_01_000001 |
There was a problem hiding this comment.
hm, is CONTAINER_ID still needed? in the zookeeper standalone mode, it generates a new one, isn't it the case?
| USER=tez | ||
| HADOOP_USER_NAME=tez |
There was a problem hiding this comment.
I can see finally DAGAppMaster is started as:
-Duser.name="$HADOOP_USER_NAME" \
do we need a separate HADOOP_USER_NAME env var?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
tez-dist/src/docker/tez-am/README.md
Outdated
| Hadoop containers as defined in the `docker-compose.yml` file. | ||
|
|
||
| 8. For running a hive query using this docker image: | ||
| * Create a directory tez-plugins and add hive-exec jar in it. |
There was a problem hiding this comment.
first I was about to remove this, but we can keep, with some modification that a hive-related documentation burden is not on Tez community, so I believe "add hive-exec jar in it." is too specific, instead, this wants to be a guideline and say "Add all Hive jars that might be needed in the Tez AM, e.g., for split generation - typically the hive-exec jar, but in general, everything that was previously localized by Yarn."
There was a problem hiding this comment.
ACK. Please complete the review, will address all review comments.
| # volumes: | ||
| # - ./custom-tez-site.xml:/opt/tez/custom-conf/tez-site.xml | ||
| # Uncomment the following lines if you want to mount custom | ||
| # plugins/JARS for instance hive-exec jar |
There was a problem hiding this comment.
same here as recommended above regarding "hive-exec", maybe you can remove "for instance hive-exec jar" altogether, given that the other comment is descriptive enough
| - zookeeper_logs:/logs | ||
|
|
||
| hadoop-cluster: | ||
| image: minimal-hadoop:3.4.2 |
There was a problem hiding this comment.
can you give a try for the official hadoop image I suggested in another comment? apache/hive#6343 (comment)
There was a problem hiding this comment.
sure. will do it by tomorrow.
There was a problem hiding this comment.
Will use apache/hadoop:3.4.2-lean image.
|
thanks @Aggarwal-Raghav , this is so close, left minor comments
other than that, this is in a very good shape and I feel it's good to go in, we can fix anything later that might come along the way |
|
|
Everything should be running at this point With TEZ-4686 cherrypick: Without TEZ-4686 cherrypick: |
|
💔 -1 overall
This message was automatically generated. |
3 participants
No description provided.