kfaraz commented on code in PR #19700:
URL: https://github.com/apache/druid/pull/19700#discussion_r3613772069
##########
extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java:
##########
@@ -59,10 +60,15 @@ public void testRequireRestartWhenRabbitTaskTuningChanges()
{
final RabbitStreamSupervisorSpec oldSpec = supervisorSpec(tuningConfig(15,
16, 17));
- // requireRestart is invoked on the running (old) spec with the proposed
spec as argument.
- Assert.assertTrue(oldSpec.requireRestart(supervisorSpec(tuningConfig(20,
16, 17))));
- Assert.assertTrue(oldSpec.requireRestart(supervisorSpec(tuningConfig(15,
20, 17))));
- Assert.assertTrue(oldSpec.requireRestart(supervisorSpec(tuningConfig(15,
16, 20))));
+ // getActionOnUpdateTo is invoked on the running (old) spec with the
proposed spec as argument.
+ Assert.assertTrue(requiresRestart(oldSpec, supervisorSpec(tuningConfig(20,
16, 17))));
Review Comment:
Maybe we should verify the actual value returned by the
`getActionOnUpdateTo` method rather than converting it to boolean using a
private method.
##########
indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java:
##########
@@ -1511,7 +1513,12 @@ public void
testRequireRestart_tuningConfigChangeRestarts()
final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build();
final SeekableStreamSupervisorSpec newSpec =
seed.toBuilder().tuningConfig(EasyMock.mock(SeekableStreamSupervisorTuningConfig.class)).build();
- Assert.assertTrue(oldSpec.requireRestart(newSpec));
+ Assert.assertTrue(requiresRestart(oldSpec, newSpec));
+ }
+
+ private static boolean requiresRestart(SupervisorSpec oldSpec,
SupervisorSpec newSpec)
Review Comment:
Nit: we can probably remove this method and verify the actual values
returned by the `getActionOnUpdateTo` method in the tests themselves.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -197,7 +197,16 @@ public SupervisorSpecUpdateResult
createOrUpdateAndStartSupervisor(
if (!skipRestartIfUnmodified) {
// Always stop/recreate, persisting whenever the spec actually changed
(or is new).
final boolean specChanged = isSpecChangedAndValidated(spec);
- final SupervisorSpec existingSpec =
possiblyStopAndRemoveSupervisorInternal(spec.getId(), false);
+ final Pair<Supervisor, SupervisorSpec> current =
supervisors.get(spec.getId());
Review Comment:
The `skipRestartIfUnmodified` flag is false by default.
So, in that case, I wonder if we shouldn't just retain current behaviour of
always restarting the tasks.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -197,7 +197,16 @@ public SupervisorSpecUpdateResult
createOrUpdateAndStartSupervisor(
if (!skipRestartIfUnmodified) {
// Always stop/recreate, persisting whenever the spec actually changed
(or is new).
final boolean specChanged = isSpecChangedAndValidated(spec);
- final SupervisorSpec existingSpec =
possiblyStopAndRemoveSupervisorInternal(spec.getId(), false);
+ final Pair<Supervisor, SupervisorSpec> current =
supervisors.get(spec.getId());
+ final SupervisorSpec currentSpec = current == null ? null :
current.rhs;
+ final boolean shouldTerminateTasks =
+ currentSpec == null
+ || currentSpec.getActionOnUpdateTo(spec) ==
SupervisorSpecUpdateAction.RESTART_SUPERVISOR_AND_TASKS;
+ final SupervisorSpec existingSpec =
possiblyStopAndRemoveSupervisorInternal(
+ spec.getId(),
+ false,
+ shouldTerminateTasks
+ );
spec.merge(existingSpec);
createAndStartSupervisorInternal(spec, specChanged);
return SupervisorSpecUpdateResult.of(specChanged, true);
Review Comment:
Change the 2nd argument of `SupervisorSpecUpdateResult.of()` method to be
`SupervisorSpecUpdateAction` instead of a boolean.
Also, add a new field `restartedTasks` inside `SupervisorSpecUpdateResult`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
