Skip to content

kvserver: fix the log tag on split#59813

Open
andreimatei wants to merge 1 commit intocockroachdb:masterfrom
andreimatei:small.logtag
Open

kvserver: fix the log tag on split#59813
andreimatei wants to merge 1 commit intocockroachdb:masterfrom
andreimatei:small.logtag

Conversation

@andreimatei
Copy link
Copy Markdown
Contributor

Make the initial lease application on the RHS after a split reflect the
fact that it's executing in the context of the RHS, not the LHS.

Release note: None

@andreimatei andreimatei requested review from nvb and tbg February 4, 2021 18:53
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)


pkg/kv/kvserver/store_split.go, line 278 at r1 (raw file):

	rightRepl.mu.Lock()
	defer rightRepl.mu.Unlock()
	ctx = logtags.AddTag(ctx, "r", rightRepl.RangeID)

Can we use the RHS's AmbientContext for this?

Copy link
Copy Markdown
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)


pkg/kv/kvserver/store_split.go, line 278 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can we use the RHS's AmbientContext for this?

ya, done

Copy link
Copy Markdown
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)


pkg/kv/kvserver/replica_proposal.go, line 306 at r2 (raw file):

	ctx context.Context, newLease roachpb.Lease, permitJump bool,
) {
	ctx = r.AnnotateCtx(ctx)

Let's keep this in prepareRightReplicaForSplit. That's the only case where the context won't already be properly annotated.


pkg/kv/kvserver/store_split.go, line 278 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

ya, done

Did you confirm that the AmbientContext can be used to overwrite an existing log tag?

@tbg tbg removed their request for review February 22, 2021 11:38
@andreimatei andreimatei requested a review from a team as a code owner January 22, 2022 21:43
Make the initial lease application on the RHS after a split reflect the
fact that it's executing in the context of the RHS, not the LHS.

Release note: None
Copy link
Copy Markdown
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/kv/kvserver/store_split.go, line 278 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Did you confirm that the AmbientContext can be used to overwrite an existing log tag?

I did. This is also documented a few layers down, here


pkg/kv/kvserver/replica_proposal.go, line 306 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's keep this in prepareRightReplicaForSplit. That's the only case where the context won't already be properly annotated.

done

@irfansharif irfansharif added the X-noremind Bots won't notify about PRs with X-noremind label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

X-noremind Bots won't notify about PRs with X-noremind

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants