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

ref(kafka): Remove ShardedProducer #3415

Merged
merged 9 commits into from
Apr 15, 2024

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Apr 12, 2024

This PR removes the ShardedProducer and it's related configuration params since it's not used anymore.

Closes: #3339

@iambriccardo iambriccardo marked this pull request as ready for review April 12, 2024 09:29
@iambriccardo iambriccardo requested a review from a team as a code owner April 12, 2024 09:29
/// Sends the payload to the correct producer for the current topic.
fn send(
&self,
organization_id: u64,
_organization_id: u64,
Copy link
Member Author

Choose a reason for hiding this comment

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

This parameter is not needed anymore but it's piped through so much code that I was wondering if we should keep it for future use. If not, we can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it.

CHANGELOG.md Outdated
@@ -44,6 +44,7 @@
- Route spans according to trace_id. ([#3387](https://github.com/getsentry/relay/pull/3387))
- Log span when encountering a validation error. ([#3401](https://github.com/getsentry/relay/pull/3401))
- Optionally skip normalization. ([#3377](https://github.com/getsentry/relay/pull/3377))
- Remove `ShardedProducer`. ([#3415](https://github.com/getsentry/relay/pull/3415))
Copy link
Member

Choose a reason for hiding this comment

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

Can we mark this as a breaking change and put it under the other breaking change in **Features**?

Copy link
Member

Choose a reason for hiding this comment

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

@iambriccardo can you also mark it as a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do we mark it as a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nvm just saw

Copy link
Member

Choose a reason for hiding this comment

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

And now move it below the other breaking change :p.

Yeah it's not very fancy, we should probably at some point rework our changelog a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like we should just create a set of rules that one should follow since right now it's a bit copy pasty.

/// Sends the payload to the correct producer for the current topic.
fn send(
&self,
organization_id: u64,
_organization_id: u64,
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it.

@iambriccardo iambriccardo requested a review from jjbayer April 12, 2024 11:14
@iambriccardo
Copy link
Member Author

I made sure that no active configs in prod are using the sharded configuration.

@iambriccardo iambriccardo merged commit f02d3b9 into master Apr 15, 2024
20 checks passed
@iambriccardo iambriccardo deleted the riccardo/ref/remove-sharded-producer branch April 15, 2024 06:40
jjbayer added a commit that referenced this pull request Apr 19, 2024
Remove `SingleProducer` and `ProducerInner` types.

Follow-up to #3415.

---------

Co-authored-by: David Herberth <david.herberth@sentry.io>
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.

Remove ShardedProducer
3 participants