Skip to content
This repository has been archived by the owner on Oct 17, 2018. It is now read-only.

Add message size limit to encoder and decoder #18

Merged
merged 1 commit into from
May 23, 2018
Merged

Add message size limit to encoder and decoder #18

merged 1 commit into from
May 23, 2018

Conversation

cw9
Copy link
Contributor

@cw9 cw9 commented May 22, 2018

No description provided.

Copy link
Contributor

@xichen2020 xichen2020 left a comment

Choose a reason for hiding this comment

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

LGTM with a comment

}
}

func (e *encoder) Encode(m Marshaler) error {
size := m.Size()
if size > e.maxMessageSize {
return fmt.Errorf("message size %d is larger than maximum supported size %d", size, e.maxMessageSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Food for thought: it might make sense to distinguish different types of errors so that callers can selectively choose to reset the encoder/decoder as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could return a different error here and handle it differently in the writer, but I can just drop the data, and the user will have no way to know why it's being dropped and might retry again. I think it would be better to have the Buffer to reject the oversized data and return an error with the reason right in the beginning and save all the future work like trying to send it to all the consumers and then being dropped after the error in encoder, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly, perhaps it should be checked first thing in the producer API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sg

@cw9 cw9 merged commit 6e0c9a5 into master May 23, 2018
@cw9 cw9 deleted the chao-datasize branch May 23, 2018 14:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants