-
Notifications
You must be signed in to change notification settings - Fork 117
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
ClickHouse as a subprocess #5347
Conversation
cli/cmd/start/clickhouse.go
Outdated
if _, err := os.Stat(destPath); err == nil { | ||
// ClickHouse binary already exists | ||
// TODO: Check compatibility in case the binary is outdated. | ||
return destPath, nil | ||
} |
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.
Can we just incorporate the version number in the path, i.e. ~/.rill/clickhouse/vXX/
?
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.
I haven't found a way to determine the version of the Clickhouse binary without running and querying it.
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.
What if we pin to a specific version when downloading the binary? Like we pin to a specific DuckDB version. Then we would know it, right?
# Conflicts: # cli/cmd/start/start.go
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
…ckhouse-sub-process
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.
Caught up with Eugene and he mentioned the following todos are still outstanding:
- Download the proper binary per OS and architecture
- Pin to a specific ClickHouse version and cache it by version name in
~/.rill
- Debug the timeout errors that occasionally occur
if err := e.cmd.Wait(); err != nil { | ||
e.logger.Error("clickhouse server exited with an error", zap.Error(err)) | ||
} |
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.
What about when there's a graceful shutdown (i.e. stop
is called) – will ClickHouse shut down with status 0 or will there be some kind of error? If there's an error, we should probably skip it here.
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.
There is no error in case of a graceful shutdown
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Closes #5207