Move file storage extension to core #3273
Conversation
pmm-sumo
changed the title
pmm-sumo
force-pushed
the
main-with-storage-extension
branch
from
1cbb225 to
af7e210
Compare
pmm-sumo
force-pushed
the
main-with-storage-extension
branch
from
af7e210 to
10c5218
Compare
pmm-sumo
changed the title
| @@ -0,0 +1 @@ | |||
| include ../../Makefile.Common | |||
There was a problem hiding this comment.
Artifact from being a contrib component. Should be removed in core implementation.
| @@ -0,0 +1 @@ | |||
| include ../../Makefile.Common | |||
| if err != nil { | ||
| return nil, fmt.Errorf("create client: %v", err) | ||
| } | ||
| lfs.clients = append(lfs.clients, client) |
There was a problem hiding this comment.
Is it guaranteed that GetClient is not called by multiple goroutines? You need to protect the slice.
There was a problem hiding this comment.
This slice is only used for client closure, which will become the responsibility of each component that requests a client. So this will go away entirely. Point taken though.
| for _, client := range lfs.clients { | ||
| client.close() | ||
| } |
There was a problem hiding this comment.
Sounds like a chaos here, there is no clear owner of the components. Which component is responsible to close the "client" instance? Not well documented and based on the code seems a bit strange to say the least.
There was a problem hiding this comment.
Yeah, fair point.
When I implemented this, I had in mind the concept of cleaning up stale storage files #3149 (contrib), which I think would properly be the responsibility of this extension and might look similar to this implementation of tracking/closing clients.
In any case, you are right - the clients can and should be handled by the components that requested them, and this expectation should be clearly documented.
|
Thanks @bogdandrutu , let me give a bit of context. I am researching how to implement WAL in queued_retry (and then other components, batch processor, group by trace processor, etc.) and one of the ideas is to leverage storage extension which is already part of In this PR, I just wanted to show what would be the scope of bringing storage extension to the core, without changing the structure of it |
There was a problem hiding this comment.
@pmm-sumo I will make the noted change on a contrib PR. Please let me know if you'd like to me to PR to your branch here as well.
| @@ -0,0 +1 @@ | |||
| include ../../Makefile.Common | |||
There was a problem hiding this comment.
Artifact from being a contrib component. Should be removed in core implementation.
| @@ -0,0 +1 @@ | |||
| include ../../Makefile.Common | |||
| for _, client := range lfs.clients { | ||
| client.close() | ||
| } |
There was a problem hiding this comment.
Yeah, fair point.
When I implemented this, I had in mind the concept of cleaning up stale storage files #3149 (contrib), which I think would properly be the responsibility of this extension and might look similar to this implementation of tracking/closing clients.
In any case, you are right - the clients can and should be handled by the components that requested them, and this expectation should be clearly documented.
| if err != nil { | ||
| return nil, fmt.Errorf("create client: %v", err) | ||
| } | ||
| lfs.clients = append(lfs.clients, client) |
There was a problem hiding this comment.
This slice is only used for client closure, which will become the responsibility of each component that requests a client. So this will go away entirely. Point taken though.
pmm-sumo
changed the title
Thank you @djaglowski ! I can take care of that later and just sync the changes. Also, if you prefer the prepare the PR with filestorage changes to core (which I think you deserve, as the author) :) then it works perfectly fine for me too. I can adjust my PR with WAL implementation later accordingly to either of options. |
|
As per our discussion during the SIG, closing this one. Going to extract just the API part and propose it in a separate PR |
Description:
This is a copy of file storage extension moved into the core, which might be required for file-storage extension backed WAL implementation