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

pkg/agent: trigger batch before grpc message size overflow #836

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

heylongdacoder
Copy link
Contributor

Fixes: #203

Signed-off-by: heylongdacoder heylongdacoder@gmail.com

Fixes: parca-dev#203

Signed-off-by: heylongdacoder <heylongdacoder@gmail.com>
@heylongdacoder heylongdacoder requested a review from a team as a code owner September 24, 2022 08:21
@heylongdacoder
Copy link
Contributor Author

Hello, I don't have much experience on writing concurrent code, might make some mistake. Thanks for guidance in advance. :D

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Looking great!

I think we can simplify this code and make it more maintainable. We can get rid of one of the mutex locking/unlocking and remove one of the channels if we can make batch method to receive a request (probably we should rename it to just send after this change). With this flow will be easier and less error prone.

What do you think?

mtx *sync.RWMutex
series []*profilestorepb.RawProfileSeries
triggerBatch chan struct{}
batchDone chan struct{}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of batchDone in here.

select {
case b.batchDone <- struct{}{}:
default:
}
}
}

func (b *BatchWriteClient) batch(ctx context.Context) error {
Copy link
Member

@kakkoyun kakkoyun Oct 10, 2022

Choose a reason for hiding this comment

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

Suggested change
func (b *BatchWriteClient) batch(ctx context.Context) error {
func batch(ctx context.Context, r *profilestorepb.WriteRawRequest) error {

This method can receive the request and by this way we can get rid of the lock in this method if I'm not mistaken. This method could also omit the write client as a receiver if it's the case.

@kakkoyun kakkoyun added this to the v0.10.0 milestone Oct 12, 2022
@kakkoyun
Copy link
Member

@heylongdacoder Let me know if you need any help with this.

@heylongdacoder
Copy link
Contributor Author

@kakkoyun thanks for the review and guidance! yes I need some help. I am not sure how would it works if we removed the batchDone channel. If we removed batchDone, I think there is a chance where WriteRaw function will continue to append data to the overflowed writeRawRequest(https://github.com/parca-dev/parca-agent/pull/836/files#diff-7276397f96515c11ca49f4f72de2237d63936ab985888a5a4fe07ccf8c371d42R166) before the batch function could send it.

Currently I got another idea to avoid the creation of these triggerBatch and batchDone channels. Instead of a writeRawRequest, what about we create a slices of writeRawRequest and a writeRawRequestIndex? WriteRaw function will write to writeRawRequest[writeRawRequestIndex] and when the current one is going to be overflowed, then we write to writeRawRequest[writeRawRequestIndex+1]. Then the batch function either loop through the slices of writeRawRequest to fire gRPC request sequentially or create goroutine for each element to fire gRPC request. Do you think this would work and is this a better approach?

@kakkoyun kakkoyun removed this from the v0.10.0 milestone Nov 1, 2022
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.

grpc: received message larger than max
2 participants