-
Notifications
You must be signed in to change notification settings - Fork 15.3k
KAFKA-20491: Add uncommitted bytes limit #22597
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,7 @@ public class GlobalStreamThread extends Thread { | |
| private volatile long fetchDeadlineClientInstanceId = -1; | ||
| private volatile KafkaFutureImpl<Uuid> clientInstanceIdFuture = new KafkaFutureImpl<>(); | ||
| private final CountDownLatch initializationLatch = new CountDownLatch(1); | ||
| private volatile long maxUncommittedBytes; | ||
|
|
||
| /** | ||
| * The states that the global stream thread can be in | ||
|
|
@@ -202,6 +203,7 @@ public GlobalStreamThread(final ProcessorTopology topology, | |
| final Consumer<byte[], byte[]> globalConsumer, | ||
| final StateDirectory stateDirectory, | ||
| final long cacheSizeBytes, | ||
| final long maxUncommittedBytes, | ||
| final StreamsMetricsImpl streamsMetrics, | ||
| final Time time, | ||
| final String threadClientId, | ||
|
|
@@ -221,6 +223,7 @@ public GlobalStreamThread(final ProcessorTopology topology, | |
| this.stateRestoreListener = stateRestoreListener; | ||
| this.streamsUncaughtExceptionHandler = streamsUncaughtExceptionHandler; | ||
| this.cacheSize = new AtomicLong(-1L); | ||
| this.maxUncommittedBytes = maxUncommittedBytes; | ||
| } | ||
|
|
||
| static class StateConsumer { | ||
|
|
@@ -259,6 +262,14 @@ void pollAndUpdate() { | |
| stateMaintainer.maybeCheckpoint(); | ||
| } | ||
|
|
||
| void flushState() { | ||
| stateMaintainer.flushState(); | ||
| } | ||
|
|
||
| long approximateNumUncommittedBytes() { | ||
| return stateMaintainer.approximateNumUncommittedBytes(); | ||
| } | ||
|
|
||
| public void close(final boolean wipeStateStore) throws IOException { | ||
| try { | ||
| globalConsumer.close(); | ||
|
|
@@ -301,6 +312,13 @@ public void run() { | |
| } | ||
| stateConsumer.pollAndUpdate(); | ||
|
|
||
| final long uncommittedLimit = maxUncommittedBytes; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to preempt some feedback here and clarify why we copy The reason is that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get that but I'm wondering if we used
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so. The crucial thing here is that the following few lines use a consistent value, so we would still need to copy it out of the AtomicLong into a temporary variable. |
||
| if (uncommittedLimit > 0 | ||
| && stateConsumer.approximateNumUncommittedBytes() > uncommittedLimit) { | ||
| log.debug("Committing global state: uncommitted bytes exceeded {}", uncommittedLimit); | ||
| stateConsumer.flushState(); | ||
| } | ||
|
|
||
| if (fetchDeadlineClientInstanceId != -1) { | ||
| if (fetchDeadlineClientInstanceId >= time.milliseconds()) { | ||
| try { | ||
|
|
@@ -372,6 +390,10 @@ public void resize(final long cacheSize) { | |
| this.cacheSize.set(cacheSize); | ||
| } | ||
|
|
||
| public void resizeMaxUncommittedBytes(final long maxUncommittedBytes) { | ||
| this.maxUncommittedBytes = maxUncommittedBytes; | ||
| } | ||
|
|
||
| private StateConsumer initialize() { | ||
| StateConsumer stateConsumer = null; | ||
| try { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may seem strange to floor
numStreamThreadsat0, sinceStreamsConfig.NUM_STREAM_THREADShas a minimum of1, but it's actually possible for there to be 0 stream threads... If youremoveStreamThreadthe last remaining stream thread; or if youreplaceStreamThread, there's a narrow window where the old thread has been removed and the new one has not registered yet.