-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Allow to skip loading unused plugins in Docker container #12486
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,39 @@ | |
|
||
set -xeuo pipefail | ||
|
||
set +e | ||
grep -s -q 'node.id' /etc/trino/node.properties | ||
NODE_ID_EXISTS=$? | ||
set -e | ||
|
||
NODE_ID="" | ||
if [[ ${NODE_ID_EXISTS} != 0 ]] ; then | ||
if ! grep -s -q 'node.id' /etc/trino/node.properties; then | ||
nineinchnick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
NODE_ID="-Dnode.id=${HOSTNAME}" | ||
fi | ||
|
||
exec /usr/lib/trino/bin/launcher run --etc-dir /etc/trino ${NODE_ID} "$@" | ||
base=/usr/lib/trino | ||
if [ -n "${TRINO_ENABLE_PLUGINS:-}" ] && [ -n "${TRINO_DISABLE_PLUGINS:-}" ]; then | ||
echo >&2 "Cannot set both \$TRINO_DISABLE_PLUGINS and \$TRINO_ENABLE_PLUGINS" | ||
exit 1 | ||
elif [ -n "${TRINO_ENABLE_PLUGINS:-}" ]; then | ||
if [ -n "${TRINO_DISABLE_PLUGINS:-}" ]; then | ||
echo >&2 "Cannot set both \$TRINO_DISABLE_PLUGINS and \$TRINO_ENABLE_PLUGINS" | ||
exit 1 | ||
fi | ||
mv "$base/plugin" "$base/plugin.disabled" | ||
mv "/etc/trino/catalog" "/etc/trino/catalog.disabled" | ||
mkdir -p "$base/plugin" /etc/trino/catalog | ||
IFS=, read -ra names <<< "$TRINO_ENABLE_PLUGINS" | ||
for name in "${names[@]}"; do | ||
mv "$base/plugin.disabled/$name" "$base/plugin/$name" | ||
if [ -f "/etc/trino/catalog.disabled/$name.properties" ]; then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. plugin name != catalog name did you mean TRINO_ENABLED_CATALOGS env var? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, you're right. I added this because there are default catalogs that will cause the server to fail at startup if the connectors/plugins they use are disabled. I want to keep this simple so I'd like to avoid adding even more env vars. Would grepping for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's another reason to only do this at build time - to avoid adding new logic to parse catalog configs outside of Java. |
||
mv "/etc/trino/catalog.disabled/$name.properties" "/etc/trino/catalog/$name.properties" | ||
fi | ||
done | ||
elif [ -n "${TRINO_DISABLE_PLUGINS:-}" ]; then | ||
mkdir -p "$base/plugin.disabled" /etc/trino/catalog.disabled | ||
IFS=, read -ra names <<< "$TRINO_DISABLE_PLUGINS" | ||
for name in "${names[@]}"; do | ||
mv "$base/plugin/$name" "$base/plugin.disabled/$name" | ||
if [ -f "/etc/trino/catalog/$name.properties" ]; then | ||
mv "/etc/trino/catalog/$name.properties" "/etc/trino/catalog.disabled/$name.properties" | ||
fi | ||
done | ||
fi | ||
|
||
exec /usr/lib/trino/bin/launcher run --etc-dir /etc/trino "${NODE_ID}" "$@" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,10 +13,8 @@ function test_trino_starts { | |
CONTAINER_ID= | ||
trap cleanup EXIT | ||
|
||
local CONTAINER_NAME=$1 | ||
local PLATFORM=$2 | ||
# We aren't passing --rm here to make sure container is available for inspection in case of failures | ||
CONTAINER_ID=$(docker run -d --platform "${PLATFORM}" "${CONTAINER_NAME}") | ||
CONTAINER_ID=$(docker run -d "$@") | ||
|
||
set +e | ||
I=0 | ||
|
@@ -41,6 +39,15 @@ function test_trino_starts { | |
[[ ${RESULT} == '"success"' ]] | ||
} | ||
|
||
function test_trino_fails { | ||
if ! timeout 10 docker run --rm "$@"; then | ||
if [ $? == 124 ]; then | ||
echo >&2 "Command expected to fail but did not: docker run --rm $*" | ||
exit 1 | ||
fi | ||
fi | ||
} | ||
|
||
function test_javahome { | ||
local CONTAINER_NAME=$1 | ||
local PLATFORM=$2 | ||
|
@@ -56,6 +63,8 @@ function test_container { | |
local PLATFORM=$2 | ||
echo "🐢 Validating ${CONTAINER_NAME} on platform ${PLATFORM}..." | ||
test_javahome "${CONTAINER_NAME}" "${PLATFORM}" | ||
test_trino_starts "${CONTAINER_NAME}" "${PLATFORM}" | ||
test_trino_starts -e TRINO_ENABLE_PLUGINS=jmx,memory --platform "${PLATFORM}" "${CONTAINER_NAME}" | ||
test_trino_starts -e TRINO_DISABLE_PLUGINS=tpch,tpcds --platform "${PLATFORM}" "${CONTAINER_NAME}" | ||
test_trino_fails -e TRINO_ENABLE_PLUGINS=jmx,memory -e TRINO_DISABLE_PLUGINS=tpch,tpcds "${CONTAINER_NAME}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test_trino_fails should assert on the "exception message" |
||
echo "🎉 Validated ${CONTAINER_NAME} on platform ${PLATFORM}" | ||
} |
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.
unrelated