[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r128219778
 
 

 ##
 File path: docker/Makefile
 ##
 @@ -0,0 +1,180 @@
+
+VERSION ?= 4.4.0-alpine
+IMAGE ?= caiok/bookkeeper:$(VERSION)
+BOOKIE ?= 1
+DOCKER_NETWORK ?= bk_network
+
+BUILD_DIR ?= $(VERSION)
+
+CONTAINER_NAME = bookkeeper-$(BOOKIE)
+DOCKER_HOSTNAME = $(shell hostname)
+BK_LOCAL_DATA_DIR = /tmp/test_bk
+BK_LOCAL_CONTAINER_DATA_DIR = $(BK_LOCAL_DATA_DIR)/$(CONTAINER_NAME)
+
+ZK_CONTAINER_NAME=test_zookeeper
+ZK_LOCAL_DATA_DIR=$(BK_LOCAL_DATA_DIR)/zookkeeper
+
+CONTAINER_IP=$(shell docker inspect --format '{{ .NetworkSettings.IPAddress 
}}' $(CONTAINER_NAME))
+
+#NOCACHE=--no-cache
+NOCACHE=
+
+#  #
+
+.PHONY: all build run create start stop shell exec root-shell root-exec info 
ip clean-files clean
+
+#  #
+
+all:
+   make info
+
+#  #
+
+# Build the bookkeeper image.
+#   make build
+
+build:
+   #-docker rmi -f $(IMAGE)
+   
+   cd $(BUILD_DIR) ; \
+   time docker build \
+   $(NOCACHE) \
+   -t $(IMAGE) .
+
+#  #
+
+# Create and run a bookkeeper container with data persisted on local 
filesystem. It needs the zookkeeper container.
+# In order to launch several bookies, the command need the bookie number
+#   make run-bk BOOKIE=4
+
+run-bk:
+   # Temporary gimmick: clear all data because of bookkeeper blocking 
check on host / data integrity
+   #-sudo rm -rf $(BK_LOCAL_CONTAINER_DATA_DIR)
+   
+   mkdir -p $(BK_LOCAL_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/journal \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/ledger \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/index
+   
+   -docker rm -f $(CONTAINER_NAME)
+   docker run -it\
+   --network $(DOCKER_NETWORK) \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/journal:/data/journal \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/ledger:/data/ledger \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/index:/data/index \
+   --name "$(CONTAINER_NAME)" \
+   --hostname "$(CONTAINER_NAME)" \
+   --env ZK_SERVERS=$(ZK_CONTAINER_NAME):2181 \
+   $(IMAGE)
+
+#  #
+
+# Create run and destroy a container that will format zookkeeper metadata
+#   make run-format
+
+run-format:
+   docker run -it --rm \
+   --network $(DOCKER_NETWORK) \
+   --env ZK_SERVERS=$(ZK_CONTAINER_NAME):2181 \
+   $(IMAGE) \
+   bookkeeper shell metaformat $(FORMAT_OPTS)
+
+#  #
+
+# Create and run the zookkeeper container needed by the ensemble
+#   make run-zk
+
+run-zk:
+   -docker network create $(DOCKER_NETWORK)
+   mkdir -pv $(BK_LOCAL_DATA_DIR) $(ZK_LOCAL_DATA_DIR) 
$(ZK_LOCAL_DATA_DIR)/data $(ZK_LOCAL_DATA_DIR)/datalog
+   -docker rm -f $(ZK_CONTAINER_NAME)
+   docker run -it --rm \
+   --network $(DOCKER_NETWORK) \
+   --name "$(ZK_CONTAINER_NAME)" \
+   --hostname "$(ZK_CONTAINER_NAME)" \
+   -v $(ZK_LOCAL_DATA_DIR)/data:/data \
+   -v $(ZK_LOCAL_DATA_DIR)/datalog:/datalog \
+   -p 2181:2181 \
+   zookeeper
+
+#  #
+
+# Create and run a container running the bookkeeper tutorial application (a 
simple dice rolling application). It's possibile
+# to run several dice applications in order to simulate a real life concurrent 
scenario.
+#   make run-dice
+run-dice:
 
 Review comment:
   #261 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r128217985
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
+BK_USER=bookkeeper \
+BK_PORT= \
+BK_BUILD_PORT=3181 \
+BOOKIE_OPTS="" \
+BK_JOURNAL_DIR=/data/journal \
 
 Review comment:
   No, env vars refer to internal container directories and I don't think it's 
very useful changing them.
   Current build already creates three different dirs, so the end user just 
have to mount the desired host dirs to these dirs. Take a look at makefile 
example (target run-bk) for seeing this in action. It's just like the common 
mounting mechanism in unix, the target system will not notice the difference.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r128216700
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
 
 Review comment:
   No, it hasn't been applied, yet. I just created #260 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r128213959
 
 

 ##
 File path: docker/4.4.0-alpine/run.sh
 ##
 @@ -0,0 +1,68 @@
+#/bin/bash
+
+# -- #
+set -x -e -u
+# -- #
+
+# -- #
+# Allow the container to be started with `--user`
+if [ "$1" = 'bookkeeper' -a "$(id -u)" = '0' ]; then
+chown -R "$BK_USER" "${BK_DIR}" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+exec su-exec "$BK_USER" /bin/bash "$0" "$@"
+exit
+fi
+# -- #
+
+# -- #
+# Copy input config files in Bookkeeper configuration directory
+cp -vaf /conf/* ${BK_DIR}/conf || true
+chown -R "$BK_USER" ${BK_DIR}/conf
+
+# Bookkeeper setup
 
 Review comment:
   Matteo's py only handles not commented out properties present in original 
files. It needs to be enhanced with handling of commented ones IMHO, in order 
to not create confusion (if the doc says "you can set BK_ for setting 
corresponding BK property" I expect this behavior, if it don't work it's not 
kind to demand the user dig in the code to figure out the reason). If we 
provide default conf files with all properties not commented out, then the 
problem arise if user mount it's own conf files.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127750910
 
 

 ##
 File path: docker/Makefile
 ##
 @@ -0,0 +1,180 @@
+
+VERSION ?= 4.4.0-alpine
+IMAGE ?= caiok/bookkeeper:$(VERSION)
+BOOKIE ?= 1
+DOCKER_NETWORK ?= bk_network
+
+BUILD_DIR ?= $(VERSION)
+
+CONTAINER_NAME = bookkeeper-$(BOOKIE)
+DOCKER_HOSTNAME = $(shell hostname)
+BK_LOCAL_DATA_DIR = /tmp/test_bk
+BK_LOCAL_CONTAINER_DATA_DIR = $(BK_LOCAL_DATA_DIR)/$(CONTAINER_NAME)
+
+ZK_CONTAINER_NAME=test_zookeeper
+ZK_LOCAL_DATA_DIR=$(BK_LOCAL_DATA_DIR)/zookkeeper
+
+CONTAINER_IP=$(shell docker inspect --format '{{ .NetworkSettings.IPAddress 
}}' $(CONTAINER_NAME))
+
+#NOCACHE=--no-cache
+NOCACHE=
+
+#  #
+
+.PHONY: all build run create start stop shell exec root-shell root-exec info 
ip clean-files clean
+
+#  #
+
+all:
+   make info
+
+#  #
+
+# Build the bookkeeper image.
+#   make build
+
+build:
+   #-docker rmi -f $(IMAGE)
+   
+   cd $(BUILD_DIR) ; \
+   time docker build \
+   $(NOCACHE) \
+   -t $(IMAGE) .
+
+#  #
+
+# Create and run a bookkeeper container with data persisted on local 
filesystem. It needs the zookkeeper container.
+# In order to launch several bookies, the command need the bookie number
+#   make run-bk BOOKIE=4
+
+run-bk:
+   # Temporary gimmick: clear all data because of bookkeeper blocking 
check on host / data integrity
+   #-sudo rm -rf $(BK_LOCAL_CONTAINER_DATA_DIR)
+   
+   mkdir -p $(BK_LOCAL_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/journal \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/ledger \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/index
+   
+   -docker rm -f $(CONTAINER_NAME)
+   docker run -it\
+   --network $(DOCKER_NETWORK) \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/journal:/data/journal \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/ledger:/data/ledger \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/index:/data/index \
+   --name "$(CONTAINER_NAME)" \
+   --hostname "$(CONTAINER_NAME)" \
+   --env ZK_SERVERS=$(ZK_CONTAINER_NAME):2181 \
+   $(IMAGE)
+
+#  #
+
+# Create run and destroy a container that will format zookkeeper metadata
+#   make run-format
+
+run-format:
+   docker run -it --rm \
+   --network $(DOCKER_NETWORK) \
+   --env ZK_SERVERS=$(ZK_CONTAINER_NAME):2181 \
+   $(IMAGE) \
+   bookkeeper shell metaformat $(FORMAT_OPTS)
+
+#  #
+
+# Create and run the zookkeeper container needed by the ensemble
+#   make run-zk
+
+run-zk:
+   -docker network create $(DOCKER_NETWORK)
+   mkdir -pv $(BK_LOCAL_DATA_DIR) $(ZK_LOCAL_DATA_DIR) 
$(ZK_LOCAL_DATA_DIR)/data $(ZK_LOCAL_DATA_DIR)/datalog
+   -docker rm -f $(ZK_CONTAINER_NAME)
+   docker run -it --rm \
+   --network $(DOCKER_NETWORK) \
+   --name "$(ZK_CONTAINER_NAME)" \
+   --hostname "$(ZK_CONTAINER_NAME)" \
+   -v $(ZK_LOCAL_DATA_DIR)/data:/data \
+   -v $(ZK_LOCAL_DATA_DIR)/datalog:/datalog \
+   -p 2181:2181 \
+   zookeeper
+
+#  #
+
+# Create and run a container running the bookkeeper tutorial application (a 
simple dice rolling application). It's possibile
+# to run several dice applications in order to simulate a real life concurrent 
scenario.
+#   make run-dice
+run-dice:
 
 Review comment:
   Good point, docker-compose was my first-choice, but it lacks a way to start 
containers in a prefixed order, and this made it impossible to execute 
metaformat before running bk containers. With last additions to bk image, 
docker-compose could be used to run zk, the ensemble and tutorials instance.
   
   BTW, I've done some changes in order to use the tutorial code in its own 
docker image. They can be found here: 
https://github.com/caiok/bookkeeper-tutorial
   
   Maybe they could be merged before adding the code to bk, but some cleanup 
it's needed. I could do this if there is consensus about bringing this code in 
bk.
   
   Even adding a docker-compose example could be much useful then the Makefile 
one.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127412474
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
+BK_USER=bookkeeper \
+BK_PORT= \
 
 Review comment:
   @sijie @jiazhai  I remembered the reason behind this implementation. We 
provide to docker user the ability to mount configurations file at run time. 
These files will be rewritten with the env variables set at run. If we do 
provide a default for the port, in run.sh we have no way to determine if was 
passed by the user or it's the default, so it will be rewritten in each case, 
overwriting the port specified by the user in conf file. I will restore the 
BK_PORT=[blank] and BK_BUILD_PORT
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127411473
 
 

 ##
 File path: docker/4.4.0-alpine/run.sh
 ##
 @@ -0,0 +1,68 @@
+#/bin/bash
+
+# -- #
+set -x -e -u
+# -- #
+
+# -- #
+# Allow the container to be started with `--user`
+if [ "$1" = 'bookkeeper' -a "$(id -u)" = '0' ]; then
+chown -R "$BK_USER" "${BK_DIR}" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+exec su-exec "$BK_USER" /bin/bash "$0" "$@"
+exit
+fi
+# -- #
+
+# -- #
+# Copy input config files in Bookkeeper configuration directory
+cp -vaf /conf/* ${BK_DIR}/conf || true
+chown -R "$BK_USER" ${BK_DIR}/conf
+
+# Bookkeeper setup
 
 Review comment:
   Is the first step really needed or we could use the default bk_server.conf 
and then in run phase use found BK_xxx env variables for substitute confs in 
it? Furthermore, the first step will brake the possibility to mount an already 
set configuration file (it would be entirely rewritten). 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127402908
 
 

 ##
 File path: docker/4.4.0-alpine/run.sh
 ##
 @@ -0,0 +1,68 @@
+#/bin/bash
+
+# -- #
+set -x -e -u
+# -- #
+
+# -- #
+# Allow the container to be started with `--user`
+if [ "$1" = 'bookkeeper' -a "$(id -u)" = '0' ]; then
+chown -R "$BK_USER" "${BK_DIR}" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+exec su-exec "$BK_USER" /bin/bash "$0" "$@"
+exit
+fi
+# -- #
+
+# -- #
+# Copy input config files in Bookkeeper configuration directory
+cp -vaf /conf/* ${BK_DIR}/conf || true
+chown -R "$BK_USER" ${BK_DIR}/conf
+
+# Bookkeeper setup
+sed -r -i.bak \
+   -e "s|^journalDirectory.*=.*|journalDirectory=${BK_JOURNAL_DIR}|" \
+   -e "s|^ledgerDirectories.*=.*|ledgerDirectories=${BK_LEDGER_DIR}|" \
+   -e "s|^[# ]*indexDirectories.*=.*|indexDirectories=${BK_INDEX_DIR}|" \
+   -e "s|^[# 
]*useHostNameAsBookieID.*=.*false|useHostNameAsBookieID=true|" \
+   ${BK_DIR}/conf/bk_server.conf
+
+if [[ "${ZK_SERVERS}" != "" ]]; then
+   sed -r -i "s|^zkServers.*=.*|zkServers=${ZK_SERVERS}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+if [[ "${BK_PORT}" != "" ]]; then
+   sed -r -i "s|^bookiePort.*=.*|bookiePort=${BK_PORT}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+if [[ "${BK_LEDGERS_PATH}" != "" ]]; then
+   sed -r -i "s|^[# 
]*zkLedgersRootPath.*=.*|zkLedgersRootPath=${BK_LEDGERS_PATH}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+
+diff ${BK_DIR}/conf/bk_server.conf.bak ${BK_DIR}/conf/bk_server.conf || true
+# -- #
+
+# -- #
+# Wait for zookeeper server
+#set +x
+#zk_server1=$(echo ${ZK_SERVERS} | cut -d"," -f1)
+#zk_server1_host=$(echo ${zk_server1} | cut -d":" -f1)
+#zk_server1_port=$(echo ${zk_server1} | cut -d":" -f2)
+
+#echo -en "\nWaiting for Zookeeper (${zk_server1_host}:${zk_server1_port})..."
+#while [[ $(nc -z ${zk_server1_host} ${zk_server1_port}) -ne 0 ]] ; do
+#  echo -n "."
+#  sleep 2
+#done
+#echo " Connected!"
+#set -x
+# -- #
+
+
+# -- #
+# Trying to format bookkeeper dir on zookeeper. If the dir already exists, it 
does nothing.
 
 Review comment:
   I've messed up someway this line before open the PR, the correct one is 
obviously "exec bookkeeper shell metaformat -nonInteractive || true"
   
   @sijie This command should ignore format if it find zk dir structure already 
formatted, right? What kind of issues can it arise? (just my curiosity)
   @jiazhai I will do metaformat only if it's passed a non empty env var 
BK_TRY_METAFORMAT, it should satisfy both requirements.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127387138
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
 
 Review comment:
   I think this script doesn't handle the commented out option lines, like the 
majority of options in 
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/conf/bk_server.conf
 . It's a good idea anyway, and I'm fine to execute this script before the 
current substitutions done in run.sh. Maybe in a second step with a version 
that handles even commented out options?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127384452
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
 
 Review comment:
   Yes it is. I use to put placeholders and then have a makefile that generates 
the real dockerfile. It could be a good be a good idea, even for the bookkeeper 
version (I don't know how much bk changes between releases, but I suppose that 
the dockerfile, run.sh and healthcheck.sh will remain the same).
   
   I'm not sure if to introduce this change now or in a second step. @jiazhai 
@fpj @merlimat Any preferences / ideas?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127252807
 
 

 ##
 File path: docker/Makefile
 ##
 @@ -0,0 +1,180 @@
+
+VERSION ?= 4.4.0-alpine
+IMAGE ?= caiok/bookkeeper:$(VERSION)
+BOOKIE ?= 1
+DOCKER_NETWORK ?= bk_network
+
+BUILD_DIR ?= $(VERSION)
+
+CONTAINER_NAME = bookkeeper-$(BOOKIE)
+DOCKER_HOSTNAME = $(shell hostname)
+BK_LOCAL_DATA_DIR = /tmp/test_bk
+BK_LOCAL_CONTAINER_DATA_DIR = $(BK_LOCAL_DATA_DIR)/$(CONTAINER_NAME)
+
+ZK_CONTAINER_NAME=test_zookeeper
+ZK_LOCAL_DATA_DIR=$(BK_LOCAL_DATA_DIR)/zookkeeper
+
+CONTAINER_IP=$(shell docker inspect --format '{{ .NetworkSettings.IPAddress 
}}' $(CONTAINER_NAME))
+
+#NOCACHE=--no-cache
+NOCACHE=
+
+#  #
+
+.PHONY: all build run create start stop shell exec root-shell root-exec info 
ip clean-files clean
+
+#  #
+
+all:
+   make info
+
+#  #
+
+# Build the bookkeeper image.
+#   make build
+
+build:
+   #-docker rmi -f $(IMAGE)
+   
+   cd $(BUILD_DIR) ; \
+   time docker build \
+   $(NOCACHE) \
+   -t $(IMAGE) .
+
+#  #
+
+# Create and run a bookkeeper container with data persisted on local 
filesystem. It needs the zookkeeper container.
+# In order to launch several bookies, the command need the bookie number
+#   make run-bk BOOKIE=4
+
+run-bk:
+   # Temporary gimmick: clear all data because of bookkeeper blocking 
check on host / data integrity
+   #-sudo rm -rf $(BK_LOCAL_CONTAINER_DATA_DIR)
+   
+   mkdir -p $(BK_LOCAL_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/journal \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/ledger \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/index
+   
+   -docker rm -f $(CONTAINER_NAME)
+   docker run -it\
+   --network $(DOCKER_NETWORK) \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/journal:/data/journal \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/ledger:/data/ledger \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/index:/data/index \
+   --name "$(CONTAINER_NAME)" \
+   --hostname "$(CONTAINER_NAME)" \
+   --env ZK_SERVERS=$(ZK_CONTAINER_NAME):2181 \
+   $(IMAGE)
+
+#  #
+
+# Create run and destroy a container that will format zookkeeper metadata
+#   make run-format
+
+run-format:
+   docker run -it --rm \
+   --network $(DOCKER_NETWORK) \
+   --env ZK_SERVERS=$(ZK_CONTAINER_NAME):2181 \
+   $(IMAGE) \
+   bookkeeper shell metaformat $(FORMAT_OPTS)
+
+#  #
+
+# Create and run the zookkeeper container needed by the ensemble
+#   make run-zk
+
+run-zk:
+   -docker network create $(DOCKER_NETWORK)
+   mkdir -pv $(BK_LOCAL_DATA_DIR) $(ZK_LOCAL_DATA_DIR) 
$(ZK_LOCAL_DATA_DIR)/data $(ZK_LOCAL_DATA_DIR)/datalog
+   -docker rm -f $(ZK_CONTAINER_NAME)
+   docker run -it --rm \
+   --network $(DOCKER_NETWORK) \
+   --name "$(ZK_CONTAINER_NAME)" \
+   --hostname "$(ZK_CONTAINER_NAME)" \
+   -v $(ZK_LOCAL_DATA_DIR)/data:/data \
+   -v $(ZK_LOCAL_DATA_DIR)/datalog:/datalog \
+   -p 2181:2181 \
+   zookeeper
+
+#  #
+
+# Create and run a container running the bookkeeper tutorial application (a 
simple dice rolling application). It's possibile
+# to run several dice applications in order to simulate a real life concurrent 
scenario.
+#   make run-dice
+run-dice:
 
 Review comment:
   Are you saying to bring the tutorial code in bk repo and have a docker build 
for that image? I think it could be a good idea (so the code will be updated 
with releases), but it need a general consensus. @sijie @jvrao @jiazhai 
@eolivelli what do you think about it?
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127252406
 
 

 ##
 File path: docker/Makefile
 ##
 @@ -0,0 +1,180 @@
+
+VERSION ?= 4.4.0-alpine
+IMAGE ?= caiok/bookkeeper:$(VERSION)
+BOOKIE ?= 1
+DOCKER_NETWORK ?= bk_network
+
+BUILD_DIR ?= $(VERSION)
+
+CONTAINER_NAME = bookkeeper-$(BOOKIE)
+DOCKER_HOSTNAME = $(shell hostname)
+BK_LOCAL_DATA_DIR = /tmp/test_bk
+BK_LOCAL_CONTAINER_DATA_DIR = $(BK_LOCAL_DATA_DIR)/$(CONTAINER_NAME)
+
+ZK_CONTAINER_NAME=test_zookeeper
+ZK_LOCAL_DATA_DIR=$(BK_LOCAL_DATA_DIR)/zookkeeper
+
+CONTAINER_IP=$(shell docker inspect --format '{{ .NetworkSettings.IPAddress 
}}' $(CONTAINER_NAME))
+
+#NOCACHE=--no-cache
+NOCACHE=
+
+#  #
+
+.PHONY: all build run create start stop shell exec root-shell root-exec info 
ip clean-files clean
+
+#  #
+
+all:
+   make info
+
+#  #
+
+# Build the bookkeeper image.
+#   make build
+
+build:
+   #-docker rmi -f $(IMAGE)
+   
+   cd $(BUILD_DIR) ; \
+   time docker build \
+   $(NOCACHE) \
+   -t $(IMAGE) .
+
+#  #
+
+# Create and run a bookkeeper container with data persisted on local 
filesystem. It needs the zookkeeper container.
+# In order to launch several bookies, the command need the bookie number
+#   make run-bk BOOKIE=4
+
+run-bk:
+   # Temporary gimmick: clear all data because of bookkeeper blocking 
check on host / data integrity
+   #-sudo rm -rf $(BK_LOCAL_CONTAINER_DATA_DIR)
+   
+   mkdir -p $(BK_LOCAL_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR) \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/journal \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/ledger \
+   $(BK_LOCAL_CONTAINER_DATA_DIR)/index
+   
+   -docker rm -f $(CONTAINER_NAME)
+   docker run -it\
+   --network $(DOCKER_NETWORK) \
+   --volume $(BK_LOCAL_CONTAINER_DATA_DIR)/journal:/data/journal \
 
 Review comment:
   You have to manually mount them. E.g:
   --volume /real/path/1:/data/journal/journal-1
   --volume /real/path/2:/data/journal/journal-2
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127249570
 
 

 ##
 File path: docker/4.4.0-alpine/healthcheck.sh
 ##
 @@ -0,0 +1,6 @@
+#!/bin/bash
+
+set -x -e -u
+
+# Simple healtcheck on BK port
+nc -z localhost ${BK_PORT}
 
 Review comment:
   Interesting suggestion. Doing it every 3 seconds doesn't impact 
performances, right?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127248747
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
+BK_USER=bookkeeper \
+BK_PORT= \
+BK_BUILD_PORT=3181 \
+BOOKIE_OPTS="" \
+BK_JOURNAL_DIR=/data/journal \
+BK_LEDGER_DIR=/data/ledger \
+BK_INDEX_DIR=/data/index \
+BK_LEDGERS_PATH=
+
+# Add a user and make dirs
+RUN set -x \
+&& adduser -D "${BK_USER}" \
+&& mkdir -p "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" "${BK_INDEX_DIR}" \
+&& chown "$BK_USER:$BK_USER" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+
+ARG GPG_KEY=D0BC8D8A4E90A40AFDFC43B3E22A746A68E327C1
+ARG BK_VERSION=4.4.0
+
+ENV DISTRO_NAME=bookkeeper-server-${BK_VERSION}-bin
+   
+# Download Apache Bookkeeper, verify its PGP signature, untar and clean up
+RUN set -x \
+&& apk add --no-cache --virtual .build-deps \
+gnupg \
+wget \
+&& mkdir -pv /opt \
+&& cd /opt \
+&& wget -q 
"https://archive.apache.org/dist/bookkeeper/bookkeeper-${BK_VERSION}/${DISTRO_NAME}.tar.gz;
 \
+&& wget -q 
"https://archive.apache.org/dist/bookkeeper/bookkeeper-${BK_VERSION}/${DISTRO_NAME}.tar.gz.asc;
 \
+&& export GNUPGHOME="$(mktemp -d)" \
+#&& gpg --keyserver ha.pool.sks-keyservers.net --recv-key "$GPG_KEY" \
+#&& gpg --batch --verify "$DISTRO_NAME.tar.gz.asc" "$DISTRO_NAME.tar.gz" \
+&& tar -xzf "$DISTRO_NAME.tar.gz" \
+&& rm -r "$GNUPGHOME" "$DISTRO_NAME.tar.gz" "$DISTRO_NAME.tar.gz.asc" \
+&& apk del .build-deps
+
+ENV BK_DIR=/opt/bookkeeper-server-${BK_VERSION}
 
 Review comment:
   I agree
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127248048
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
+BK_USER=bookkeeper \
+BK_PORT= \
+BK_BUILD_PORT=3181 \
+BOOKIE_OPTS="" \
+BK_JOURNAL_DIR=/data/journal \
+BK_LEDGER_DIR=/data/ledger \
+BK_INDEX_DIR=/data/index \
+BK_LEDGERS_PATH=
+
+# Add a user and make dirs
+RUN set -x \
+&& adduser -D "${BK_USER}" \
+&& mkdir -p "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" "${BK_INDEX_DIR}" \
+&& chown "$BK_USER:$BK_USER" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+
+ARG GPG_KEY=D0BC8D8A4E90A40AFDFC43B3E22A746A68E327C1
+ARG BK_VERSION=4.4.0
+
+ENV DISTRO_NAME=bookkeeper-server-${BK_VERSION}-bin
+   
+# Download Apache Bookkeeper, verify its PGP signature, untar and clean up
+RUN set -x \
+&& apk add --no-cache --virtual .build-deps \
+gnupg \
+wget \
+&& mkdir -pv /opt \
+&& cd /opt \
+&& wget -q 
"https://archive.apache.org/dist/bookkeeper/bookkeeper-${BK_VERSION}/${DISTRO_NAME}.tar.gz;
 \
 
 Review comment:
   The docker build process will be executed from docker hub (it will retrieve 
dockerfile and than make the build every night or so). For creating an image 
for the the latest commit (the "on build" image, like it's usually named) we 
need to publish that jars anyway.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127238474
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
+BK_USER=bookkeeper \
+BK_PORT= \
 
 Review comment:
   Blank is the default so it let the run.sh script to know that the env 
variable has not been set by "docker run" and let the config file original 
value. I'm fine anyway in providing a default here with documentation purpose.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127236866
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
 
 Review comment:
   I have usually seen physical persons in MAINTAINER command, but I'm fine 
with your proposal 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127232920
 
 

 ##
 File path: docker/Makefile
 ##
 @@ -0,0 +1,180 @@
+
+VERSION ?= 4.4.0-alpine
+IMAGE ?= caiok/bookkeeper:$(VERSION)
+BOOKIE ?= 1
+DOCKER_NETWORK ?= bk_network
+
+BUILD_DIR ?= $(VERSION)
+
+CONTAINER_NAME = bookkeeper-$(BOOKIE)
+DOCKER_HOSTNAME = $(shell hostname)
+BK_LOCAL_DATA_DIR = /tmp/test_bk
+BK_LOCAL_CONTAINER_DATA_DIR = $(BK_LOCAL_DATA_DIR)/$(CONTAINER_NAME)
+
+ZK_CONTAINER_NAME=test_zookeeper
+ZK_LOCAL_DATA_DIR=$(BK_LOCAL_DATA_DIR)/zookkeeper
+
+CONTAINER_IP=$(shell docker inspect --format '{{ .NetworkSettings.IPAddress 
}}' $(CONTAINER_NAME))
+
+#NOCACHE=--no-cache
+NOCACHE=
+
+#  #
+
+.PHONY: all build run create start stop shell exec root-shell root-exec info 
ip clean-files clean
+
+#  #
+
+all:
+   make info
+
+#  #
+
+# Build the bookkeeper image.
+#   make build
+
+build:
+   #-docker rmi -f $(IMAGE)
+   
+   cd $(BUILD_DIR) ; \
+   time docker build \
+   $(NOCACHE) \
+   -t $(IMAGE) .
+
+#  #
+
+# Create and run a bookkeeper container with data persisted on local 
filesystem. It needs the zookkeeper container.
+# In order to launch several bookies, the command need the bookie number
+#   make run-bk BOOKIE=4
+
+run-bk:
+   # Temporary gimmick: clear all data because of bookkeeper blocking 
check on host / data integrity
+   #-sudo rm -rf $(BK_LOCAL_CONTAINER_DATA_DIR)
 
 Review comment:
   Yes
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127232485
 
 

 ##
 File path: docker/4.4.0-alpine/run.sh
 ##
 @@ -0,0 +1,68 @@
+#/bin/bash
+
+# -- #
+set -x -e -u
+# -- #
+
+# -- #
+# Allow the container to be started with `--user`
+if [ "$1" = 'bookkeeper' -a "$(id -u)" = '0' ]; then
+chown -R "$BK_USER" "${BK_DIR}" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+exec su-exec "$BK_USER" /bin/bash "$0" "$@"
+exit
+fi
+# -- #
+
+# -- #
+# Copy input config files in Bookkeeper configuration directory
+cp -vaf /conf/* ${BK_DIR}/conf || true
+chown -R "$BK_USER" ${BK_DIR}/conf
+
+# Bookkeeper setup
+sed -r -i.bak \
+   -e "s|^journalDirectory.*=.*|journalDirectory=${BK_JOURNAL_DIR}|" \
+   -e "s|^ledgerDirectories.*=.*|ledgerDirectories=${BK_LEDGER_DIR}|" \
+   -e "s|^[# ]*indexDirectories.*=.*|indexDirectories=${BK_INDEX_DIR}|" \
+   -e "s|^[# 
]*useHostNameAsBookieID.*=.*false|useHostNameAsBookieID=true|" \
+   ${BK_DIR}/conf/bk_server.conf
+
+if [[ "${ZK_SERVERS}" != "" ]]; then
+   sed -r -i "s|^zkServers.*=.*|zkServers=${ZK_SERVERS}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+if [[ "${BK_PORT}" != "" ]]; then
+   sed -r -i "s|^bookiePort.*=.*|bookiePort=${BK_PORT}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+if [[ "${BK_LEDGERS_PATH}" != "" ]]; then
+   sed -r -i "s|^[# 
]*zkLedgersRootPath.*=.*|zkLedgersRootPath=${BK_LEDGERS_PATH}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+
+diff ${BK_DIR}/conf/bk_server.conf.bak ${BK_DIR}/conf/bk_server.conf || true
+# -- #
+
+# -- #
+# Wait for zookeeper server
+#set +x
 
 Review comment:
   I have un-commented them, it could be useful to wait for zk before start 
bookkeeper. Maybe can be enhanced with a timeout.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services