Skip to content

Add a generic message reader interface - #363

Open
meroton-benjamin wants to merge 5 commits into
buildbarn:mainfrom
meroton:pr/message-reader
Open

Add a generic message reader interface#363
meroton-benjamin wants to merge 5 commits into
buildbarn:mainfrom
meroton:pr/message-reader

Conversation

@meroton-benjamin

Copy link
Copy Markdown
Contributor

Adds a generic message reader interface for reading proto messages from storage. Many Buildbarn components depend on blobstore.BlobAccess only as a source of proto.Message and this interface allows us to sever that dependency in components which are otherwise not interested in the blobstore.BlobAccess interface.

Comment thread pkg/storage/message_reader.go Outdated
//
// Implementations may use the provided message to store the
// unmarshalled message.
ReadMessage(ctx context.Context, d digest.Digest, msg T) (T, error)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's with the msg T argument? Normally you only provide that to ToProto() to indicate the type you're interested in. For MessageReader that should already be implied by the interface itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

msg T argument removed in favor of adding an allocator to the constructor.

Adds a generic message reader interface for reading proto messages from
storage. Many Buildbarn components depend on blobstore.BlobAccess only
as a source of proto.Message and this interface allows us to sever that
dependency in components which are otherwise not interested in the
blobstore.BlobAccess interface.
@meroton-benjamin

Copy link
Copy Markdown
Contributor Author

State of latest version:

  • MessageReader moved to pkg/cas
  • Non-CAS uses of the MessageReader interface has been removed.
  • The storage message has been removed from the function signature and instead the constructor takes an allocator.

Looking at places which reads messages from the CAS in bb-storage, there only seems to be CompletenessCheckingBlobAccess and NestedBlobReplicator. In CompletenessCheckingBlobAccess, it actually only fetches tree objects in a streaming fashion from the CAS while NestedBlobReplicator fetches tree objects in a streaming fashion and Directory/Action objects in a non streaming fashion.

In the last commit here Use message reader in bb-copy a resolution to this has been done by changing the NestedBlobReplicator constructor to:

func NewNestedBlobReplicator(replicator CASReplicator, maximumMessageSizeBytes int, actionReader cas.MessageReader[*remoteexecution.Action], directoryReader cas.MessageReader[*remoteexecution.Directory], treeReader BlobStreamReader, digestKeyFormat digest.KeyFormat)

With:

type CASReplicator interface {
	ReplicateMultiple(ctx context.Context, digests digest.Set) error
}

type BlobStreamReader interface {
	ReadStream(ctx context.Context, d digest.Digest) (io.ReadCloser, error)
}

Which allows me to tie up the loop. Does this look like a good signature and if so I can consume the BlobStreamReader interface from CompletenessCheckingBlobAccess as well.

"google.golang.org/protobuf/proto"
)

type blobAccessMessageReader[T proto.Message] struct {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you change this line to:

type blobAccessMessageReader[T any, TPtr interface {
        *T
        proto.Message
}] struct {

Then you can use T to refer to a concrete message type, and TPtr whenever you need a pointer to one. That way you can get rid of the allocator function.

Comment thread pkg/cas/message_reader.go

// MessageReader can be used to read a parsed proto.Message from the
// Content Addressable Storage.
type MessageReader[T proto.Message] interface {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you wanted to, you could change this proto.Message to any. That way you get rid of the dependency on the Protobuf package. Though in this case I'm ambivalent.


// BlobStreamReader provides a stream to the raw bytes of a blob.
type BlobStreamReader interface {
ReadStream(ctx context.Context, d digest.Digest) (io.ReadCloser, error)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm just thinking: aren't we going to need an interface like this on the NaiveBuildDirectory side to download files? If so, maybe this should also live in pkg/cas?

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.

2 participants