hashicorp/raft

msgpack dependencies

SimonRichardson opened this issue · 7 comments

This is more of a potential problem that's up and coming, rather than a problem right now (msgpack v1.1.5 tag and mspack v0.5.5 as seen https://github.com/hashicorp/go-msgpack/tags). The upstream msgpack has changed how time was encoded/decoded and the workaround was provided.

I suspect either two options, decode the data with TimeNotBuiltin set and then encode it without, allowing a stable migration or it might be prudent to expose TimeNotBuiltin in the raft configuration (under another name LegacyEncoding or similar), so this can be changed for users with existing data.

Hi @SimonRichardson,

It looks like there's been a new release of hashicorp/go-msgpack, and presumably that maintains our fork's absention from the new time serialization. We'll update this library to make use of that 1.1.5 release.

If you update raft, then existing deployments that utilize the 0.5.5 release that upgrades to 1.5.5 will hit the following error, unless a migration is applied.

failed to get last log at index 52: msgpack decode error [pos 13]: invalid length of bytes for decoding time - expecting 4 or 8 or 12, got 15

Ah, I think I misunderstood you. I thought you were talking about upgrading to newer hashicorp/go-msgpack versions that already exist. But your actual concern was: if hashicorp/go-msgpack were to pull in the upstream changes from ugorji/go, in particular ugorji/go@debb8e2, then any time.TIme values we serialized with older msgpack versions would be a problem. Is that right?

Indeed.

stale commented

Hey there,
We wanted to check in on this request since it has been inactive for at least 90 days.
Have you reviewed the latest godocs?
If you think this is still an important issue in the latest version of the Raft library or
its documentation please feel let us know and we'll keep it open for investigation.
If there is still no activity on this request in 30 days, we will go ahead and close it.
Thank you!

This is still an issue that it would be good to see a fix for.

This has been addressed, see #577 (comment).