Add docker container for MapR by AS186170 · Pull Request #14 · Teradata/docker-images

AS186170 · 2017-01-31T19:02:56Z

mapr52-code checkin for docker container

sanjay990 · 2017-01-31T19:14:14Z

Rewrite the commit message. Something like Add docker container for MapR

sanjay990 · 2017-01-31T19:17:59Z

teradatalabs/mapr52-base/Dockerfile

@@ -0,0 +1,55 @@
+FROM teradatalabs/centos6-java8-oracle


License header missing. Add them to other files too

@sanjay990 License header added for the files

sanjay990 · 2017-01-31T19:20:04Z

teradatalabs/mapr52-base/Dockerfile

+FROM teradatalabs/centos6-java8-oracle
+MAINTAINER Teradata Docker Team <docker@teradata.com>
+
+


Remove blank line

sanjay990 · 2017-01-31T19:46:03Z

teradatalabs/mapr52-base/Dockerfile

+RUN rpm --import http://package.mapr.com/releases/pub/maprgpg.key
+
+#add user and password
+RUN adduser mapr


Adding user and creating a home directory can be merged into one -d. Also looks like you can merge many of these commands. Try to combine them as far as possible

sanjay990 · 2017-01-31T19:47:28Z

teradatalabs/mapr52-base/Dockerfile

+
+#Add repo for mapr
+ADD files/maprtech.repo /etc/yum.repos.d/maprtech.repo
+RUN yum update -y


If possible, combine all the software installation with this step and do this first.

I don't know if I agree. Keeping the packages close to where they are configured makes more sense to me. Maybe the line below about installing utility packages can be merged here but I think the MapR and Python installation blocks should remain where they are.

sanjay990 · 2017-01-31T19:48:10Z

teradatalabs/mapr52-hive/Dockerfile

+
+# RUN SETUP script
+RUN /root/setup.sh
+


Remove all these extra lines from all the files.

sanjay990 · 2017-01-31T19:51:46Z

teradatalabs/mapr52-hive/files/socks-proxy.sh

@@ -0,0 +1 @@
+../../../commons/socks-proxy.sh


Add new line

@sanjay990 new line is not required as per artur , its same in other containers also

sanjay990 · 2017-01-31T19:52:14Z

teradatalabs/mapr52-hive/files/supervisord.d/bootstrap.conf

@@ -0,0 +1 @@
+../../../../commons/supervisord.d/bootstrap.conf


Add new line

Those are symlinks, so there's no control over it / doesn't matter

@sanjay990 new line is not required as per artur , its same in other containers also

sanjay990 · 2017-01-31T19:52:18Z

teradatalabs/mapr52-hive/files/supervisord.d/mysql-metastore.conf

@@ -0,0 +1 @@
+../../../../commons/supervisord.d/mysql-metastore.conf


Add new line

@sanjay990 new line is not required as per artur , its same in other containers also

The symlinks got messed up somehow. Please replace all the files with similar content with symlinks to the files pointed by the paths in those files.

sanjay990 · 2017-01-31T19:52:21Z

teradatalabs/mapr52-hive/files/supervisord.d/socks-proxy.conf

@@ -0,0 +1 @@
+../../../../commons/supervisord.d/socks-proxy.conf


Add new line

@sanjay990 new line is not required as per artur , its same in other containers also

sanjay990 · 2017-01-31T19:52:51Z

teradatalabs/mapr52-hive/files/wardenTracker.sh

+done
+
+
+


Remove extra lines

ArturGajowy · 2017-01-31T21:04:11Z

Please split the commit in two parts, one for the base image and another one for the -hive one

ArturGajowy · 2017-01-31T21:05:13Z

Please add a capabilities.txt file so that the tests are run for the -hive image. See any ohter -hive image.

ArturGajowy

just a skim and two comments for now, please ping me when those are addressed

petroav

As Sanjay pointed out, please re-write your commit message. In general, if you are committing for any Presto related project follow this guideline: http://chris.beams.io/posts/git-commit/.

A lot of my comments relate to style, which is just as important as content. In future pay attention to the style around you in the project and try emulate it as much as possible.

petroav · 2017-01-31T19:24:23Z

teradatalabs/mapr52-base/Dockerfile

+MAINTAINER Teradata Docker Team <docker@teradata.com>
+
+
+#Add repo for mapr


Please adopt a consistent comment style throughout this commit. Other Dockerfiles all have a '#' followed by a space, followed by a comment. The comment should start with a capital letter or be in all caps. The other Dockerfiles have mixed style in terms of small vs large cap.

Also, please stylize mapr as MapR in all your comments.

petroav · 2017-01-31T20:16:11Z

teradatalabs/mapr52-base/Dockerfile

+
+#Add repo for mapr
+ADD files/maprtech.repo /etc/yum.repos.d/maprtech.repo
+RUN yum update -y


I don't know if I agree. Keeping the packages close to where they are configured makes more sense to me. Maybe the line below about installing utility packages can be merged here but I think the MapR and Python installation blocks should remain where they are.

petroav · 2017-01-31T20:16:37Z

teradatalabs/mapr52-base/Dockerfile

+ADD files/maprtech.repo /etc/yum.repos.d/maprtech.repo
+RUN yum update -y
+
+#install utility softwares


Software is a non-countable noun so softwares is incorrect. Either say "Install utility software" or "Install utility packages"

petroav · 2017-01-31T20:18:20Z

teradatalabs/mapr52-base/Dockerfile

+RUN yum update -y
+
+#install utility softwares
+RUN yum install iputils vim openssh-server openssh-clients sudo -y


I think it is more idiomatic for Linux commands to have options before the positional arguments so I think you should move the -y option to the front on all instances of yum install in this Dockerfile and other files.

petroav · 2017-01-31T20:18:36Z

teradatalabs/mapr52-base/Dockerfile

+#install utility softwares
+RUN yum install iputils vim openssh-server openssh-clients sudo -y
+
+#configure sshh


sshh is incorrect. Also stylize to SSH.

petroav · 2017-01-31T21:00:59Z

teradatalabs/mapr52-hive/files/supervisord.conf

@@ -0,0 +1,22 @@
+[supervisord]


Can't we use the configuration in the common subfolder of the project? I'm not too familiar how this work so maybe this is a question for @ArturGajowy.

petroav · 2017-01-31T21:01:51Z

teradatalabs/mapr52-hive/files/supervisord.conf

+
+[supervisorctl]
+serverurl = unix:///tmp/supervisor.sock
+prompt = cdh-pseudo-distributed


If we do end up keeping this config maybe we should update this to say something other than cdh-pseudo-distributed?

petroav · 2017-01-31T21:31:05Z

teradatalabs/mapr52-hive/files/wardenTracker.sh

+   maprcliReady=$(maprcli service list -node $hname | grep 'ERROR (10009)' | wc -l)
+   Services=0
+else
+   Services=$(maprcli service list -node $hname | grep JobHistoryServer |awk '{$1=$1};1' | tr ' ' '\n' | tail -1f)


You're grepping for JobHistoryServer but the comment says CLDB.

petroav · 2017-01-31T21:31:20Z

teradatalabs/mapr52-hive/files/wardenTracker.sh

+done
+
+#wait for NodeManager to start
+


Extra newline here and in the other code blocks.

petroav · 2017-01-31T21:32:06Z

teradatalabs/mapr52-hive/files/wardenTracker.sh

@@ -0,0 +1,54 @@
+#!/bin/bash


I don't know enough bash to say if this is idiomatic so I'll leave the review here to @ArturGajowy.

ArturGajowy · 2017-02-07T13:56:47Z

Guys, any updates on this? Please ping me when the (actionable) comments made so far are applied

cc @petroav @AS186170

AS186170 · 2017-02-13T20:27:56Z

@ArturGajowy i have started working on modification for the review comments today.Will let you know once completed.

Add docker container for MapR

Add review changes for MapR docker container

Rebasing the branch to master for splitting mapr commits into two

Add docker container code for MapR-Base

Add docker container code for MapR-Hive

AS186170 · 2017-02-15T16:38:14Z

@sanjay990 @petroav @ArturGajowy Review comments are now complete for the branch

petroav

Couple small comments.

petroav · 2017-02-16T23:59:40Z

teradatalabs/mapr52-base/README.md

+# mapr52-base
+
+
+Docker image with Hive installed from MapR repositories.


I don't think this is correct. This README is in the mapr52-base folder so this image shouldn't have Hive installed.

+1. Just base this readme on https://github.com/Teradata/docker-images/blob/master/teradatalabs/iop-base/README.md

petroav · 2017-02-16T23:59:48Z

teradatalabs/mapr52-base/README.md

@@ -0,0 +1,9 @@
+# mapr52-base
+
+


Extra newline.

petroav · 2017-02-17T16:21:36Z

teradatalabs/mapr52-hive/README.md

@@ -0,0 +1,17 @@
+# mapr52-hive 
+
+Docker image with HDFS, YARN and HIVE installed. Please note that running services have lower memory heap size set.


There's no HDFS in this image, only MapR FS.

petroav · 2017-02-17T16:23:28Z

teradatalabs/mapr52-hive/capabilities.txt

@@ -0,0 +1,4 @@
+exposes_hive


What is this file for?

So that the tests defined in https://github.com/Teradata/docker-images/blob/master/test/image_tests/image_tests.bats.sh are run for this image

petroav · 2017-02-17T16:32:31Z

teradatalabs/mapr52-hive/files/setup.sh

@@ -0,0 +1,9 @@
+#!/bin/bash


Hmm, yea but for example setup.sh for cdh5-hive-master does a bunch of things, so it's OK to name it something generic like setup.sh whereas here all we're doing is configuring MySQL so I think we should rename it. Don't feel strongly about it though. I can also see the argument for being consistent with other image files. Up to you.

ArturGajowy

The tests fail (see travis log). Seems like hive is not reachable on hadoop-master:10000. Please fix it and then we can proceed.

ArturGajowy · 2017-02-22T11:42:40Z

Please make the PR only contain two commits, with the respective messages:
Add mapr-base image
Add mapr-hive image
both containing only the changes relevant accordting to their messages

ArturGajowy · 2017-02-22T11:46:14Z

Please drop '52' from the image & directories names, we're not going to have two versions of the mapr container (say mapr-52 and mapr-53), so there's no point.

ArturGajowy · 2017-02-22T11:48:16Z

teradatalabs/mapr52-base/README.md

@@ -0,0 +1,9 @@
+# mapr52-base


add the badges, as it is done for any other image. Just copy + paste then search & replace the image name

ArturGajowy

First round done. Let me know when that's fixed and the tests pass on travis.

ArturGajowy · 2017-02-22T11:48:32Z

teradatalabs/mapr52-hive/README.md

@@ -0,0 +1,17 @@
+# mapr52-hive 


ditto badges

ArturGajowy · 2017-02-22T11:49:46Z

teradatalabs/mapr52-base/README.md

+# mapr52-base
+
+
+Docker image with Hive installed from MapR repositories.


+1. Just base this readme on https://github.com/Teradata/docker-images/blob/master/teradatalabs/iop-base/README.md

ArturGajowy · 2017-02-22T11:51:48Z

teradatalabs/mapr52-hive/capabilities.txt

@@ -0,0 +1,4 @@
+exposes_hive


So that the tests defined in https://github.com/Teradata/docker-images/blob/master/test/image_tests/image_tests.bats.sh are run for this image

ArturGajowy · 2017-02-22T11:52:19Z

teradatalabs/mapr52-hive/files/bootstrap.sh

+#!/bin/sh
+
+# CONFIGURE MapR
+/opt/mapr/server/configure.sh  -N mycluster -Z localhost -C localhost -HS localhost -no-autostart


double space before -N

ArturGajowy · 2017-02-22T11:54:00Z

teradatalabs/mapr52-hive/files/wardenTracker.sh

@@ -0,0 +1,54 @@
+#!/bin/bash
+
+hname=$(hostname)


use CAPITAL_UNDERSCORE variable names, as is the de-facto standard for bash files

ArturGajowy · 2017-02-22T17:10:50Z

teradatalabs/mapr52-hive/Dockerfile

+MAINTAINER Teradata Docker Team <docker@teradata.com>
+
+# ADD ALL REQUIRED SCRIPTS AND FILES TO ROOT DIRECTORY
+ADD files/*.sh /root/


add each file verbatim, don't use *. It's better if we keep this explicit

ArturGajowy · 2017-02-22T17:12:22Z

teradatalabs/mapr52-hive/Dockerfile

+ADD files/conf/hive-site.xml /opt/mapr/hive/hive-1.2/conf/hive-site.xml
+ADD files/conf/core-site.xml /opt/mapr/hadoop/hadoop-2.7.0/etc/hadoop/core-site.xml
+ADD files/supervisord.conf /etc/supervisord.conf
+COPY files/supervisord.d/* /etc/supervisord.d/


This should be done in the base image, see

# Copy supervisord startup script and base configs COPY files/startup.sh /root/startup.sh COPY files/supervisord.conf /etc/supervisord.conf COPY files/supervisord.d/bootstrap.conf /etc/supervisord.d/bootstrap.conf # Add supervisord configs in child images ONBUILD COPY files/supervisord.d/* /etc/supervisord.d/

in any other base image

ArturGajowy · 2017-02-22T17:13:37Z

teradatalabs/mapr52-hive/Dockerfile

+COPY files/supervisord.d/* /etc/supervisord.d/
+
+RUN chmod 777 /root/*.sh \
+# INSTALL UTILITY SOFTWARE  


don't install utility software, unless it's in a centos6-* image and we've all agreed it's needed

ArturGajowy · 2017-02-22T17:14:38Z

teradatalabs/mapr52-hive/Dockerfile

+ADD files/supervisord.conf /etc/supervisord.conf
+COPY files/supervisord.d/* /etc/supervisord.d/
+
+RUN chmod 777 /root/*.sh \


why is that neeeded? Try to remove that or add a comment why it's needed

ArturGajowy · 2017-02-22T17:16:01Z

teradatalabs/mapr52-hive/files/bootstrap.sh

+/opt/mapr/server/configure.sh  -N mycluster -Z localhost -C localhost -HS localhost -no-autostart
+
+# SETUP FLAT FILE /home/mapr/storagefile
+dd if=/dev/zero of=/home/mapr/storagefile bs=1G count=10


I'm not sure if this should be done here or in a setup.sh script. Probably depends on image size and the container startup time

Add Kerberize docker container for MapR with hive

Add modification for docker-base

Modification for kerberized MapR container

Adding modifications to test cluster for mapr

This reverts commit 586ebc7.

Adding test container changes to accomodate mapr

Adding Symlinks for the common files

Reverting links

Modification to include env.sh changes in Kerberized MapR cluster

Adding commands to start httpfs services

Adding privileged in docker-compose for MapR

Changing Test container to accomodate mapr

modification of tests to accomadate hive user

Fixing kerberos container

Debugging Bootsrap to know where its stuck

Reverting changes for debugging bootstrap

Moving partition creation to mapr-base and changes in capabilities

decreasing allocation of space to 3 GB

Increasing allocation of space back to 10 GB

makefile changes to debug images size

Changes in travis to increase wait time during build

Reverting the debugging changes

Adding httpfs to MapR-base and moving storagefile partition creation to hive

Removing creation of partition from bootstrap of kerberized container and removing chown

Adding SSHD AND THE SOCKS PROXY for kerberized mapr

Rearranging the code in MapR cluster to make it ready for review

This changes will make maprcli wait for MapR ticket in wardenTracker

changes in travis

Travis yml changes

AS186170 · 2017-04-25T13:46:02Z

@petroav @ArturGajowy All the review comments are incorporated and all the tests are passing .

mapr52-code checkin

6559ed5

mapr52-code checkin for docker container

sanjay990 self-requested a review January 31, 2017 19:17

sanjay990 requested changes Jan 31, 2017

View reviewed changes

ArturGajowy reviewed Jan 31, 2017

View reviewed changes

petroav suggested changes Jan 31, 2017

View reviewed changes

AS186170 changed the title ~~mapr52-code checkin~~ Add docker container for MapR Feb 13, 2017

singhash added 5 commits February 14, 2017 15:51

Add docker container for MapR

4e58395

Add docker container for MapR

Add review changes for MapR docker container

9a9b813

Add review changes for MapR docker container

Rebasing the branch to master

2c4ab26

Rebasing the branch to master for splitting mapr commits into two

Add docker container for MapR without hive

c09dafb

Add docker container code for MapR-Base

Add docker container for MapR with hive

4093ab7

Add docker container code for MapR-Hive

petroav reviewed Feb 17, 2017

View reviewed changes

ArturGajowy suggested changes Feb 21, 2017

View reviewed changes

ArturGajowy reviewed Feb 22, 2017

View reviewed changes

ArturGajowy suggested changes Feb 22, 2017

View reviewed changes

singhash added 7 commits March 16, 2017 16:35

Add Kerberize docker container for MapR with hive

fceaa6e

Add Kerberize docker container for MapR with hive

Add modification for docker-base

614519c

Add modification for docker-base

Modification for kerberized MapR container

7bdc127

Modification for kerberized MapR container

Adding modifications to test cluster for mapr

586ebc7

Adding modifications to test cluster for mapr

Revert "Adding modifications to test cluster for mapr"

4426b3b

This reverts commit 586ebc7.

Adding test container changes for mapr

dfcb009

Adding test container changes to accomodate mapr

Merge remote-tracking branch 'refs/remotes/origin/master' into mapr-52

63b3db4

singhash and others added 29 commits March 23, 2017 13:34

Adding Symlinks for the common files

8061998

Adding Symlinks for the common files

Reverting links

52ae1d5

Reverting links

Add symlinks to mapr52-hive

b4edeb9

Fix links inside supervisord.d directory

3e15ca4

Modification to include env.sh changes

b7a3b58

Modification to include env.sh changes in Kerberized MapR cluster

Adding commands to start httpfs services

56c37e9

Adding commands to start httpfs services

Adding privileged in docker-compose for MapR

875f70d

Adding privileged in docker-compose for MapR

Changing Test container to accomodate mapr

86ae203

Changing Test container to accomodate mapr

modification of tests to accomadate hive user

d4223b4

modification of tests to accomadate hive user

Fixing kerberos container

3ebb748

Fixing kerberos container

Start sshd and the socks-proxy from bootstrap.sh

1f5f062

Changes for Debugging Bootsrap to know where its stuck

239e587

Debugging Bootsrap to know where its stuck

Reverting changes for debugging bootstrap

2fb8915

Reverting changes for debugging bootstrap

Moving partition creation to mapr-base

54221d4

Moving partition creation to mapr-base and changes in capabilities

decreasing allocation of space to 3 GB

73461bb

decreasing allocation of space to 3 GB

Increasing allocation of space back to 10 GB

7cd2813

Increasing allocation of space back to 10 GB

Pick user to execute Hadoop commands based on image name

904a566

makefile changes to debug images size

7259c45

makefile changes to debug images size

Changes in travis to increase wait time during build

f5b8ae0

Changes in travis to increase wait time during build

Reverting the debugging changes

c0fd0ea

Reverting the debugging changes

Adding httpfs to MapR-base

0dfc673

Adding httpfs to MapR-base and moving storagefile partition creation to hive

Removing creation of partition from kerberized container

e4c5f91

Removing creation of partition from bootstrap of kerberized container and removing chown

Adding SSHD AND THE SOCKS PROXY for kerberized mapr

c01bcd5

Adding SSHD AND THE SOCKS PROXY for kerberized mapr

Rearranging the code in MapR cluster to make it ready for review

d15d552

Rearranging the code in MapR cluster to make it ready for review

Adding missing sshd service start in base

6458491

Adding Logic to wait for MapR ticket in Warden

ad61f7a

This changes will make maprcli wait for MapR ticket in wardenTracker

changes in travis

96627fc

changes in travis

Adding Review Changes for MapR

6046f9f

Travis yml changes

ec74714

Travis yml changes

		FROM teradatalabs/centos6-java8-oracle
		MAINTAINER Teradata Docker Team <docker@teradata.com>

		@@ -0,0 +1 @@
		../../../commons/socks-proxy.sh No newline at end of file

		@@ -0,0 +1 @@
		../../../../commons/supervisord.d/bootstrap.conf No newline at end of file

		@@ -0,0 +1 @@
		../../../../commons/supervisord.d/mysql-metastore.conf No newline at end of file

		@@ -0,0 +1 @@
		../../../../commons/supervisord.d/socks-proxy.conf No newline at end of file

		MAINTAINER Teradata Docker Team <docker@teradata.com>


		#Add repo for mapr

		# mapr52-base


		Docker image with Hive installed from MapR repositories.

		@@ -0,0 +1,17 @@
		# mapr52-hive

		Docker image with HDFS, YARN and HIVE installed. Please note that running services have lower memory heap size set.

		@@ -0,0 +1,54 @@
		#!/bin/bash

		hname=$(hostname)

Comments

Conversation

AS186170 commented Jan 31, 2017

Uh oh!

sanjay990 commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArturGajowy commented Jan 31, 2017

Uh oh!

ArturGajowy commented Jan 31, 2017

Uh oh!

ArturGajowy left a comment

Choose a reason for hiding this comment

Uh oh!

petroav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!