Upgrade go-msgpack to v2 2.1.1 #38
Merged
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
This adds the option to specify what version of the `time.Time` encoding format is desired. The default option is false, which preserves compatibility with the current dependency of `hashicorp/go-msgpack 0.5.5` in go.mod, but users of this library will probably want to override the the setting. While we're here, upgrade the go.mod file. I tested that this appears to work with Nomad. This needs to be updated and merged after hashicorp/raft#577
5 tasks
2 tasks
swenson
pushed a commit
to hashicorp/vault
that referenced
this pull request
Oct 19, 2023
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designed to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running Vault 1.15.0 (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 and will need to be updated after they are merged to get the `go.mod` fixes removed.
8 tasks
schmichael
reviewed
Oct 23, 2023
| // use the newer format of time.Time when encoding, used in versions <=0.5.5 | ||
| // by default. Decoding is not affected, as all decoders know how to decode | ||
| // both formats. | ||
| MsgpackUseNewTimeFormat bool |
There was a problem hiding this comment.
If this is the format from 0.5.5 and before, shouldn't this be MsgpackUse Old TimeFormat? I swear I'm not trying to be pedantic and am genuinely confused. 😅
There was a problem hiding this comment.
MsgpackUseNewTimeFormat being true will cause TimeNotBuiltin to be set to false when configuring the msgpack Codec.
TimeNotBuiltin forces the msgpack library to not treat time as a builtin but instead rely on the binary marshaller (or text marshaller interfaces according to the docs).
I think the behavior is correct and also that having the zero value here cause disabling of the not-backwards-compatible time encoding is best so that we don't break Consul or Nomad.
I do think that the comment about the field is a little off as it sort of says that it causes the newer format to be used and that the format would be the default in versions less than 0.5.5. Should it be versions greater than 0.5.5?
There was a problem hiding this comment.
@mkeeler is correct. I got the comment slightly wrong, and will correct it.
It's confusing due to the double-negative and trying to work with the default-false behavior of Go.
mkeeler
approved these changes
Oct 24, 2023
swenson
pushed a commit
to hashicorp/nomad
that referenced
this pull request
Oct 25, 2023
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designe to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running the prior version of Nomad (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/serf#705 - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 - [ ] hashicorp/memberlist#292 and maybe - [ ] hashicorp/net-rpc-msgpackrpc#12
swenson
pushed a commit
to hashicorp/vault
that referenced
this pull request
Oct 25, 2023
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designed to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running Vault 1.15.0 (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 and will need to be updated after they are merged to get the `go.mod` fixes removed.
|
Thanks! |
swenson
pushed a commit
to hashicorp/vault
that referenced
this pull request
Nov 15, 2023
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designed to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running Vault 1.15.0 (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 and will need to be updated after they are merged to get the `go.mod` fixes removed.
swenson
pushed a commit
to hashicorp/vault
that referenced
this pull request
Dec 22, 2023
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designed to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running Vault 1.15.0 (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 and will need to be updated after they are merged to get the `go.mod` fixes removed.
swenson
pushed a commit
to hashicorp/vault
that referenced
this pull request
Jan 8, 2024
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designed to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running Vault 1.15.0 (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 and will need to be updated after they are merged to get the `go.mod` fixes removed.
swenson
pushed a commit
to hashicorp/vault
that referenced
this pull request
Jan 8, 2024
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designed to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running Vault 1.15.0 (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - hashicorp/raft-boltdb#38 - hashicorp/raft#577 I did a simple pair of benchmarks (one with a final sync, one without) and ran them before and after on both my Mac (M2 Max) laptop and my Linux (AMD Threadripper 3970X) desktop. tl;dr There was no performance difference for this benchmark. ``` goos: darwin goarch: arm64 pkg: github.com/hashicorp/vault/physical/raft │ a.txt │ b.txt │ │ sec/op │ sec/op vs base │ RaftWithNetwork-10 58.65m ± 2% 58.62m ± 2% ~ (p=0.937 n=6) ``` ``` goos: linux goarch: amd64 pkg: github.com/hashicorp/vault/physical/raft cpu: AMD Ryzen Threadripper 3970X 32-Core Processor │ c.txt │ d.txt │ │ sec/op │ sec/op vs base │ RaftWithNetwork-64 5.861m ± 1% 5.837m ± 0% ~ (p=0.240 n=6) ```
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.
This adds the option to specify what version of the
time.Timeencoding format is desired. The default option is false, which preserves compatibility with the current dependency ofhashicorp/go-msgpack 0.5.5in go.mod, but users of this library will probably want to override the the setting.While we're here, upgrade the go.mod file.
I tested that this appears to work with Nomad.
This needs to be updated and merged after
hashicorp/raft#577