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

fix: message.max.bytes should default to 1048576 not 1 MB #2804

Merged
merged 1 commit into from Feb 27, 2024

Conversation

puellanivis
Copy link
Contributor

@puellanivis puellanivis commented Feb 15, 2024

https://kafka.apache.org/documentation/#brokerconfigs_message.max.bytes

Says this default value should be 1048588, not 1 megabyte (and also weirdly not 1 Mebibyte πŸ€·β€β™€οΈ ) matching these expected values is important, as one language could publish an event that another language would then refuse to republish.

The produced error message ConfigurationError(fmt.Sprintf("Attempt to produce message larger than configured Producer.MaxMessageBytes: %d > %d", size, p.conf.Producer.MaxMessageBytes) is also poorly designed to allow for event handling of that message. It would require an errors.As followed by a strings.HasPrefix(…) which is really not what Go recommends to handle errors (that is, direct string manipulation, which is inherently fragile as the error string should be easily changeable.) This change doesn’t really address this issue, but I wanted to mention it.

@dnwe
Copy link
Collaborator

dnwe commented Feb 21, 2024

@puellanivis thanks for pointing out this discrepancy with current versions of Kafka. However, it is slightly more nuanced as this value has had different default overs the years.

The Java producer since v0.8.2 has always had max.request.size of 1048576 (the base 2 version of 1 MB (MiB))

Up to Kafka v0.8.2 message.max.bytes was 1000000 server side
From Kafka v0.9.0 up to Kafka v2.4 it was then 1000012 (adding 12 bytes for log overhead from KAFKA-506)
From Kafka v2.5 onwards it has been a default 1048588 (the base 2 value of 1 MB (MiB) + 12 bytes for overhead again) based on KAFKA-4203 updating the broker config default to match the Java producer default

@puellanivis
Copy link
Contributor Author

Yeah, I’m totally sure that it’s been through a lot of different values. 😰 Just thinking matching whatever is current is useful.

I suppose in our specific use case, we should probably set it to some value greater than whatever anyone would pick as a default. That way we can avoid β€œchasing a default value”.

@dnwe
Copy link
Collaborator

dnwe commented Feb 21, 2024

I think it probably makes sense for us to match the Java producer, which uses 1048576

@puellanivis
Copy link
Contributor Author

I suppose I need to rebase and add sign offs into the commit anyways… actually, I’d probably be faster to just open a new PR with a signed commit message. πŸ˜‚

@dnwe dnwe changed the title message.max.bytes should be 1048588 not 1 MB fix: message.max.bytes should default to 1048576 not 1 MB Feb 22, 2024
Saram should match the max.request.size default from the Java producer config.

https://kafka.apache.org/documentation/#producerconfigs_max.request.size

Note that the server has its own limit on the record batch size which
defaults to slightly higher than this, but is configurable and can also
be set per topic too.

https://kafka.apache.org/documentation/#brokerconfigs_message.max.bytes

Co-authord-by: Cassondra Foesch <puellanivis@gmail.com>
Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
@dnwe
Copy link
Collaborator

dnwe commented Feb 22, 2024

No worries, I've done the rebase and updated the value to 1024*1024 as discussed

@puellanivis
Copy link
Contributor Author

Is there anything blocking now?

@dnwe
Copy link
Collaborator

dnwe commented Feb 27, 2024

No further holdups, I was just waiting for you to reply with a thumbsup before I went ahead and merged πŸ˜„

@dnwe dnwe merged commit 80b180c into IBM:main Feb 27, 2024
13 checks passed
@puellanivis
Copy link
Contributor Author

πŸ˜† I guess in this case, I could have thumbs upped in a full comment rather than just as a reaction.

@puellanivis puellanivis deleted the patch-2 branch February 27, 2024 21:56
mx-psi pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Apr 2, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/IBM/sarama](https://togithub.com/IBM/sarama) | `v1.43.0`
-> `v1.43.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fIBM%2fsarama/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fIBM%2fsarama/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fIBM%2fsarama/v1.43.0/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fIBM%2fsarama/v1.43.0/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>IBM/sarama (github.com/IBM/sarama)</summary>

### [`v1.43.1`](https://togithub.com/IBM/sarama/releases/tag/v1.43.1):
Version 1.43.1 (2024-03-27)

[Compare
Source](https://togithub.com/IBM/sarama/compare/v1.43.0...v1.43.1)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

#### What's Changed

##### πŸ› Fixes

- fix: message.max.bytes should default to
[`1048576`](https://togithub.com/IBM/sarama/commit/1048576) not 1 MB by
[@&#8203;puellanivis](https://togithub.com/puellanivis) in
[IBM/sarama#2804
- fix: add locking around broker throttle timer to prevent race
condition by [@&#8203;chengsha](https://togithub.com/chengsha) in
[IBM/sarama#2826

##### πŸ“¦ Dependency updates

- chore(deps): bump go.opentelemetry.io/otel/sdk from 1.23.1 to 1.24.0
in /examples/interceptors by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2816
- chore(deps): bump the golang-org-x group with 1 update by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2825
- chore(deps): bump github.com/stretchr/testify from 1.8.4 to 1.9.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2822
- chore(deps): bump
go.opentelemetry.io/otel/exporters/stdout/stdoutmetric from 1.23.1 to
1.24.0 in /examples/interceptors by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2815

#### New Contributors

- [@&#8203;chengsha](https://togithub.com/chengsha) made their first
contribution in
[IBM/sarama#2826

**Full Changelog**:
IBM/sarama@v1.43.0...v1.43.1

</details>

---

### Configuration

πŸ“… **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

β™» **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

πŸ”• **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
ycombinator pushed a commit to ycombinator/opentelemetry-collector-contrib that referenced this pull request Apr 9, 2024
…etry#32091)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/IBM/sarama](https://togithub.com/IBM/sarama) | `v1.43.0`
-> `v1.43.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fIBM%2fsarama/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fIBM%2fsarama/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fIBM%2fsarama/v1.43.0/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fIBM%2fsarama/v1.43.0/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>IBM/sarama (github.com/IBM/sarama)</summary>

### [`v1.43.1`](https://togithub.com/IBM/sarama/releases/tag/v1.43.1):
Version 1.43.1 (2024-03-27)

[Compare
Source](https://togithub.com/IBM/sarama/compare/v1.43.0...v1.43.1)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

#### What's Changed

##### πŸ› Fixes

- fix: message.max.bytes should default to
[`1048576`](https://togithub.com/IBM/sarama/commit/1048576) not 1 MB by
[@&open-telemetry#8203;puellanivis](https://togithub.com/puellanivis) in
[IBM/sarama#2804
- fix: add locking around broker throttle timer to prevent race
condition by [@&open-telemetry#8203;chengsha](https://togithub.com/chengsha) in
[IBM/sarama#2826

##### πŸ“¦ Dependency updates

- chore(deps): bump go.opentelemetry.io/otel/sdk from 1.23.1 to 1.24.0
in /examples/interceptors by
[@&open-telemetry#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2816
- chore(deps): bump the golang-org-x group with 1 update by
[@&open-telemetry#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2825
- chore(deps): bump github.com/stretchr/testify from 1.8.4 to 1.9.0 by
[@&open-telemetry#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2822
- chore(deps): bump
go.opentelemetry.io/otel/exporters/stdout/stdoutmetric from 1.23.1 to
1.24.0 in /examples/interceptors by
[@&open-telemetry#8203;dependabot](https://togithub.com/dependabot) in
[IBM/sarama#2815

#### New Contributors

- [@&open-telemetry#8203;chengsha](https://togithub.com/chengsha) made their first
contribution in
[IBM/sarama#2826

**Full Changelog**:
IBM/sarama@v1.43.0...v1.43.1

</details>

---

### Configuration

πŸ“… **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

β™» **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

πŸ”• **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants