Skip to content

Commit 53e00f3

Browse files
committed
Address review feedback: fix double-apply, log levels, and Initializing→Stopped
- Fix transition() to use get()+updateAndGet() instead of re-applying the transition function to derive the new state for logging - Log all transitions at INFO, Failed transitions at WARN - Support Initializing→Stopped transition so VCs are not orphaned if shutdown occurs during startup - Assert failure cause on Stopped state in startup failure test Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
1 parent dc141c0 commit 53e00f3

5 files changed

Lines changed: 34 additions & 5 deletions

File tree

kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/KafkaProxy.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,9 @@ private void transitionAllToDraining() {
427427
else if (m.getState() instanceof VirtualClusterLifecycleState.Failed) {
428428
m.stop();
429429
}
430+
else if (m.getState() instanceof VirtualClusterLifecycleState.Initializing) {
431+
m.stop();
432+
}
430433
});
431434
}
432435

kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/VirtualClusterLifecycleManager.java

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,13 +86,16 @@ public void drainComplete() {
8686
}
8787

8888
/**
89-
* Transitions from {@link Failed} to {@link Stopped}.
89+
* Transitions to {@link Stopped} from {@link Failed} or {@link Initializing}.
9090
*/
9191
public void stop() {
9292
transition(current -> {
9393
if (current instanceof Failed s) {
9494
return s.toStopped();
9595
}
96+
if (current instanceof Initializing s) {
97+
return s.toStopped();
98+
}
9699
throw unexpectedState(current, "stop");
97100
});
98101
}
@@ -106,9 +109,11 @@ public String getClusterName() {
106109
}
107110

108111
private void transition(UnaryOperator<VirtualClusterLifecycleState> transitionFn) {
109-
VirtualClusterLifecycleState previous = state.getAndUpdate(transitionFn);
110-
VirtualClusterLifecycleState current = transitionFn.apply(previous);
111-
var logBuilder = (current instanceof Initializing || current instanceof Stopped) ? LOGGER.atInfo() : LOGGER.atDebug();
112+
// Use get() then updateAndGet() rather than getAndUpdate() so we capture
113+
// both the previous and new state without re-applying the transition function.
114+
VirtualClusterLifecycleState previous = state.get();
115+
VirtualClusterLifecycleState current = state.updateAndGet(transitionFn);
116+
var logBuilder = (current instanceof Failed) ? LOGGER.atWarn() : LOGGER.atInfo();
112117
logBuilder
113118
.addKeyValue("virtualCluster", clusterName)
114119
.addKeyValue("from", previous.getClass().getSimpleName())

kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/VirtualClusterLifecycleState.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,14 @@ public Serving toServing() {
4040
public Failed toFailed(Throwable cause) {
4141
return new Failed(cause);
4242
}
43+
44+
/**
45+
* Cluster removed before initialization completed (e.g. proxy shutdown during startup).
46+
* @return the new stopped state
47+
*/
48+
public Stopped toStopped() {
49+
return new Stopped(null);
50+
}
4351
}
4452

4553
/** The proxy has completed setup and is serving traffic for this cluster. */

kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/KafkaProxyLifecycleTest.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,10 @@ void shouldTransitionToStoppedOnStartupFailure() throws Exception {
142142
// then
143143
var manager = proxy.lifecycleManagerFor("demo1");
144144
assertThat(manager).isNotNull();
145-
assertThat(manager.getState()).isInstanceOf(Stopped.class);
145+
assertThat(manager.getState())
146+
.asInstanceOf(org.assertj.core.api.InstanceOfAssertFactories.type(Stopped.class))
147+
.extracting(Stopped::priorFailureCause)
148+
.isInstanceOf(PluginConfigurationException.class);
146149
}
147150
}
148151
}

kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/internal/VirtualClusterLifecycleManagerTest.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,16 @@ void shouldRetainFailureCauseAfterStop() {
116116
.isInstanceOfSatisfying(Stopped.class, stopped -> assertThat(stopped.priorFailureCause()).isSameAs(cause));
117117
}
118118

119+
@Test
120+
void shouldTransitionFromInitializingToStoppedOnShutdown() {
121+
// when
122+
manager.stop();
123+
124+
// then
125+
assertThat(manager.getState())
126+
.isInstanceOfSatisfying(Stopped.class, stopped -> assertThat(stopped.priorFailureCause()).isNull());
127+
}
128+
119129
@Test
120130
void shouldHaveNoPriorFailureCauseWhenStoppedFromDraining() {
121131
// given

0 commit comments

Comments
 (0)