feat: Serialize duration as string, not as nanosecs, in API responses and DB #1542
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaljurecko
force-pushed
the
michaljurecko-PSGO-387-duration-pkg
branch
from
b8e7465
to
daccaa0
Compare
michaljurecko
commented
Jan 25, 2024
Comment on lines
+1
to
+7
| // Package duration provides a wrapper for time.Duration type, | ||
| // to serialize duration as string instead of int64 nanoseconds. | ||
| // | ||
| // This cannot be changed in Go 1.X, it would break backward compatibility, see: | ||
| // https://github.com/golang/go/issues/10275 | ||
| // "This isn't possible to change now, as it would change the encodings produced by programs that exist today." | ||
| package duration |
Comment on lines
472
to
478
| "wait": false, | ||
| "checkInterval": 1000000, | ||
| "checkInterval": "1ms", | ||
| "countTrigger": 100, | ||
| "bytesTrigger": "100KB", | ||
| "intervalTrigger": 100000000 | ||
| "intervalTrigger": "100ms" | ||
| } | ||
| } |
There was a problem hiding this comment.
Change in the database snapshot.
Comment on lines
39
to
41
| syncCounter++ | ||
| clk.Add(cfg.SyncInterval) | ||
| clk.Add(cfg.SyncInterval.Duration()) | ||
| assert.Eventually(t, func() bool { |
There was a problem hiding this comment.
Updated in related code.
jachym-tousek-keboola
approved these changes
Jan 26, 2024
Comment on lines
+144
to
+147
| if v, ok := fl.Field().Interface().(time.Duration); ok { | ||
| value = v | ||
| } else if v, ok := fl.Field().Interface().(duration.Duration); ok { | ||
| value = v.Duration() |
There was a problem hiding this comment.
Okay I think I need a bit more info on type casting here. 🤔 With duration.Duration being defined as
type Duration time.Duration
I'd expect the first condition to pass even for duration.Duration so the second branch would never happen.
If that's not the case, what am I missing?
There was a problem hiding this comment.
Notes from call:
var value any = duration.Duration(123) // 123ns
var value any = int64(123)
--------------
if v, ok := value.(duration.Duration); ok {
// ...
}
--------------
swtich v := value.(type) {
case duration.Duration:
case int64:
default:
panic(....)
}
Comment on lines
-19
to
+22
| SyncInterval time.Duration `configKey:"interval" configUsage:"Statistics synchronization interval, from memory to the etcd." validate:"required,minDuration=100ms,maxDuration=5s"` | ||
| SyncTimeout time.Duration `configKey:"timeout" configUsage:"Statistics synchronization timeout." validate:"required,minDuration=1s,maxDuration=1m"` | ||
| SyncInterval duration.Duration `configKey:"interval" configUsage:"Statistics synchronization interval, from memory to the etcd." validate:"required,minDuration=100ms,maxDuration=5s"` | ||
| SyncTimeout duration.Duration `configKey:"timeout" configUsage:"Statistics synchronization timeout." validate:"required,minDuration=1s,maxDuration=1m"` |
There was a problem hiding this comment.
I wonder, should we disallow time.Duration in linter?
There was a problem hiding this comment.
No, if the struct is not used in db/api, time.Duration is ok.
Base automatically changed from
michaljurecko-PSGO-387-config-patch
to
RFC-2023-008
January 26, 2024 11:19
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of: https://keboola.atlassian.net/browse/PSGO-388
Changes:
time.Durationis by default serialized asint64, as number of nanoseconds.time.Durationvalues that need to be handled.duration.Durationtype, that serializes as string, for example:100ms,1h0m, ...