Factor out object read/write from BucketAlertStore into ProtobufStore. #3998
Conversation
|
|
||
| // ProtobufStore is used to read/write protobuf messages to object storage backend. It is implemented | ||
| // using the Thanos objstore.Bucket interface | ||
| type ProtobufStore struct { |
There was a problem hiding this comment.
Naming suggestions welcome.
| @@ -0,0 +1,95 @@ | |||
| package bucketclient | |||
There was a problem hiding this comment.
I assume there's a better place to put this file, perhaps somewhere in pkg/storage or pkg/util, just tell me where and I'll move it.
There was a problem hiding this comment.
As far as it will be used only here, I would keep it here. We can move it around when required. Right now, it doesn't look necessary.
stevesg
force-pushed
the
alert-store-refactor
branch
3 times, most recently
from
610f8e0 to
d075287
Compare
There was a problem hiding this comment.
Thanks for working on this refactoring.
I left a couple of comments but then I stopped reviewing it cause I had a doubt. The structure of the alertmanager config in the bucket is alerts/<user id> where alerts/<user id> is the object key storing the alertmanager config for <user id>.
We're not used to have this pattern for the storage, while we typically have alerts/<user id>/<object name> structure. When we designed the state persistence, we agreed on using this latter structure, but I've the feeling it will clash with this refactoring, because ProtobufStore is designed around the alertmanager structure which is a special case.
I would like to better understand how you plan to use ProtobufStore for the state persistence before continuing with the review. Thanks!
| @@ -0,0 +1,95 @@ | |||
| package bucketclient | |||
There was a problem hiding this comment.
As far as it will be used only here, I would keep it here. We can move it around when required. Right now, it doesn't look necessary.
| } | ||
|
|
||
| // Set writes the content for an entry in the store for a particular user. | ||
| func (s *ProtobufStore) Set(ctx context.Context, userID string, msg proto.Message) error { |
There was a problem hiding this comment.
What do you think about calling it Upload() to keep it specular to the underlying objstore.Bucket function name?
Signed-off-by: Steve Simpson <[email protected]>
stevesg
force-pushed
the
alert-store-refactor
branch
from
d075287 to
d6d47bf
Compare
|
After talking to @pracucci, I like the idea of adding the interfaces onto the existing I've move this to draft for now - no need to review further. |
|
Closing this in preference of #4020 |
What this PR does:
Factor out object read/write from BucketAlertStore into ProtobufStore. The BucketAlertStore is 90% of what I need for persisting the notifications/silences state, so it seems wasteful to copy/paste it.