Skip to content

kv: remove clock update on BatchResponse#76233

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/clockUpdateOnResp
Aug 11, 2022
Merged

kv: remove clock update on BatchResponse#76233
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/clockUpdateOnResp

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Feb 8, 2022

Before this change, we were updating the local clock with each BatchResponse's WriteTimestamp. This was meant to handle cases where the batch request timestamp was forwarded during evaluation.

This was unnecessary for two reasons.

The first is that a BatchResponse can legitimately carry an operation timestamp that leads the local HLC clock on the leaseholder that evaluated the request. This has been true since #80706, which introduced the concept of a "local timestamp". This allowed us to remove the (broken) attempt at ensuring that the HLC on a leaseholder always leads the MVCC timestamp of all values in the leaseholder's keyspace (see the update to pkg/kv/kvserver/uncertainty/doc.go in that PR).

The second was that it was not even correct. The idea behind bumping the HLC on the response path was to ensure that if a batch request was forwarded to a newer timestamp during evaluation and then completed a write, that forwarded timestamp would be reflected in the leaseholder's HLC. However, this ignored the fact that any forwarded timestamp must have either come from an existing value in the range or from the leaseholder's clock. So if those didn't lead the clock, the derived timestamp wouldn't either. It also ignored the fact that the clock bump here was too late (post-latch release) and if it had actually been needed (it wasn't), it wouldn't have even ensured that the timestamp on any lease transfer led the maximum time of any response served by the outgoing leaseholder.

There are no mixed-version migration concerns of this change, because #80706 ensured that any future-time operation will still continue to use the synthetic bit until all nodes are running v22.2 or later.

@nvb nvb requested a review from tbg February 8, 2022 14:43
@nvb nvb requested a review from a team as a code owner February 8, 2022 14:43
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stared at the comment a bunch but still confused. Attempt at a re-wording to highlight my particular confusions:

  • The first is that a BatchRequest can legitimately remain ahead of the local HLC clock if it contains a synthetic timestamp ("local timestamp") tomorrow. For example, <actually give an example, I'm too confused right now, what do you mean by this bit about "as long as any clock reading"..?>
  • The second is that even for requests without such timestamps, the response timestamp would only ever differ from the incoming timestamp by one logical tick, so... (and here I'm confused again, if you say "they don't" then why is that ok, is this synthetic bit again?)

I'm admittedly not anywhere near my brightest moment right now, but I'd appreciate a comment that caters to me in my current state as well 🥲

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Before this change, we were updating the local clock with each BatchResponse's
WriteTimestamp. This was meant to handle cases where the batch request timestamp
was forwarded during evaluation.

This was unnecessary for two reasons.

The first is that a BatchResponse can legitimately carry an operation timestamp
that leads the local HLC clock on the leaseholder that evaluated the request.
This has been true since cockroachdb#80706, which introduced the concept of a "local
timestamp". This allowed us to remove the (broken) attempt at ensuring that the
HLC on a leaseholder always leads the MVCC timestamp of all values in the
leaseholder's keyspace (see the update to `pkg/kv/kvserver/uncertainty/doc.go`
in that PR).

The second was that it was not even correct. The idea behind bumping the HLC on
the response path was to ensure that if a batch request was forwarded to a newer
timestamp during evaluation and then completed a write, that forwarded timestamp
would be reflected in the leaseholder's HLC. However, this ignored the fact that
any forwarded timestamp must have either come from an existing value in the
range or from the leaseholder's clock. So if those didn't lead the clock, the
derived timestamp wouldn't either. It also ignored the fact that the clock bump
here was too late (post-latch release) and if it had actually been needed (it
wasn't), it wouldn't have even ensured that the timestamp on any lease transfer
led the maximum time of any response served by the outgoing leaseholder.

There are no mixed-version migration concerns of this change, because cockroachdb#80706
ensured that any future-time operation will still continue to use the synthetic
bit until all nodes are running v22.2 or later.
@nvb nvb force-pushed the nvanbenschoten/clockUpdateOnResp branch from b9ee582 to 35151c9 Compare August 8, 2022 18:08
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Aug 8, 2022

@tbg thanks for taking a look and providing suggestions. I've rewritten the commit message to more clearly explain why this is unnecessary now and also was broken before when we thought it was necessary. PTAL.

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Aug 11, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 11, 2022

Build succeeded:

@craig craig bot merged commit 8e3ee57 into cockroachdb:master Aug 11, 2022
@nvb nvb deleted the nvanbenschoten/clockUpdateOnResp branch August 12, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants