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

Make ClickhouseRepo task timeouts configurable in CE #4494

Merged
merged 13 commits into from
Sep 9, 2024

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Aug 30, 2024

Changes

This PR makes CE's ClickhouseRepo tasks reuse timeouts defined on CLICKHOUSE_DATABASE_URL

For example, setting CLICKHOUSE_DATABASE_URL=http://plausible_events_db:8123/plausible_events_db?timeout=120000 would make the task timeout value equal eight minutes (480000=url_timeout*4). It has already been used to solve some timeout issues before: #3311 - so it makes sense to standardize on this approach rather than introduce extra ENV vars.

Related issues and discussions, where increasing the timeout value for tasks might help:

Some examples for how the timeout param in URL affects Repo config:

# for export CLICKHOUSE_DATABASE_URL=http://localhost:8123/plausible_events_db
iex> Keyword.fetch!(Plausible.ClickhouseRepo.config(), :timeout)
#==> 15000 <- this is the default from db_connection

# for export CLICKHOUSE_DATABASE_URL="http://localhost:8123/plausible_events_db?timeout=120000"
iex> Keyword.fetch!(Plausible.ClickhouseRepo.config(), :timeout)
#==> 120000 <- this would make tasks timeout after 120000*4=480000 or eight minutes

Tests

Changelog

  • Does not need a new entry, I'll just reply in the discussions above that the task timeouts can be configured in CLICKHOUSE_DATABASE_URL

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@ruslandoga ruslandoga changed the title remove ch timeouts from ce Remove ClickHouse timeouts from CE Aug 30, 2024
@ruslandoga ruslandoga mentioned this pull request Aug 30, 2024
4 tasks
@ruslandoga ruslandoga added the self-hosting Anything self-hosted label Aug 30, 2024
@ruslandoga ruslandoga changed the title Remove ClickHouse timeouts from CE Make ClickHouse timeouts configurable in CE Sep 2, 2024
@ruslandoga ruslandoga force-pushed the infinite-timeout-in-ce branch from 50c54d4 to 034fe8c Compare September 2, 2024 09:39
@ruslandoga ruslandoga changed the title Make ClickHouse timeouts configurable in CE Make ClickhouseRepo task timeouts configurable in CE Sep 2, 2024
@ruslandoga ruslandoga force-pushed the infinite-timeout-in-ce branch from 034fe8c to eec47cb Compare September 2, 2024 09:46
@task_timeout
else
ch_timeout = Keyword.fetch!(config(), :timeout)
max(ch_timeout * 2, @task_timeout)
Copy link
Contributor Author

@ruslandoga ruslandoga Sep 2, 2024

Choose a reason for hiding this comment

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

I'm doubling the timeout value from the repo just to make sure we don't timeout the task before db_connection does. It could be max(ch_timeout + 1, @task_timeout) but I thought that * 2 is safer since maybe some tasks do more than one DB request. Maybe it should be * 4 since this seems to be the default relation, (@task_timeout / default_db_connection_timeout = 60_000 / 15_000 = 4)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's adjust it to 4 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruslandoga ruslandoga marked this pull request as ready for review September 2, 2024 10:13
@ruslandoga ruslandoga requested a review from a team September 2, 2024 10:14
@zoldar
Copy link
Contributor

zoldar commented Sep 5, 2024

Not sure how (or if) to test this

@ruslandoga Perhaps starting a repo process without name and using https://hexdocs.pm/ecto/Ecto.Repo.html#c:put_dynamic_repo/1 would enable testing it (the original repo pid/name would have to be put back at the end of the test though).

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Sep 7, 2024

I tried adding the test:

And now I think it is too complicated for what it is testing...
So I'm going to remove it once CI is green for the current iteration :) Done: ce960da

@ruslandoga ruslandoga force-pushed the infinite-timeout-in-ce branch from ca02738 to d32160d Compare September 7, 2024 11:06
@zoldar zoldar merged commit 9ba9e2a into master Sep 9, 2024
10 checks passed
@zoldar zoldar deleted the infinite-timeout-in-ce branch September 9, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
self-hosting Anything self-hosted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants