Add snapshot duration field in QdrantClusterSnapshot status #43
Conversation
Signed-off-by: Emruz Hossain <[email protected]>
hossainemruz
force-pushed
the
feat/emruz/add-snapshot-duration
branch
from
7c5de67 to
0e44aca
Compare
Signed-off-by: Emruz Hossain <[email protected]>
| RetainUntil *metav1.Time `json:"retainUntil,omitempty"` | ||
| // Duration specifies how long it took for the snapshot to complete | ||
| // +optional | ||
| Duration *metav1.Duration `json:"duration,omitempty"` |
There was a problem hiding this comment.
Duration for the field name is too ambiguous - how about CompletionTime, since we speak about that in the comment above?
There was a problem hiding this comment.
Okay. Then, which one should we chose?
CompletionTime *metav1.Duration `json:"completionTime,omitempty"`CompletionTimeSeconds *int `json:"completionTimeSeconds,omitempty"`There was a problem hiding this comment.
Renamed Duration to CompletionTime
There was a problem hiding this comment.
I would go for CompletionTimeSeconds if the operation is never shorter than 1s.
There was a problem hiding this comment.
We will sent this as string to cluster-api. So, it will be much easier to have this as metav1.Duration. It will automatically format into human-readable string.
If we use CompletionTimeSeconds, then we have to reformat it in the cluster-api side before we show to the user. Also, it will not look good on additional print column.
There was a problem hiding this comment.
It will automatically format into human-readable string.
You should then document what is the schema of the field, i.e. how it should be parsed if somebody need that.
There was a problem hiding this comment.
Added comment showing sample format.
Signed-off-by: Emruz Hossain <[email protected]>
Signed-off-by: Emruz Hossain <[email protected]>
Ticket: https://github.com/qdrant/cloud-pm/issues/1769