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.

Sign up for GitHub

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

Jump to bottom

[bp/1.24] Kafka + deps #29171

Closed
wants to merge 7 commits into from
Closed

[bp/1.24] Kafka + deps #29171

wants to merge 7 commits into from

Conversation

phlax
Copy link
Member

@phlax phlax commented Aug 21, 2023

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Aug 21, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #29171 was opened by phlax.

see: more, trace.

@phlax phlax added this to the 1.24.11 milestone Aug 21, 2023
@phlax phlax mentioned this pull request Aug 21, 2023
Closed
@phlax
Copy link
Member Author

phlax commented Aug 21, 2023

/retest clang_tidy+asan

@adamkotwasinski
Copy link
Contributor

👍 LGTM looks like a backport of multiple PRs

I'm wondering why clang_tidy is failing for that code while it was not complaining for the original source (maybe we are not running them there as it's contrib?) - kinda busy to investigate now, but if you really want to merge it, just delete this file and this UT section. I'm also wondering if that could be a false positive.... needs further looking.

Or maybe you don't want to backport the whole filter work and just a dependency?

@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Aug 22, 2023
@phlax
Copy link
Member Author

phlax commented Aug 22, 2023

not sure re clang-tidy - im seeing a few clang-tidy issues on branches

Or maybe you don't want to backport the whole filter work and just a dependency?

i dont mind either way - the goal is to update the dependency but as soon as i tried there were conflicts etc - so i picked the commits required to resolve

@adamkotwasinski @phlax
kafka: add shared consumer manager (envoyproxy#24494)
9fc50c7 
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
@adamkotwasinski @phlax
kafka: add support for list-offsets request (envoyproxy#24375)
00bb10b 
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
@adamkotwasinski @phlax
kafka: fetch request (envoyproxy#24606)
ce34ea5 
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
@adamkotwasinski @phlax
kafka: inbound record distributor/store (envoyproxy#24518)
185cb3c 
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
@adamkotwasinski @phlax
kafka: fetch record converter (envoyproxy#25300)
d329702 
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
@adamkotwasinski @phlax
kafka: provide Fetch support, giving mesh-filter stateful consumer pr…
c4b5fb0 
…oxy (envoyproxy#25611)

Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
@adamkotwasinski @phlax
kafka: upgrade dependencies to 3.5.1 (envoyproxy#28932)
432f244 
Signed-off-by: Adam Kotwasinski <adam.kotwasinski@gmail.com>
Signed-off-by: Ryan Northey <ryan@synca.io>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Aug 22, 2023
RyanTheOptimist
RyanTheOptimist reviewed Aug 22, 2023
View reviewed changes
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

drive-by: Why are we back-porting these kafka changes? (I'm sure there's a good reason, just curious what it is)

@phlax
Copy link
Member Author

phlax commented Aug 22, 2023

drive-by: Why are we back-porting these kafka changes? (I'm sure there's a good reason, just curious what it is)

posting previous answer - #28441 (comment)

@RyanTheOptimist
Copy link
Contributor

drive-by: Why are we back-porting these kafka changes? (I'm sure there's a good reason, just curious what it is)

posting previous answer - #28441 (comment)

Thanks. That question was about why we're updating dependencies, i.e. why are we updating the kafka dep. But in this PR we are changing a bunch of Envoy-specific Kafka code. Is that required for some reason? In any case, please update the PR description to explain.

@phlax
Copy link
Member Author

phlax commented Aug 22, 2023

in that case i think the question is answered here #29171 (comment)

@RyanTheOptimist
Copy link
Contributor

in that case i think the question is answered here #29171 (comment)

I see. I'm really surprised that we're back-porting API changes to a branch which is 10 months old. Do we really need to do all of this?

In any case, can you please update the PR description to explain these changes? As CONTRIBUTING.md says, "Your PR description should have details on what the PR does." So maybe you want to say something like:

"Update the Kafka dependencies to 3.5.1. In order to do this, we need to pull in recent changes to Envoy-specific Kafka code to avoid conflicts"

@phlax
Copy link
Member Author

phlax commented Aug 22, 2023

In any case, can you please update the PR description to explain these changes? As CONTRIBUTING.md says, "Your PR description should have details on what the PR does." So maybe you want to say something like:

sure but as explained these backport PRs get rebased - i get that a description is useful but this is not the usual case (the commits contain the descriptions/pr refs etc)

Do we really need to do all of this?

that remains to be seen and is the reason that i separated these updates for the 2 branches that required code changes

@RyanTheOptimist RyanTheOptimist self-assigned this Aug 24, 2023
@RyanTheOptimist
Copy link
Contributor

/wait

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 23, 2023
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist left review comments

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

Assignees

@moderation moderation

@adamkotwasinski adamkotwasinski

@RyanTheOptimist RyanTheOptimist

Labels
deps Approval required for changes to Envoy's external dependencies stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Milestone
1.24.11
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@phlax @adamkotwasinski @moderation @RyanTheOptimist