Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ private void close(long timeout, TimeUnit timeUnit, boolean swallowException) {

log.info("Closing the Kafka producer with timeoutMillis = {} ms.", timeUnit.toMillis(timeout));
// this will keep track of the first encountered exception
AtomicReference<Throwable> firstException = new AtomicReference<Throwable>();
AtomicReference<Throwable> firstException = new AtomicReference<>();
boolean invokedFromCallback = Thread.currentThread() == this.ioThread;
if (timeout > 0) {
if (invokedFromCallback) {
Expand Down
37 changes: 21 additions & 16 deletions streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,12 @@ public boolean isValidTransition(final State newState) {
return validTransitions.contains(newState.ordinal());
}
}

private final Object stateLock = new Object();

private volatile State state = State.CREATED;
private StateListener stateListener = null;

private KafkaStreams.StateListener stateListener = null;


/**
Expand All @@ -208,25 +212,26 @@ public interface StateListener {
}

/**
* An app can set a single {@link StateListener} so that the app is notified when state changes.
* An app can set a single {@link KafkaStreams.StateListener} so that the app is notified when state changes.
* @param listener a new state listener
*/
public void setStateListener(final StateListener listener) {
public void setStateListener(final KafkaStreams.StateListener listener) {
stateListener = listener;
}

private synchronized void setState(final State newState) {
final State oldState = state;
if (!state.isValidTransition(newState)) {
log.warn("{} Unexpected state transition from {} to {}.", logPrefix, oldState, newState);
} else {
log.info("{} State transition from {} to {}.", logPrefix, oldState, newState);
}

state = newState;

if (stateListener != null) {
stateListener.onChange(state, oldState);
private void setState(final State newState) {
synchronized (this.stateLock) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: remove this

final State oldState = state;
if (!state.isValidTransition(newState)) {
log.warn(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: fix line break

"{} Unexpected state transition from {} to {}.", logPrefix, oldState, newState);
} else {
log.info("{} State transition from {} to {}.", logPrefix, oldState, newState);
}
state = newState;
if (stateListener != null) {
stateListener.onChange(state, oldState);
}
}
}

Expand All @@ -248,7 +253,7 @@ public synchronized State state() {
return Collections.unmodifiableMap(metrics.metrics());
}

private class StreamStateListener implements StreamThread.StateListener {
private final class StreamStateListener implements StreamThread.StateListener {
@Override
public synchronized void onChange(final StreamThread thread,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dguy @enothereska This synchronized here seems suspicious. Is it really the aim to synchronize on the listener instance when updating variables like threadState? Seems like a bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like there should only be a single StreamStateListener and threadState should be a member.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @dguy. That makes sense. @original-brownbear, maybe you can do a follow-up that does that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ijuma put it on my todos, will get to this in about ~12h :)

final StreamThread.State newState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public boolean isValidTransition(final State newState) {
}

private volatile State state = State.NOT_RUNNING;
private StateListener stateListener = null;
private StreamThread.StateListener stateListener = null;

/**
* Listen to state change events
Expand All @@ -141,10 +141,10 @@ public interface StateListener {
}

/**
* Set the {@link StateListener} to be notified when state changes. Note this API is internal to
* Set the {@link StreamThread.StateListener} to be notified when state changes. Note this API is internal to
* Kafka Streams and is not intended to be used by an external application.
*/
public void setStateListener(final StateListener listener) {
public void setStateListener(final StreamThread.StateListener listener) {
this.stateListener = listener;
}

Expand Down Expand Up @@ -463,7 +463,7 @@ private RuntimeException performOnAllTasks(final AbstractTaskAction action,
action.apply(task);
} catch (RuntimeException t) {
log.error("{} Failed while executing {} {} due to {}: ",
StreamThread.this.logPrefix,
this.logPrefix,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: also remove this

task.getClass().getSimpleName(),
task.id(),
exceptionMessage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ public void testCannotCleanupWhileRunning() throws Exception {
try {
streams.cleanUp();
} catch (final IllegalStateException e) {
Assert.assertEquals("Cannot clean up while running.", e.getMessage());
assertEquals("Cannot clean up while running.", e.getMessage());
throw e;
} finally {
streams.close();
Expand Down