-
Notifications
You must be signed in to change notification settings - Fork 42
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] Use chrono::UTC for distributed_ddl_queue endpoint #7001
Conversation
You can add query-specific settings by tacking them on at the end: https://clickhouse.com/docs/en/sql-reference/statements/select#syntax. So you would do |
Nice! Thanks @bnaecker, I've made the modifications |
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.
Looks good! One question about parsing, but otherwise good to go. Thanks!
clickhouse-admin/types/src/lib.rs
Outdated
@@ -1007,7 +1008,7 @@ pub struct DistributedDdlQueue { | |||
/// Exception message | |||
pub exception_text: String, | |||
/// Query finish time | |||
pub query_finish_time: String, | |||
pub query_finish_time: DateTime<Utc>, | |||
/// Duration of query execution (in milliseconds) | |||
pub query_duration_ms: String, |
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.
Looks like this is a UInt64
, is it worth parsing into a stronger type during deserialization? Something like a Duration
, or at least u64
?
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.
It's a string with a number in it :( I initially had it as a u64 and it didn't parse, but let me try with Duration
!
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.
Ah, this is because we're asking for JSON, and by default ClickHouse quotes 64-bit numbers. That causes serde
to think it's a string, because it's written like "1"
, rather than just 1
. You can set the output_format_json_quote_64bit_integers
setting to ask it not to that.
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.
Looks like Duration doesn't accept strings either :(
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.
See my earlier comment. You can ask CH not to quote big integers, so that serde
can parse it just fine.
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.
ugh sorry, GitHub hid that comment from me, I see it now thanks!
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.
No worries, I figured :) Just didn't want you to lose track of it. I think that will work, but it's honestly up to you if you want to apply it. I don't have context for what this all will be used for!
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.
That worked, thanks! It's definitely better with strong types. Wasn't able to get Duration
to parse but at least it's u64
now. We're going to use this endpoint (and others) for long term monitoring and debugging of the ClickHouse cluster.
Modified the SQL query to set the date/time format to one we can parse.