Skip to content

kafka: new deserializers#17202

Merged
mattklein123 merged 4 commits intoenvoyproxy:mainfrom
adamkotwasinski:kafka-new-deserializers
Jul 15, 2021
Merged

kafka: new deserializers#17202
mattklein123 merged 4 commits intoenvoyproxy:mainfrom
adamkotwasinski:kafka-new-deserializers

Conversation

@adamkotwasinski
Copy link
Contributor

@adamkotwasinski adamkotwasinski commented Jun 30, 2021

Commit Message: kafka: new deserializers
Additional Description: new deserializers for kafka variable-length encoded int32 & int64 (called varint, varlong there). These deserializers will be used in kafka-mesh-filter to decode internals of received kafka produce request objects so we can get the real messages out of the envelope (like here), and forward them to kafka producers.
Risk Level: Low
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
@adamkotwasinski adamkotwasinski marked this pull request as ready for review June 30, 2021 20:28
@lizan
Copy link
Member

lizan commented Jul 2, 2021

/assign-from @envoyproxy/first-pass-reviewers

@repokitteh-read-only
Copy link

@envoyproxy/first-pass-reviewers assignee is @junr03

🐱

Caused by: a #17202 (comment) was created by @lizan.

see: more, trace.

bool ready_ = false;
};

class VarInt32Deserializer : public Deserializer<int32_t> {
Copy link
Member

Choose a reason for hiding this comment

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

should we add class comments like the other classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

VarUInt32Deserializer varuint32_deserializer_;
};

class VarInt64Deserializer : public Deserializer<int64_t> {
Copy link
Member

Choose a reason for hiding this comment

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

Do we eventually need a VarUInt64Deserializer that this deserializer can be written on top off like the above?

Copy link
Contributor Author

@adamkotwasinski adamkotwasinski Jul 8, 2021

Choose a reason for hiding this comment

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

EXPECT_THROW_WITH_REGEX(testee.feed(data), EnvoyException, "is too long");
}

// Missing tests (chunks).
Copy link
Member

Choose a reason for hiding this comment

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

is this a todo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - sorry about that! More tests have been added.

@junr03
Copy link
Member

junr03 commented Jul 8, 2021

/wait

Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17202 (comment) was created by @adamkotwasinski.

see: more, trace.

Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

thanks for the changes will move up to @mattklein123 for final pass.

@junr03 junr03 assigned mattklein123 and unassigned junr03 Jul 14, 2021
@mattklein123 mattklein123 merged commit 90a291d into envoyproxy:main Jul 15, 2021
@adamkotwasinski adamkotwasinski deleted the kafka-new-deserializers branch July 15, 2021 17:03
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
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.

4 participants