Skip to content

Configurable initial and max retained batch sizes #3139

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

lahsivjar
Copy link
Contributor

Closes: #2780

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@lahsivjar
Copy link
Contributor Author

@jbowens Would appreciate a quick review whenever you have time.

@lahsivjar
Copy link
Contributor Author

@jbowens Friendly ping, any feedback on the PR is appreciated.

@inge4pres
Copy link

This is very cool and I also have a similar use case 👍🏼
Plus, there is no breaking change to the public API so it would be nice to have this landed 🙏🏼

Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here! 😅

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @lahsivjar)


batch.go line 275 at r4 (raw file):

	formatKey      base.FormatKey
	abbreviatedKey AbbreviatedKey
	opts           *batchOptions

I think this should not be a pointer—otherwise, we'll suffer an additional heap allocation for every new batch initialization

Copy link
Member

@RaduBerinde RaduBerinde 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: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jbowens and @lahsivjar)


batch.go line 275 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I think this should not be a pointer—otherwise, we'll suffer an additional heap allocation for every new batch initialization

+1. ensureDefaults should just modify in-place and return nothing.


batch.go line 2421 at r4 (raw file):

func WithInitialSizeBytes(s int) BatchOption {
	return func(opts *batchOptions) {
		if s > 0 {

[nit] The special handling of s <= 0 is odd here, it shouldn't be valid to use such a value.

@lahsivjar
Copy link
Contributor Author

@jbowens @RaduBerinde Thanks both for your reviews. I have addressed the comments now.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Thanks! Can you squash the commits?

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @jbowens and @lahsivjar)


batch_test.go line 1744 at r5 (raw file):

		},
		{
			name: "with_invalid_values",

I think we need to remove this test.

@lahsivjar
Copy link
Contributor Author

Thanks! Can you squash the commits?

@RaduBerinde Done! Also removed the bad test.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Thank you!

@RaduBerinde RaduBerinde merged commit d253ff7 into cockroachdb:master Apr 11, 2024
@lahsivjar lahsivjar deleted the batch-opts branch April 26, 2024 11:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make batchMaxRetainedSize configurable
5 participants