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

server, cloud: plumb an ExternalStorage memory monitor #80669

Closed
wants to merge 1 commit into from

Conversation

adityamaru
Copy link
Contributor

@adityamaru adityamaru commented Apr 27, 2022

This change adds a dedicated ExternalStorage memory monitor
as a child of the root memory monitor. This can be used to memory
monitor ExternalStorage operations.

As a start, we memory monitor the ChunkSize that every Azure,S3 and GCS
Writer will buffer when uploading file chunks to storage. During a backup
this could be num nodes * ChunkSize of memory which was previously
unmonitored.

Release note: None

@adityamaru adityamaru requested review from dt and stevendanna April 27, 2022 21:56
@adityamaru adityamaru requested a review from a team as a code owner April 27, 2022 21:56
@adityamaru adityamaru requested a review from a team April 27, 2022 21:56
@adityamaru adityamaru requested review from a team as code owners April 27, 2022 21:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru removed request for a team April 27, 2022 21:57
@adityamaru
Copy link
Contributor Author

First commit is from #80668.

This change adds a dedicated ExternalStorage memory monitor
as a child of the root memory monitor. This can be used to memory
monitor ExternalStorage operations.

As a start, we memory monitor the ChunkSize that every Azure,S3 and GCS
Writer will buffer when uploading file chunks to storage. During a backup
this could be num nodes * ChunkSize of memory which was previously
unmonitored.

Release note: None
@adityamaru adityamaru force-pushed the chunk-size-setting branch from b0c1217 to 2a045b5 Compare May 16, 2022 20:42
@adityamaru
Copy link
Contributor Author

CI failures are just lint fixups, so this is RFAL!

@dt
Copy link
Member

dt commented May 17, 2022

Would it make sense to define a common wrapper and change MakeExternalStorage to always return a wrapped specific implementation?

We already have it doing that with a wrapper that is only concerned with limiters, but we could generalize that wrapper so it is responsible for all common tasks, like wrapping returned readers/writers in rate limiters, reserving the common buffer size for any created reader or writer / freeing it on close, etc?

Having every implementation need to remember to do the grow/shink calls just right makes me a little nervous long term.

@adityamaru
Copy link
Contributor Author

Would it make sense to define a common wrapper and change MakeExternalStorage to always return a wrapped specific implementation?

I think it does, I did briefly think about it in the context of having a common place where we wrap storage-specific errors as transient. @stevendanna is this going to interfere with the accounting change you are trying to check-in?

@stevendanna
Copy link
Collaborator

I definitely like the idea of implementing this in a wrapper.

@stevendanna is this going to interfere with the accounting change you are trying to check-in?

I'm not too worried. I think if we structure the code well, the worst case is that my backport will need just a few lines of code removed in a couple of places.

@stevendanna
Copy link
Collaborator

Another follow up question here: Currently, we are setting up this monitor at server-startup and passing it into the builder. This is nice since then the caller doesn't have to worry about it. But, do we have cases where it would make more sense to allow the caller to pass in a monitor?

@adityamaru adityamaru closed this Dec 28, 2022
@adityamaru adityamaru deleted the chunk-size-setting branch December 28, 2022 06:00
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.

4 participants