Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ring: make ring.Buffer generic #93736

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Dec 15, 2022

Epic: none

Release note: None

@ajwerner ajwerner requested a review from a team December 15, 2022 20:03
@ajwerner ajwerner requested a review from a team as a code owner December 15, 2022 20:03
@ajwerner ajwerner requested a review from a team December 15, 2022 20:03
@ajwerner ajwerner requested review from a team as code owners December 15, 2022 20:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/generic-ring-buffer branch 7 times, most recently from f475105 to 91a0fb4 Compare December 16, 2022 02:32
Copy link
Contributor

@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 @ajwerner)


pkg/util/tracing/crdbspan.go line 157 at r4 (raw file):

	// verbose recording because of the span limit) are not part of this buffer;
	// they're in finishedChildren.Root.StructuredRecords.
	structured sizeLimitedBuffer[*tracingpb.StructuredRecord]

do these need to be pointers any more or were they only pointer because they had to be boxed in interface{} anyway?


pkg/util/tracing/tracer.go line 970 at r4 (raw file):

	tagsAlloc             [3]attribute.KeyValue
	childrenAlloc         [4]childRef
	structuredEventsAlloc [3]*tracingpb.StructuredRecord

do

Copy link
Contributor Author

@ajwerner ajwerner 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)


pkg/util/tracing/crdbspan.go line 157 at r4 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

do these need to be pointers any more or were they only pointer because they had to be boxed in interface{} anyway?

I think they want to be pointers because the protoutil.Message interface is implemented on the pointer and they're constructed as pointers, no? Also, for better or for worse, the ring.Buffer doesn't have a mechanism to get a pointer into the buffer itself. If it did you'd have to be careful about lifecycles. I think it's better to leave them as pointers in this PR.


pkg/util/tracing/tracer.go line 970 at r4 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

do

?

@ajwerner ajwerner force-pushed the ajwerner/generic-ring-buffer branch from 91a0fb4 to 68f23cd Compare December 16, 2022 19:06
Epic: none

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/generic-ring-buffer branch from 68f23cd to 3f3218d Compare December 19, 2022 14:51
Copy link
Contributor

@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.

:lgtm:

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

@ajwerner
Copy link
Contributor Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 19, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Dec 19, 2022

Build succeeded:

@craig craig bot merged commit 369c405 into cockroachdb:master Dec 19, 2022
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