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

k/topics: make write caching configs settable at creation #17515

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

bharathv
Copy link
Contributor

@bharathv bharathv commented Mar 31, 2024

Allows having write caching property overrides at topic creation time.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Mar 31, 2024

new failures in https://buildkite.com/redpanda/redpanda/builds/47122#018e924b-6194-4a17-aae7-de9fb3938d38:

"rptest.tests.mirror_maker_test.TestMirrorMakerService.test_consumer_group_mirroring.source_type=kafka"

new failures in https://buildkite.com/redpanda/redpanda/builds/47122#018e923b-431e-4887-879c-a7633839d0c2:

"rptest.tests.mirror_maker_test.TestMirrorMakerService.test_consumer_group_mirroring.source_type=kafka"

new failures in https://buildkite.com/redpanda/redpanda/builds/47122#018e923b-4319-4618-b660-198fdc0a633c:

"rptest.tests.mirror_maker_test.TestMirrorMakerService.test_simple_end_to_end.source_type=kafka"

new failures in https://buildkite.com/redpanda/redpanda/builds/47122#018e924b-618f-482e-8b7b-ba648d70a0f2:

"rptest.tests.mirror_maker_test.TestMirrorMakerService.test_simple_end_to_end.source_type=kafka"

new failures in https://buildkite.com/redpanda/redpanda/builds/47130#018e95c7-31e6-4929-91cf-8a0e308115ee:

"rptest.tests.mirror_maker_test.TestMirrorMakerService.test_simple_end_to_end.source_type=kafka"

new failures in https://buildkite.com/redpanda/redpanda/builds/47130#018e95c7-31eb-45d4-991e-49536258b1c6:

"rptest.tests.mirror_maker_test.TestMirrorMakerService.test_consumer_group_mirroring.source_type=kafka"

ballard26
ballard26 previously approved these changes Mar 31, 2024
Copy link
Contributor

@ballard26 ballard26 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the quick fix!

@emaxerrno
Copy link
Contributor

Should the name be “write cache” vs “write caching” I don’t see other properties having “ing”

@mattschumpert - this can be in a separate change

Jitter was implemented but the actual waiter source was not updated to
use the jittered duration.
@dotnwat dotnwat requested a review from nvartolomei April 1, 2024 20:27
With very large flush delays (eg: ns::max()), the background
flusher CV runs into an UB because of a seastar bug that doesn't
accept large timeouts supposedly used as indefinitel waits. This commit
worksaround by converting the delay into larger duration type (thus
smaller integral value) avoiding the overflow.
Missed this in the original patch series that added these properties.
Certain kafka clients set flush.ms default to long_max which oveflows
during serde serialization as it requires that any duration does not
exceed nanoseconds::max(). If we encounter such a value for flush.ms
during topic creation it seems logical to clamp the value to
nanoseconds::max(), rather than failing the topic creation for
better user experience and backward compatibility.

This commit clamps flush.ms to nanoseconds::max() during topic creation
and alteration.
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Apr 1, 2024

Copy link
Contributor

@ballard26 ballard26 left a comment

Choose a reason for hiding this comment

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

LGTM

@piyushredpanda
Copy link
Contributor

Good to merge this, @bharathv ? We can trigger a build after we merge this if @ballard26 needs this change.

@bharathv bharathv merged commit b586a0d into redpanda-data:dev Apr 2, 2024
18 checks passed
@mattschumpert
Copy link

@emaxerrno a bunch of properties do end in 'ing' related to data balancing, controller log rate limiting, principal mapping etc.

I don't have a strong opinion here, but given it is one cache per broker and this is a global cluster config, I have a small concern that using 'cache' may make people think of 1 shared cache per cluster, especially in certain cloud contexts where the broker count is less in the foreground. So, a slight preference to keep the general capability name with 'ing'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants