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

owncloud.d: don't wait for clustered redis #95

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@galexrt
Copy link
Contributor

commented Aug 4, 2019

Don't wait-for-it for clustered redis.

@galexrt galexrt force-pushed the galexrt:fix_redis_waitforit branch from f2f5603 to a644e79 Aug 4, 2019

owncloud.d: don't wait for clustered redis
Signed-off-by: Alexander Trost <galexrt@googlemail.com>

@galexrt galexrt force-pushed the galexrt:fix_redis_waitforit branch from a644e79 to 91b3d60 Aug 4, 2019

@tboerger tboerger requested a review from patrickjahns Aug 5, 2019

@patrickjahns

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@galexrt - would it make sense to wait for a single node of the cluster to be up ?

@galexrt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@patrickjahns Sure, though then I would say let's test all given addresses with a timeout of 30 seconds each and continue as long as one address is "responding".
Does that sound good to you?

@patrickjahns

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Sounds perfect - just wondering if its "worth the effort"

@galexrt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@patrickjahns Well there is a reason the PR right now just disables it. ;-)

Testing just one of the given Redis addresses isn't "enough" as it might lead to a "false positive" startup abort because one of the, e.g., 3 Redis endpoints of the Redis cluster is down.
Testing could be done in parallel, but it wouldn't be worth it.

@patrickjahns
Copy link
Member

left a comment

LGTM

@patrickjahns

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Then lets continue with disabling for now

@patrickjahns patrickjahns merged commit 68e3b40 into owncloud-docker:master Aug 6, 2019

1 check passed

continuous-integration/drone/pr the build was successful
Details
@patrickjahns

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Can you port it also to bionic+xenial branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.