Skip to content
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 dbindex to redis.config.php #2236

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Hujgo
Copy link

@Hujgo Hujgo commented Jun 14, 2024

add dbindex for redis as comment. referenced from a6f2a3c commit.

add dbindex for redis as comment. referenced from a6f2a3c commit

Signed-off-by: Arjun Santhosh <[email protected]>
@joshtrichards
Copy link
Member

That's not a comment. :-)

Also, this would have to be handled in the PHP session handler too otherwise the result might not be what people expect...

docker/docker-entrypoint.sh

Lines 108 to 133 in 064069b

if [ -n "${REDIS_HOST+x}" ]; then
echo "Configuring Redis as session handler"
{
file_env REDIS_HOST_PASSWORD
echo 'session.save_handler = redis'
# check if redis host is an unix socket path
if [ "$(echo "$REDIS_HOST" | cut -c1-1)" = "/" ]; then
if [ -n "${REDIS_HOST_PASSWORD+x}" ]; then
echo "session.save_path = \"unix://${REDIS_HOST}?auth=${REDIS_HOST_PASSWORD}\""
else
echo "session.save_path = \"unix://${REDIS_HOST}\""
fi
# check if redis password has been set
elif [ -n "${REDIS_HOST_PASSWORD+x}" ]; then
echo "session.save_path = \"tcp://${REDIS_HOST}:${REDIS_HOST_PORT:=6379}?auth=${REDIS_HOST_PASSWORD}\""
else
echo "session.save_path = \"tcp://${REDIS_HOST}:${REDIS_HOST_PORT:=6379}\""
fi
echo "redis.session.locking_enabled = 1"
echo "redis.session.lock_retries = -1"
# redis.session.lock_wait_time is specified in microseconds.
# Wait 10ms before retrying the lock rather than the default 2ms.
echo "redis.session.lock_wait_time = 10000"
} > /usr/local/etc/php/conf.d/redis-session.ini
fi

@joshtrichards joshtrichards changed the title Update redis.config.php Add dbindex to redis.config.php Jun 16, 2024
@@ -6,6 +6,7 @@
'redis' => array(
'host' => getenv('REDIS_HOST'),
'password' => getenv('REDIS_HOST_PASSWORD_FILE') && file_exists(getenv('REDIS_HOST_PASSWORD_FILE')) ? trim(file_get_contents(getenv('REDIS_HOST_PASSWORD_FILE'))) : (string) getenv('REDIS_HOST_PASSWORD'),
'dbindex' => (int) getenv('REDIS_DB_INDEX'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'dbindex' => (int) getenv('REDIS_DB_INDEX'),
// 'dbindex' => (int) getenv('REDIS_DB_INDEX'),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having this in the autoconfig but commented out is pretty useless IMHO. Why did they add it like this in AIO?

@J0WI
Copy link
Contributor

J0WI commented Jun 18, 2024

kind of duplictae of #1286

Can you elaborate the use case?

@GroondCharge
Copy link

Can you elaborate the use case?

Having an external redis master-master cluster and deploying nextcloud only on designated node. I am aware that prefix hash is calculated by concatenating instanceid, path and version, so in theory there should be no conflicting prefixes, however being able to separate it in separate dbindices in case of needing to cleanup after update etc. is a bonus, especially if you're managing mutliple instances connecting to single cluster. One of the solutions would be having option to use consistent prefix for redis entries.

@GroondCharge
Copy link

If this will get pulled into main, has the session lock configuration been adjusted aswell? I've tried an example here https://github.com/nextcloud/docker/pull/2288/files

@joshtrichards
Copy link
Member

Having an external redis master-master cluster and deploying nextcloud only on designated node

But Redis Cluster doesn't support dbindex / multiple databases.

https://redis.io/docs/latest/commands/select/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants