Add utils on Encodable
#86
Conversation
WalkthroughThis change centralizes encoding utilities by moving Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Utils
participant Encoder
Caller->>Utils: GetEncodedBytes(encodableObject)
Utils->>Encoder: NewEncoder(buffer)
Utils->>Encoder: encodableObject.Encode(encoder)
Utils-->>Caller: Encoded bytes
Caller->>Utils: GetEncodedLength(encodableObject)
Utils->>Utils: GetEncodedBytes(encodableObject)
Utils-->>Caller: len(encoded bytes)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code Graph Analysis (6)internal/cluster_metadata_payload_test.go (1)
protocol/serializer/cluster_metadata_binspec.go (2)
internal/assertions/fetch_response_assertion.go (1)
protocol/utils/utils.go (4)
protocol/api/produce_request.go (2)
protocol/api/record_batch.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (13)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ryan-gang
force-pushed
the
r-add-encodable
branch
from
9d69634 to
ee2a46b
Compare
…gth for encoding length calculation
…es to unify encoding calls across packages
ryan-gang
force-pushed
the
r-add-encodable
branch
from
ee2a46b to
695bfab
Compare
| @@ -233,11 +219,13 @@ type Record struct { | |||
| Headers []RecordHeader | |||
| } | |||
|
|
|||
| func (r Record) Encode(pe *encoder.Encoder) { | |||
| pe.PutVarint(int64(r.getEncodedLength())) // Length placeholder | |||
| func (r Record) EncodeFull(pe *encoder.Encoder) { | |||
There was a problem hiding this comment.
Why does this get an "Encode" and "EncodeFull" instead of just Encode? If it's about satisfying the interface so you can use GetEncodedLength, we're doing this wrong
There was a problem hiding this comment.
Record.Encode() requires the length of the encoded bytes.
To get the encodedLength it needs to call Encode.
So, Encode is dependent on Encode.
So, to implement the interface, add to not repeat the encode logic in 2 different places, this was my solution.
There was a problem hiding this comment.
Don't seem like a good enough reason to pollute a public interface. Haven't thought about how this would work with the encoder thing we're using but if we were using a simple Encode() []byte interface for example, I'd just use a private method encodeContents() and then call something like encodeLength() + encodeContents() within Encode(). That way the public interface is simple and the same as all other "encodable" entities we have.
…or length calculation
Description
getEncodedLengthmethods in favor ofutils.GetEncodedLengthfor consistent encoding length calculation.serializer.GetEncodedByteswithutils.GetEncodedBytesto unify encoding calls across packages.This is part 2 of 2 in a stack made with GitButler:
Encodable#86 👈FetchResponseAssertion#72Summary by CodeRabbit
Refactor
Bug Fixes
New Features