optional: add Duration#56951
Conversation
|
This change is |
RaduBerinde
left a comment
There was a problem hiding this comment.
Nice! I was initially thinking that we'd make sure any kind of timer returns at least 1ns. But that is more error prone, and this is more consistent with the int stats.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @RaduBerinde)
pkg/sql/execinfrapb/component_stats.go, line 143 at r1 (raw file):
timeVal := func(v *TimeValue) { if v.HasValue() { v.Set(1)
[nit] time.Nanosecond instead of 1
pkg/sql/execinfrapb/component_stats.proto, line 116 at r1 (raw file):
// The underlying value is the logical value plus 1, so that zero remains the // special case of having no value. message TimeValue {
This is a better design than IntValue: we can't inadvertently set or compare to literal constants, or print with '%d'. It causes an extra byte in the encoded data but that's unlikely to matter.
I will switch IntValue to this design too, and move them both to a separate util/optional package (because I would already use them in other places).
pkg/sql/execinfrapb/component_stats.proto, line 120 at r1 (raw file):
// RawValue is the TimeValue's underlying value. // DO NOT ACCESS THIS DIRECTLY. Use HasValue and Value instead. optional google.protobuf.Duration raw_value = 1 [(gogoproto.nullable) = false,
[nit] could be value_plus_one so it's more clear and less likely that someone will use it without realizing that
asubiotto
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/execinfrapb/component_stats.proto, line 116 at r1 (raw file):
Previously, RaduBerinde wrote…
This is a better design than IntValue: we can't inadvertently set or compare to literal constants, or print with '%d'. It causes an extra byte in the encoded data but that's unlikely to matter.
I will switch IntValue to this design too, and move them both to a separateutil/optionalpackage (because I would already use them in other places).
👍 I'll wait for your optional PR to go in before moving it and merging this PR.
pkg/sql/execinfrapb/component_stats.proto, line 120 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] could be
value_plus_oneso it's more clear and less likely that someone will use it without realizing that
Good idea.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
This commit adds a optional.Duration protobuf that is used to represent time.Duration statistics in the ComponentStats proto. Similar to the optional.Uint struct, this protobuf allows differentiating between set and unset values. These stats were previously stored as time.Durations but EXPLAIN ANALYZE logic test flakiness on recent PRs have shown that it is valid for a time.Duration to be set to 0 in some cases (when recording less than a ns of time). The ComponentStats formatting code assumes a time of 0 is unset, so this would cause some time labels to be elided from the diagram non-deterministically. Release note: None
|
TFTR bors r=RaduBerinde |
56951: optional: add Duration r=RaduBerinde a=asubiotto This commit adds a TimeValue protobuf that is used to represent time.Duration statistics in the ComponentStats proto. Similar to the IntValue struct, this protobuf allows differentiating between set and unset values. These stats were previously stored as time.Durations but EXPLAIN ANALYZE logic test flakiness on recent PRs have shown that it is valid for a time.Duration to be set to 0 in some cases (when recording less than a ns of time). The ComponentStats formatting code assumes a time of 0 is unset, so this would cause some time labels to be elided from the diagram non-deterministically. Release note: None Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com>
|
Build failed: |
|
Looks like an unrelated bors r=RaduBerinde |
|
Build succeeded: |
This commit adds a TimeValue protobuf that is used to represent time.Duration
statistics in the ComponentStats proto. Similar to the IntValue struct, this
protobuf allows differentiating between set and unset values.
These stats were previously stored as time.Durations but EXPLAIN ANALYZE logic
test flakiness on recent PRs have shown that it is valid for a time.Duration to
be set to 0 in some cases (when recording less than a ns of time). The
ComponentStats formatting code assumes a time of 0 is unset, so this would
cause some time labels to be elided from the diagram non-deterministically.
Release note: None