kv: don't use LegacyTimestamp in Liveness records#56190
Closed
nvb wants to merge 2 commits intocockroachdb:masterfrom
Closed
kv: don't use LegacyTimestamp in Liveness records#56190nvb wants to merge 2 commits intocockroachdb:masterfrom
nvb wants to merge 2 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
6db235a to
c2bb8c9
Compare
andreimatei
reviewed
Nov 4, 2020
Contributor
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)
Contributor
Author
|
This appears to be running into issues with the |
Contributor
|
maybe #55520 helps
…On Wed, Nov 4, 2020 at 4:09 PM Nathan VanBenschoten < ***@***.***> wrote:
This appears to be running into issues with the acceptance/version-upgrade
test. I think this is because master still reports a v20.2.0 build tag,
so we still start with a v20.1 cluster in that test. I'll need to fix that
first before merging the rest of this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#56190 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC4C4LD3XRHORW4MCGZGUTSOG7J7ANCNFSM4TG4AYCQ>
.
|
Contributor
Author
|
It does! Thanks for the pointer. |
c2bb8c9 to
3bb19a3
Compare
|
I just pushed a |
3bb19a3 to
75d2fd0
Compare
nvb
added a commit
to nvb/cockroach
that referenced
this pull request
Nov 10, 2020
This was breaking cockroachdb#56190. In that PR, we change the proto encoding of Liveness. We intended to make this safe in 9c0dce9 by using the existing marshalled proto when performing cputs. Unfortunately, that change wasn't quite enough, as it didn't ensure that the local raw byte representation of the proto would get updated on a failed cput. This could lead to an infinite loop of failed liveness updates, as we saw in `acceptance/version-upgrade`. This PR will make that change safe, but unfortunately, we'll have to wait another release for it.
Contributor
Author
|
Thanks @lucy-zhang! Your change worked. Unfortunately, it revealed that the node liveness code wasn't as ready as it thought it was for variable proto encoding. So we're going to have to land #56477 in v21.1 and hold off on this PR until v21.2. That's a shame, but nothing depends on this change so we can just wait it out. |
nvb
added a commit
to nvb/cockroach
that referenced
this pull request
Nov 10, 2020
This was breaking cockroachdb#56190. In that PR, we change the proto encoding of Liveness. We intended to make this safe in 9c0dce9 by using the existing marshalled proto when performing cputs. Unfortunately, that change wasn't quite enough, as it didn't ensure that the local raw byte representation of the proto would get updated on a failed cput. This could lead to an infinite loop of failed liveness updates, as we saw in `acceptance/version-upgrade`. This PR will make that change safe, but unfortunately, we'll have to wait another release for it.
craig bot
pushed a commit
that referenced
this pull request
Nov 10, 2020
56477: liveness: replace liveness on raw encoding change r=nvanbenschoten a=nvanbenschoten This was breaking #56190. In that PR, we change the proto encoding of Liveness. We intended to make this safe in 9c0dce9 by using the existing marshalled proto when performing cputs. Unfortunately, that change wasn't quite enough, as it didn't ensure that the local raw byte representation of the proto would get updated on a failed cput. This could lead to an infinite loop of failed liveness updates, as we saw in `acceptance/version-upgrade`. This PR will make that change safe, but unfortunately, we'll have to wait another release for it. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Completes migration started in 9c0dce9.