Minor: Rename RepartitionMetrics::repartition_time to RepartitionMetrics::repart_time to match metric
#11478
Conversation
alamb
changed the title
RepartitionMetrics::repartition_time to RepartitionMetrics::repart_time to match metric
alamb
force-pushed
the
alamb/rename_field
branch
from
c53869c to
44d89a4
Compare
alamb
changed the title
RepartitionMetrics::repartition_time to RepartitionMetrics::repart_time to match metricRepartitionMetrics::repartition_time to RepartitionMetrics::repart_time to match metric
| @@ -415,7 +415,7 @@ struct RepartitionMetrics { | |||
| /// Time in nanos to execute child operator and fetch batches | |||
| fetch_time: metrics::Time, | |||
| /// Time in nanos to perform repartitioning | |||
| repartition_time: metrics::Time, | |||
| repart_time: metrics::Time, | |||
There was a problem hiding this comment.
to me, the previous name is more readable. do we need this change?
There was a problem hiding this comment.
My rationale was so that the field name matched the display name of this metric (which is repart_time)
(a few lines down from here)
Another potential would be to rename the metric to "repartition_time" which would be a user facing change (which is probably fine).
I think it would be nice to have them consistent. The exact value I have less of an opinion of
There was a problem hiding this comment.
Yeah, i agree that consistency is really desirable. Therefore this PR is a good change (I will approve).
I would prefer that we change the metric name though. Readable naming is as valuable for code as it is for external metrics. I know it's a breaking change, but I don't know of other way to improve the naming while avoiding the breakage.
|
Good plan -- how about we merge this PR (which is just code reorg) and then I'll make a follow on to propose renaming the public metric name for separate consideration |
|
SGTM |
|
Filed #11703 with the proposed change to the metric name |
Draft until #11440 mergesWhich issue does this PR close?
None
Rationale for this change
While reviewing #11440 from @korowa it took me a little while to find how the metric was plumbed through. Part of that was that the field name is slightly different
What changes are included in this PR?
Rename a field to match the metric it tracks
Are these changes tested?
By existing CI
Are there any user-facing changes?
No this is entirely internal