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

Allow stopping api server when watch command is stopped #922

Merged

Conversation

tadus21
Copy link
Contributor

@tadus21 tadus21 commented May 13, 2024

Hi, with the current --watch implementation on the API server, if error occurs in the watch goroutine, process just stops leaving API server running without performing any work and you need to manually restart the server or start watch command by the API call in order to have it running again.

I think it would make sense to stop API server if watch command failed so I am proposing a change that allows running watch command as a main API process and once its stopped, whole server is stopped as well.

Added this feature under a new watch_is_main_process config argument so it is backwards compatible and will not introduce new behaviour in the tool without explicitly enabling it.

fix #923

Does this change make sense and I could expect it to be introduced in the system?

@Slach Slach added this to the 2.5.9 milestone May 16, 2024
@Slach
Copy link
Collaborator

Slach commented May 16, 2024

build failed
could check make clean build/linux/amd64/clickhouse-backup locally?

pkg/config/config.go Outdated Show resolved Hide resolved
@tadus21 tadus21 requested a review from Slach May 17, 2024 10:52
@Slach
Copy link
Collaborator

Slach commented May 17, 2024

for tesflows pass

rm -rf ./test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot
cp ./test/testflows/.env.example ./test/testflows/.env

edit ./test/testflows/.env

run tests

bash -xe ./test/testflows/run.sh

commit changed cli.py.cli.snapshot

@coveralls
Copy link

coveralls commented May 17, 2024

Pull Request Test Coverage Report for Build 9132124402

Details

  • 5 of 12 (41.67%) changed or added relevant lines in 1 file are covered.
  • 259 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-2.0%) to 65.287%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/server/server.go 5 12 41.67%
Files with Coverage Reduction New Missed Lines %
pkg/metadata/load.go 2 81.82%
pkg/backup/restore.go 2 67.06%
pkg/backup/backuper.go 2 65.41%
cmd/clickhouse-backup/main.go 2 73.68%
pkg/storage/object_disk/object_disk.go 5 70.95%
pkg/status/status.go 9 70.2%
pkg/config/config.go 10 70.9%
pkg/storage/general.go 23 61.66%
pkg/server/server.go 23 52.97%
pkg/storage/s3.go 25 43.88%
Totals Coverage Status
Change from base Build 9131034897: -2.0%
Covered Lines: 8409
Relevant Lines: 12880

💛 - Coveralls

@Slach
Copy link
Collaborator

Slach commented May 17, 2024

All looks good now, thanks for contribution

small step before merge
could you run pull latest master? i'm upgraded github action build.yaml to allow Cleanup step works properly

@tadus21 tadus21 force-pushed the tadasobolevicius/stop_api_server_if_watch_fails branch from 73623da to 42a6946 Compare May 17, 2024 17:37
@tadus21
Copy link
Contributor Author

tadus21 commented May 17, 2024

All looks good now, thanks for contribution

small step before merge could you run pull latest master? i'm upgraded github action build.yaml to allow Cleanup step works properly

Thanks for helping with this. I think because I rebased master I need to get a new workflow approval in order to merge this

@Slach Slach merged commit a2b53f8 into Altinity:master May 18, 2024
20 checks passed
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.

API server should restart if watch command fails
3 participants