From fa855efb8a9c06e7d4f55244cb3deb27bb56148d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesse=20Tu=C4=9Flu?= Date: Mon, 1 Jun 2026 14:45:09 -0700 Subject: [PATCH 1/4] refactor: make supervisor diff+restart algorithm smarter on re-submission --- .../RabbitStreamIOConfigBuilder.java | 91 +++++ .../RabbitStreamSupervisorIOConfig.java | 28 ++ .../RabbitStreamSupervisorSpec.java | 38 ++ .../RabbitStreamSupervisorTuningConfig.java | 24 ++ .../RabbitStreamSupervisorIOConfigTest.java | 35 ++ .../RabbitStreamSupervisorTest.java | 139 +++---- .../supervisor/KafkaIOConfigBuilder.java | 45 ++- .../supervisor/KafkaSupervisorIOConfig.java | 39 ++ .../kafka/supervisor/KafkaSupervisorSpec.java | 38 ++ .../KafkaSupervisorTuningConfig.java | 24 ++ .../KafkaSupervisorIOConfigTest.java | 41 ++ .../kafka/supervisor/KafkaSupervisorTest.java | 240 ++++------- .../supervisor/KinesisIOConfigBuilder.java | 125 ++++++ .../supervisor/KinesisSupervisorIOConfig.java | 39 ++ .../supervisor/KinesisSupervisorSpec.java | 47 +++ .../KinesisSupervisorTuningConfig.java | 35 ++ .../kinesis/KinesisSamplerSpecTest.java | 60 +-- .../KinesisSupervisorIOConfigTest.java | 36 ++ .../supervisor/KinesisSupervisorTest.java | 371 +++++++----------- .../supervisor/SupervisorManager.java | 85 +++- .../supervisor/SupervisorResource.java | 3 + .../seekablestream/supervisor/IdleConfig.java | 20 + .../supervisor/LagAggregator.java | 14 + .../SeekableStreamSupervisorIOConfig.java | 68 ++++ ...SeekableStreamSupervisorIngestionSpec.java | 25 ++ .../SeekableStreamSupervisorSpec.java | 176 +++++++++ .../supervisor/SupervisorIOConfigBuilder.java | 94 ++++- .../supervisor/SupervisorManagerTest.java | 64 +++ .../supervisor/SupervisorResourceTest.java | 8 + .../SeekableStreamSupervisorIOConfigTest.java | 42 ++ .../SeekableStreamSupervisorSpecTest.java | 317 ++++++++------- .../SeekableStreamSupervisorStateTest.java | 280 +++++-------- .../SeekableStreamSupervisorTestBase.java | 83 ++-- .../overlord/supervisor/SupervisorSpec.java | 19 + 34 files changed, 1883 insertions(+), 910 deletions(-) create mode 100644 extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamIOConfigBuilder.java rename extensions-core/kafka-indexing-service/src/{test => main}/java/org/apache/druid/indexing/kafka/supervisor/KafkaIOConfigBuilder.java (68%) create mode 100644 extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisIOConfigBuilder.java rename indexing-service/src/{test => main}/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java (54%) diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamIOConfigBuilder.java b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamIOConfigBuilder.java new file mode 100644 index 000000000000..729ff7c9097d --- /dev/null +++ b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamIOConfigBuilder.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.indexing.rabbitstream.supervisor; + +import org.apache.druid.indexing.seekablestream.supervisor.SupervisorIOConfigBuilder; + +import java.util.Map; + +/** + * Builder for {@link RabbitStreamSupervisorIOConfig}. + */ +public class RabbitStreamIOConfigBuilder + extends SupervisorIOConfigBuilder +{ + private String uri; + private Map consumerProperties; + private Long pollTimeout; + + public RabbitStreamIOConfigBuilder withUri(String uri) + { + this.uri = uri; + return this; + } + + public RabbitStreamIOConfigBuilder withConsumerProperties(Map consumerProperties) + { + this.consumerProperties = consumerProperties; + return this; + } + + public RabbitStreamIOConfigBuilder withPollTimeout(Long pollTimeout) + { + this.pollTimeout = pollTimeout; + return this; + } + + /** + * Populates this builder (base and Rabbit-specific fields) from an existing config. + */ + public RabbitStreamIOConfigBuilder copyFrom(RabbitStreamSupervisorIOConfig io) + { + copyFromBase(io); + this.uri = io.getUri(); + this.consumerProperties = io.getConsumerProperties(); + this.pollTimeout = io.getPollTimeout(); + return this; + } + + @Override + public RabbitStreamSupervisorIOConfig build() + { + return new RabbitStreamSupervisorIOConfig( + stream, + uri, + inputFormat, + replicas, + taskCount, + taskDuration, + consumerProperties, + autoScalerConfig, + pollTimeout, + startDelay, + period, + completionTimeout, + useEarliestSequenceNumber, + lateMessageRejectionPeriod, + earlyMessageRejectionPeriod, + lateMessageRejectionStartDateTime, + stopTaskCount, + serverPriorityToReplicas, + boundedStreamConfig + ); + } +} diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfig.java b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfig.java index 20c4e92988b6..dd85d1dcfebc 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfig.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfig.java @@ -33,6 +33,7 @@ import javax.annotation.Nullable; import java.util.Map; +import java.util.Objects; public class RabbitStreamSupervisorIOConfig extends SeekableStreamSupervisorIOConfig { @@ -145,4 +146,31 @@ public String toString() '}'; } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (!super.equals(o)) { + return false; + } + RabbitStreamSupervisorIOConfig that = (RabbitStreamSupervisorIOConfig) o; + return pollTimeout == that.pollTimeout + && Objects.equals(uri, that.uri) + && Objects.equals(consumerProperties, that.consumerProperties); + } + + @Override + public int hashCode() + { + return Objects.hash(super.hashCode(), uri, consumerProperties, pollTimeout); + } + + @Override + public RabbitStreamIOConfigBuilder toBuilder() + { + return new RabbitStreamIOConfigBuilder().copyFrom(this); + } + } diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java index 4763a949a615..6097c186fb27 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java @@ -216,4 +216,42 @@ public String toString() ", suspend=" + isSuspended() + '}'; } + + @Override + public Builder toBuilder() + { + return new Builder().copyFrom(this); + } + + public static class Builder extends SeekableStreamSupervisorSpec.Builder + { + @Override + protected Builder self() + { + return this; + } + + @Override + public RabbitStreamSupervisorSpec build() + { + return new RabbitStreamSupervisorSpec( + id, + null, + dataSchema, + (RabbitStreamSupervisorTuningConfig) tuningConfig, + (RabbitStreamSupervisorIOConfig) ioConfig, + context, + suspended, + taskStorage, + taskMaster, + indexerMetadataStorageCoordinator, + (RabbitStreamIndexTaskClientFactory) indexTaskClientFactory, + mapper, + emitter, + monitorSchedulerConfig, + rowIngestionMetersFactory, + supervisorStateManagerConfig + ); + } + } } diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfig.java b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfig.java index a2667026fffd..28a4f55f0d72 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfig.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfig.java @@ -29,6 +29,7 @@ import org.joda.time.Period; import javax.annotation.Nullable; +import java.util.Objects; public class RabbitStreamSupervisorTuningConfig extends RabbitStreamIndexTaskTuningConfig implements SeekableStreamSupervisorTuningConfig @@ -185,6 +186,29 @@ public Duration getOffsetFetchPeriod() return offsetFetchPeriod; } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (!super.equals(o)) { + return false; + } + RabbitStreamSupervisorTuningConfig that = (RabbitStreamSupervisorTuningConfig) o; + return Objects.equals(workerThreads, that.workerThreads) + && Objects.equals(chatRetries, that.chatRetries) + && Objects.equals(httpTimeout, that.httpTimeout) + && Objects.equals(shutdownTimeout, that.shutdownTimeout) + && Objects.equals(offsetFetchPeriod, that.offsetFetchPeriod); + } + + @Override + public int hashCode() + { + return Objects.hash(super.hashCode(), workerThreads, chatRetries, httpTimeout, shutdownTimeout, offsetFetchPeriod); + } + @Override public String toString() { diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java index 0dca7f084f0a..57e5e9f7fece 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java @@ -26,6 +26,7 @@ import org.apache.druid.jackson.DefaultObjectMapper; import org.hamcrest.CoreMatchers; import org.joda.time.Duration; +import org.joda.time.Period; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; @@ -212,4 +213,38 @@ public void testUnboundedModeByDefault() throws Exception Assert.assertNull(config.getBoundedStreamConfig()); } + private static RabbitStreamIOConfigBuilder ioConfigBuilder() + { + return new RabbitStreamIOConfigBuilder() + .withStream("stream") + .withUri("rabbit://localhost") + .withReplicas(1) + .withTaskCount(2) + .withTaskDuration(new Period("PT1H")); + } + + @Test + public void testEqualsAndHashCode() + { + final RabbitStreamSupervisorIOConfig config = ioConfigBuilder().build(); + Assert.assertEquals(config, ioConfigBuilder().build()); + Assert.assertEquals(config.hashCode(), ioConfigBuilder().build().hashCode()); + Assert.assertNotEquals(config, null); + Assert.assertNotEquals(config, "not an io config"); + Assert.assertNotEquals(config, ioConfigBuilder().withUri("rabbit://other").build()); + Assert.assertNotEquals(config, ioConfigBuilder().withReplicas(9).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withTaskCount(9).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withPollTimeout(999L).build()); + } + + @Test + public void testTuningConfigEqualsAndHashCode() + { + final RabbitStreamSupervisorTuningConfig config = RabbitStreamSupervisorTuningConfig.defaultConfig(); + Assert.assertEquals(config, RabbitStreamSupervisorTuningConfig.defaultConfig()); + Assert.assertEquals(config.hashCode(), RabbitStreamSupervisorTuningConfig.defaultConfig().hashCode()); + Assert.assertNotEquals(config, null); + Assert.assertNotEquals(config, "not a tuning config"); + } + } diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTest.java b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTest.java index 82ee7afd0e01..0387c8c7517e 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTest.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTest.java @@ -196,27 +196,22 @@ private RabbitStreamSupervisor getSupervisor( DataSchema dataSchema, RabbitStreamSupervisorTuningConfig tuningConfig) { - RabbitStreamSupervisorIOConfig rabbitStreamSupervisorIOConfig = new RabbitStreamSupervisorIOConfig( - STREAM, // stream - URI, // uri - INPUT_FORMAT, // inputFormat - replicas, // replicas - taskCount, // taskCount - new Period(duration), // taskDuration - null, // consumerProperties - null, // autoscalerConfig - 400L, // poll timeout - new Period("P1D"), // start delat - new Period("PT30M"), // period - new Period("PT30S"), // completiontimeout - false, // useearliest - lateMessageRejectionPeriod, // latemessagerejection - earlyMessageRejectionPeriod, // early message rejection - null, // latemessagerejectionstartdatetime - 1, - null, - null - ); + RabbitStreamSupervisorIOConfig rabbitStreamSupervisorIOConfig = new RabbitStreamIOConfigBuilder() + .withStream(STREAM) + .withUri(URI) + .withInputFormat(INPUT_FORMAT) + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withPollTimeout(400L) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30M")) + .withCompletionTimeout(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .withStopTaskCount(1) + .build(); RabbitStreamIndexTaskClientFactory clientFactory = new RabbitStreamIndexTaskClientFactory(null, OBJECT_MAPPER); RabbitStreamSupervisor supervisor = new RabbitStreamSupervisor( @@ -263,27 +258,20 @@ public RabbitStreamSupervisor getDefaultSupervisor() @Test public void testRecordSupplier() { - RabbitStreamSupervisorIOConfig rabbitStreamSupervisorIOConfig = new RabbitStreamSupervisorIOConfig( - STREAM, // stream - URI, // uri - INPUT_FORMAT, // inputFormat - 1, // replicas - 1, // taskCount - new Period("PT30M"), // taskDuration - null, // consumerProperties - null, // autoscalerConfig - 400L, // poll timeout - new Period("P1D"), // start delat - new Period("PT30M"), // period - new Period("PT30S"), // completiontimeout - false, // useearliest - null, // latemessagerejection - null, // early message rejection - null, // latemessagerejectionstartdatetime - 1, - null, - null - ); + RabbitStreamSupervisorIOConfig rabbitStreamSupervisorIOConfig = new RabbitStreamIOConfigBuilder() + .withStream(STREAM) + .withUri(URI) + .withInputFormat(INPUT_FORMAT) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT30M")) + .withPollTimeout(400L) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30M")) + .withCompletionTimeout(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withStopTaskCount(1) + .build(); RabbitStreamIndexTaskClientFactory clientFactory = new RabbitStreamIndexTaskClientFactory(null, OBJECT_MAPPER); RabbitStreamSupervisor supervisor = new RabbitStreamSupervisor( @@ -407,27 +395,20 @@ public void testCreateTaskIOConfig() null, null, ImmutableSet.of(), - new RabbitStreamSupervisorIOConfig( - STREAM, // stream - URI, // uri - INPUT_FORMAT, // inputFormat - 1, // replicas - 1, // taskCount - new Period("PT30M"), // taskDuration - null, // consumerProperties - null, // autoscalerConfig - 400L, // poll timeout - new Period("P1D"), // start delat - new Period("PT30M"), // period - new Period("PT30S"), // completiontimeout - false, // useearliest - null, // latemessagerejection - null, // early message rejection - null, // latemessagerejectionstartdatetime - 1, - null, - null - ) + new RabbitStreamIOConfigBuilder() + .withStream(STREAM) + .withUri(URI) + .withInputFormat(INPUT_FORMAT) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT30M")) + .withPollTimeout(400L) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30M")) + .withCompletionTimeout(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withStopTaskCount(1) + .build() ); Assert.assertEquals(30L, ioConfig.getRefreshRejectionPeriodsInMinutes().longValue()); @@ -479,27 +460,17 @@ public void testBoundedModeCreateTasksWithCorrectOffsets() "queue-1", 600 ); - final RabbitStreamSupervisorIOConfig rabbitSupervisorIOConfig = new RabbitStreamSupervisorIOConfig( - STREAM, - URI, - INPUT_FORMAT, - 1, - 1, - new Period("PT30S"), - null, - null, - null, - new Period("PT30M"), - null, - null, - null, - null, - null, - null, - 1000, - null, - new BoundedStreamConfig(startOffsets, endOffsets) - ); + final RabbitStreamSupervisorIOConfig rabbitSupervisorIOConfig = new RabbitStreamIOConfigBuilder() + .withStream(STREAM) + .withUri(URI) + .withInputFormat(INPUT_FORMAT) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT30S")) + .withStartDelay(new Period("PT30M")) + .withStopTaskCount(1000) + .withBoundedStreamConfig(new BoundedStreamConfig(startOffsets, endOffsets)) + .build(); Assert.assertTrue(rabbitSupervisorIOConfig.isBounded()); diff --git a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaIOConfigBuilder.java b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaIOConfigBuilder.java similarity index 68% rename from extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaIOConfigBuilder.java rename to extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaIOConfigBuilder.java index ac8a9e1d2f7b..ce14ef644940 100644 --- a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaIOConfigBuilder.java +++ b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaIOConfigBuilder.java @@ -21,7 +21,7 @@ import org.apache.druid.data.input.InputFormat; import org.apache.druid.data.input.kafkainput.KafkaInputFormat; -import org.apache.druid.indexing.seekablestream.supervisor.BoundedStreamConfig; +import org.apache.druid.indexing.seekablestream.extension.KafkaConfigOverrides; import org.apache.druid.indexing.seekablestream.supervisor.SupervisorIOConfigBuilder; import java.util.Map; @@ -34,7 +34,9 @@ public class KafkaIOConfigBuilder extends SupervisorIOConfigBuilder consumerProperties; - private BoundedStreamConfig boundedStreamConfig; + private Long pollTimeout; + private KafkaConfigOverrides configOverrides; + private Boolean emitTimeLagMetrics; public KafkaIOConfigBuilder withTopic(String topic) { @@ -54,6 +56,24 @@ public KafkaIOConfigBuilder withConsumerProperties(Map consumerP return this; } + public KafkaIOConfigBuilder withPollTimeout(Long pollTimeout) + { + this.pollTimeout = pollTimeout; + return this; + } + + public KafkaIOConfigBuilder withConfigOverrides(KafkaConfigOverrides configOverrides) + { + this.configOverrides = configOverrides; + return this; + } + + public KafkaIOConfigBuilder withEmitTimeLagMetrics(Boolean emitTimeLagMetrics) + { + this.emitTimeLagMetrics = emitTimeLagMetrics; + return this; + } + public KafkaIOConfigBuilder withKafkaInputFormat(InputFormat valueFormat) { this.inputFormat = new KafkaInputFormat( @@ -70,9 +90,18 @@ public KafkaIOConfigBuilder withKafkaInputFormat(InputFormat valueFormat) return this; } - public KafkaIOConfigBuilder withBoundedStreamConfig(BoundedStreamConfig boundedStreamConfig) + /** + * Populates this builder (base and Kafka-specific fields) from an existing config. + */ + public KafkaIOConfigBuilder copyFrom(KafkaSupervisorIOConfig io) { - this.boundedStreamConfig = boundedStreamConfig; + copyFromBase(io); + this.topic = io.getTopic(); + this.topicPattern = io.getTopicPattern(); + this.consumerProperties = io.getConsumerProperties(); + this.pollTimeout = io.getPollTimeout(); + this.configOverrides = io.getConfigOverrides(); + this.emitTimeLagMetrics = io.isEmitTimeLagMetrics(); return this; } @@ -89,7 +118,7 @@ public KafkaSupervisorIOConfig build() consumerProperties, autoScalerConfig, lagAggregator, - null, + pollTimeout, startDelay, period, useEarliestSequenceNumber, @@ -97,11 +126,11 @@ public KafkaSupervisorIOConfig build() lateMessageRejectionPeriod, earlyMessageRejectionPeriod, lateMessageRejectionStartDateTime, - null, + configOverrides, idleConfig, stopTaskCount, - null, - null, + emitTimeLagMetrics, + serverPriorityToReplicas, boundedStreamConfig ); } diff --git a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfig.java b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfig.java index 7e9fffb7e79c..fd844b61ba10 100644 --- a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfig.java +++ b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfig.java @@ -37,6 +37,7 @@ import javax.annotation.Nullable; import java.util.Map; +import java.util.Objects; public class KafkaSupervisorIOConfig extends SeekableStreamSupervisorIOConfig { @@ -212,4 +213,42 @@ private static String checkTopicArguments(String topic, String topicPattern) return topic != null ? topic : topicPattern; } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (!super.equals(o)) { + return false; + } + KafkaSupervisorIOConfig that = (KafkaSupervisorIOConfig) o; + return pollTimeout == that.pollTimeout + && emitTimeLagMetrics == that.emitTimeLagMetrics + && Objects.equals(consumerProperties, that.consumerProperties) + && Objects.equals(configOverrides, that.configOverrides) + && Objects.equals(topic, that.topic) + && Objects.equals(topicPattern, that.topicPattern); + } + + @Override + public int hashCode() + { + return Objects.hash( + super.hashCode(), + consumerProperties, + pollTimeout, + configOverrides, + topic, + topicPattern, + emitTimeLagMetrics + ); + } + + @Override + public KafkaIOConfigBuilder toBuilder() + { + return new KafkaIOConfigBuilder().copyFrom(this); + } + } diff --git a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java index 31d3e8fad691..5a92f58b9215 100644 --- a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java +++ b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java @@ -267,4 +267,42 @@ public String toString() ", suspend=" + isSuspended() + '}'; } + + @Override + public Builder toBuilder() + { + return new Builder().copyFrom(this); + } + + public static class Builder extends SeekableStreamSupervisorSpec.Builder + { + @Override + protected Builder self() + { + return this; + } + + @Override + public KafkaSupervisorSpec build() + { + return new KafkaSupervisorSpec( + id, + null, + dataSchema, + (KafkaSupervisorTuningConfig) tuningConfig, + (KafkaSupervisorIOConfig) ioConfig, + context, + suspended, + taskStorage, + taskMaster, + indexerMetadataStorageCoordinator, + (KafkaIndexTaskClientFactory) indexTaskClientFactory, + mapper, + emitter, + monitorSchedulerConfig, + rowIngestionMetersFactory, + supervisorStateManagerConfig + ); + } + } } diff --git a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTuningConfig.java b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTuningConfig.java index c4a21674d301..5aea1e7ac150 100644 --- a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTuningConfig.java +++ b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTuningConfig.java @@ -29,6 +29,7 @@ import org.joda.time.Period; import javax.annotation.Nullable; +import java.util.Objects; public class KafkaSupervisorTuningConfig extends KafkaIndexTaskTuningConfig implements SeekableStreamSupervisorTuningConfig @@ -212,6 +213,29 @@ public String toString() '}'; } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (!super.equals(o)) { + return false; + } + KafkaSupervisorTuningConfig that = (KafkaSupervisorTuningConfig) o; + return Objects.equals(workerThreads, that.workerThreads) + && Objects.equals(chatRetries, that.chatRetries) + && Objects.equals(httpTimeout, that.httpTimeout) + && Objects.equals(shutdownTimeout, that.shutdownTimeout) + && Objects.equals(offsetFetchPeriod, that.offsetFetchPeriod); + } + + @Override + public int hashCode() + { + return Objects.hash(super.hashCode(), workerThreads, chatRetries, httpTimeout, shutdownTimeout, offsetFetchPeriod); + } + @Override public KafkaIndexTaskTuningConfig convertToTaskTuningConfig() { diff --git a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java index a6a01d8640d1..a56086adaab8 100644 --- a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java +++ b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java @@ -618,4 +618,45 @@ public void testBoundedModeRoundTrip() throws Exception Assert.assertEquals(2, deserialized.getBoundedStreamConfig().getStartSequenceNumbers().size()); Assert.assertEquals(2, deserialized.getBoundedStreamConfig().getEndSequenceNumbers().size()); } + + private static KafkaIOConfigBuilder ioConfigBuilder() + { + return new KafkaIOConfigBuilder() + .withTopic("topic") + .withConsumerProperties(ImmutableMap.of("bootstrap.servers", "localhost:9092")) + .withReplicas(1) + .withTaskCount(2) + .withTaskDuration(new Period("PT1H")); + } + + @Test + public void testEqualsAndHashCode() + { + final KafkaSupervisorIOConfig config = ioConfigBuilder().build(); + Assert.assertEquals(config, ioConfigBuilder().build()); + Assert.assertEquals(config.hashCode(), ioConfigBuilder().build().hashCode()); + Assert.assertNotEquals(config, null); + Assert.assertNotEquals(config, "not an io config"); + Assert.assertNotEquals(config, ioConfigBuilder().withTopic("other").build()); + Assert.assertNotEquals(config, ioConfigBuilder().withReplicas(9).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withTaskCount(9).build()); + Assert.assertNotEquals( + config, + ioConfigBuilder().withConsumerProperties(ImmutableMap.of("bootstrap.servers", "other:9092")).build() + ); + Assert.assertNotEquals(config, ioConfigBuilder().withEmitTimeLagMetrics(true).build()); + } + + @Test + public void testTuningConfigEqualsAndHashCode() + { + final KafkaSupervisorTuningConfig config = new KafkaTuningConfigBuilder().build(); + Assert.assertEquals(config, new KafkaTuningConfigBuilder().build()); + Assert.assertEquals(config.hashCode(), new KafkaTuningConfigBuilder().build().hashCode()); + Assert.assertNotEquals(config, null); + Assert.assertNotEquals(config, "not a tuning config"); + Assert.assertNotEquals(config, new KafkaTuningConfigBuilder().withWorkerThreads(99).build()); + Assert.assertNotEquals(config, new KafkaTuningConfigBuilder().withShutdownTimeout(new Period("PT99M")).build()); + Assert.assertNotEquals(config, new KafkaTuningConfigBuilder().withOffsetFetchPeriod(new Period("PT99S")).build()); + } } diff --git a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java index d1f082af24e9..294fe8d57ac3 100644 --- a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java +++ b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java @@ -5445,34 +5445,19 @@ public void testComputeUnassignedServerPriorities_whenMultipleReplicasPerPriorit final Map consumerProperties = KafkaConsumerConfigs.getConsumerProperties(); consumerProperties.put("bootstrap.servers", kafkaHost); - final KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaSupervisorIOConfig( - topic, - null, - INPUT_FORMAT, - null, - 1, - new Period("PT1H"), - consumerProperties, - null, - null, - KafkaSupervisorIOConfig.DEFAULT_POLL_TIMEOUT_MILLIS, - new Period("P1D"), - new Period("PT30S"), - true, - new Period("PT30M"), - null, - null, - null, - null, - null, - null, - true, - Map.of( - 10, 2, - 20, 3 - ), - null - ); + final KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaIOConfigBuilder() + .withTopic(topic) + .withInputFormat(INPUT_FORMAT) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withConsumerProperties(consumerProperties) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(true) + .withCompletionTimeout(new Period("PT30M")) + .withEmitTimeLagMetrics(true) + .withServerPriorityToReplicas(Map.of(10, 2, 20, 3)) + .build(); Assert.assertEquals(5, (int) kafkaSupervisorIOConfig.getReplicas()); @@ -5563,31 +5548,20 @@ public void testBoundedModeCreateTasksWithCorrectOffsets() throws JsonProcessing final Map consumerProperties = KafkaConsumerConfigs.getConsumerProperties(); consumerProperties.put("bootstrap.servers", kafkaHost); - final KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaSupervisorIOConfig( - topic, - null, - INPUT_FORMAT, - 1, - 1, - new Period("PT1H"), - consumerProperties, - null, - null, - KafkaSupervisorIOConfig.DEFAULT_POLL_TIMEOUT_MILLIS, - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - null, - null, - null, - true, - null, - new BoundedStreamConfig(startOffsets, endOffsets) - ); + final KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaIOConfigBuilder() + .withTopic(topic) + .withInputFormat(INPUT_FORMAT) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withConsumerProperties(consumerProperties) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withEmitTimeLagMetrics(true) + .withBoundedStreamConfig(new BoundedStreamConfig(startOffsets, endOffsets)) + .build(); Assert.assertTrue(kafkaSupervisorIOConfig.isBounded()); @@ -5840,31 +5814,24 @@ private TestableKafkaSupervisor getTestableSupervisor( final Map consumerProperties = KafkaConsumerConfigs.getConsumerProperties(); consumerProperties.put("myCustomKey", "myCustomValue"); consumerProperties.put("bootstrap.servers", kafkaHost); - KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaSupervisorIOConfig( - multiTopic ? null : topic, - multiTopic ? topicPattern : null, - INPUT_FORMAT, - replicas, - taskCount, - new Period(duration), - consumerProperties, - null, - null, - KafkaSupervisorIOConfig.DEFAULT_POLL_TIMEOUT_MILLIS, - new Period("P1D"), - new Period("PT30S"), - useEarliestOffset, - new Period("PT30M"), - lateMessageRejectionPeriod, - earlyMessageRejectionPeriod, - null, - null, - idleConfig, - null, - true, - serverPriorityToReplicas, - null - ); + KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaIOConfigBuilder() + .withTopic(multiTopic ? null : topic) + .withTopicPattern(multiTopic ? topicPattern : null) + .withInputFormat(INPUT_FORMAT) + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withConsumerProperties(consumerProperties) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(useEarliestOffset) + .withCompletionTimeout(new Period("PT30M")) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .withIdleConfig(idleConfig) + .withEmitTimeLagMetrics(true) + .withServerPriorityToReplicas(serverPriorityToReplicas) + .build(); KafkaIndexTaskClientFactory taskClientFactory = new KafkaIndexTaskClientFactory( null, @@ -5936,31 +5903,20 @@ private TestableKafkaSupervisor getTestableSupervisorCustomIsTaskCurrent( consumerProperties.put("myCustomKey", "myCustomValue"); consumerProperties.put("bootstrap.servers", kafkaHost); consumerProperties.put("isolation.level", "read_committed"); - KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaSupervisorIOConfig( - topic, - null, - INPUT_FORMAT, - replicas, - taskCount, - new Period(duration), - consumerProperties, - null, - null, - KafkaSupervisorIOConfig.DEFAULT_POLL_TIMEOUT_MILLIS, - new Period("P1D"), - new Period("PT30S"), - useEarliestOffset, - new Period("PT30M"), - lateMessageRejectionPeriod, - earlyMessageRejectionPeriod, - null, - null, - null, - null, - false, - null, - null - ); + KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaIOConfigBuilder() + .withTopic(topic) + .withInputFormat(INPUT_FORMAT) + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withConsumerProperties(consumerProperties) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(useEarliestOffset) + .withCompletionTimeout(new Period("PT30M")) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .build(); KafkaIndexTaskClientFactory taskClientFactory = new KafkaIndexTaskClientFactory( null, @@ -6032,31 +5988,20 @@ private KafkaSupervisor createSupervisor( consumerProperties.put("myCustomKey", "myCustomValue"); consumerProperties.put("bootstrap.servers", kafkaHost); consumerProperties.put("isolation.level", "read_committed"); - KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaSupervisorIOConfig( - topic, - null, - INPUT_FORMAT, - replicas, - taskCount, - new Period(duration), - consumerProperties, - null, - null, - KafkaSupervisorIOConfig.DEFAULT_POLL_TIMEOUT_MILLIS, - new Period("P1D"), - new Period("PT30S"), - useEarliestOffset, - new Period("PT30M"), - lateMessageRejectionPeriod, - earlyMessageRejectionPeriod, - null, - null, - null, - null, - false, - null, - null - ); + KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaIOConfigBuilder() + .withTopic(topic) + .withInputFormat(INPUT_FORMAT) + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withConsumerProperties(consumerProperties) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(useEarliestOffset) + .withCompletionTimeout(new Period("PT30M")) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .build(); KafkaIndexTaskClientFactory taskClientFactory = new KafkaIndexTaskClientFactory( null, @@ -6609,31 +6554,20 @@ private TestableKafkaSupervisor getTestableSupervisorWithBoundedConfig( final Map consumerProperties = KafkaConsumerConfigs.getConsumerProperties(); consumerProperties.put("myCustomKey", "myCustomValue"); consumerProperties.put("bootstrap.servers", kafkaHost); - KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaSupervisorIOConfig( - topic, - null, - INPUT_FORMAT, - replicas, - taskCount, - new Period(duration), - consumerProperties, - null, - null, - KafkaSupervisorIOConfig.DEFAULT_POLL_TIMEOUT_MILLIS, - new Period("P1D"), - new Period("PT30S"), - true, - new Period("PT30M"), - null, - null, - null, - null, - null, - null, - true, - null, - boundedConfig - ); + KafkaSupervisorIOConfig kafkaSupervisorIOConfig = new KafkaIOConfigBuilder() + .withTopic(topic) + .withInputFormat(INPUT_FORMAT) + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withConsumerProperties(consumerProperties) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(true) + .withCompletionTimeout(new Period("PT30M")) + .withEmitTimeLagMetrics(true) + .withBoundedStreamConfig(boundedConfig) + .build(); KafkaIndexTaskClientFactory taskClientFactory = new KafkaIndexTaskClientFactory( null, diff --git a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisIOConfigBuilder.java b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisIOConfigBuilder.java new file mode 100644 index 000000000000..8296f012dc0c --- /dev/null +++ b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisIOConfigBuilder.java @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.indexing.kinesis.supervisor; + +import org.apache.druid.indexing.kinesis.KinesisRegion; +import org.apache.druid.indexing.seekablestream.supervisor.SupervisorIOConfigBuilder; + +/** + * Builder for {@link KinesisSupervisorIOConfig}. + */ +public class KinesisIOConfigBuilder extends SupervisorIOConfigBuilder +{ + private String endpoint; + private KinesisRegion region; + private Integer recordsPerFetch; + private Integer fetchDelayMillis; + private String awsAssumedRoleArn; + private String awsExternalId; + private Boolean deaggregate; + + public KinesisIOConfigBuilder withEndpoint(String endpoint) + { + this.endpoint = endpoint; + return this; + } + + public KinesisIOConfigBuilder withRegion(KinesisRegion region) + { + this.region = region; + return this; + } + + public KinesisIOConfigBuilder withRecordsPerFetch(Integer recordsPerFetch) + { + this.recordsPerFetch = recordsPerFetch; + return this; + } + + public KinesisIOConfigBuilder withFetchDelayMillis(Integer fetchDelayMillis) + { + this.fetchDelayMillis = fetchDelayMillis; + return this; + } + + public KinesisIOConfigBuilder withAwsAssumedRoleArn(String awsAssumedRoleArn) + { + this.awsAssumedRoleArn = awsAssumedRoleArn; + return this; + } + + public KinesisIOConfigBuilder withAwsExternalId(String awsExternalId) + { + this.awsExternalId = awsExternalId; + return this; + } + + public KinesisIOConfigBuilder withDeaggregate(Boolean deaggregate) + { + this.deaggregate = deaggregate; + return this; + } + + /** + * Populates this builder (base and Kinesis-specific fields) from an existing config. The + * resolved {@code endpoint} is copied directly, so {@code region} (only used to derive an + * endpoint) is left null. + */ + public KinesisIOConfigBuilder copyFrom(KinesisSupervisorIOConfig io) + { + copyFromBase(io); + this.endpoint = io.getEndpoint(); + this.recordsPerFetch = io.getRecordsPerFetch(); + this.fetchDelayMillis = io.getFetchDelayMillis(); + this.awsAssumedRoleArn = io.getAwsAssumedRoleArn(); + this.awsExternalId = io.getAwsExternalId(); + this.deaggregate = io.isDeaggregate(); + return this; + } + + @Override + public KinesisSupervisorIOConfig build() + { + return new KinesisSupervisorIOConfig( + stream, + inputFormat, + endpoint, + region, + replicas, + taskCount, + taskDuration, + startDelay, + period, + useEarliestSequenceNumber, + completionTimeout, + lateMessageRejectionPeriod, + earlyMessageRejectionPeriod, + lateMessageRejectionStartDateTime, + recordsPerFetch, + fetchDelayMillis, + awsAssumedRoleArn, + awsExternalId, + autoScalerConfig, + deaggregate != null && deaggregate, + serverPriorityToReplicas, + boundedStreamConfig + ); + } +} diff --git a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfig.java b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfig.java index e20d3a261cfb..93b64efd9021 100644 --- a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfig.java +++ b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfig.java @@ -36,6 +36,7 @@ import javax.annotation.Nullable; import java.util.Map; +import java.util.Objects; public class KinesisSupervisorIOConfig extends SeekableStreamSupervisorIOConfig { @@ -183,4 +184,42 @@ public String toString() ", deaggregate=" + deaggregate + '}'; } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (!super.equals(o)) { + return false; + } + KinesisSupervisorIOConfig that = (KinesisSupervisorIOConfig) o; + return fetchDelayMillis == that.fetchDelayMillis + && deaggregate == that.deaggregate + && Objects.equals(endpoint, that.endpoint) + && Objects.equals(recordsPerFetch, that.recordsPerFetch) + && Objects.equals(awsAssumedRoleArn, that.awsAssumedRoleArn) + && Objects.equals(awsExternalId, that.awsExternalId); + } + + @Override + public int hashCode() + { + return Objects.hash( + super.hashCode(), + endpoint, + recordsPerFetch, + fetchDelayMillis, + awsAssumedRoleArn, + awsExternalId, + deaggregate + ); + } + + @Override + public KinesisIOConfigBuilder toBuilder() + { + return new KinesisIOConfigBuilder().copyFrom(this); + } } diff --git a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java index 4899337797bf..45170c645770 100644 --- a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java +++ b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java @@ -171,6 +171,53 @@ public KinesisSupervisorIngestionSpec getSpec() return (KinesisSupervisorIngestionSpec) super.getSpec(); } + @Override + public Builder toBuilder() + { + return new Builder().awsCredentialsConfig(awsCredentialsConfig).copyFrom(this); + } + + public static class Builder extends SeekableStreamSupervisorSpec.Builder + { + private AWSCredentialsConfig awsCredentialsConfig; + + public Builder awsCredentialsConfig(AWSCredentialsConfig awsCredentialsConfig) + { + this.awsCredentialsConfig = awsCredentialsConfig; + return this; + } + + @Override + protected Builder self() + { + return this; + } + + @Override + public KinesisSupervisorSpec build() + { + return new KinesisSupervisorSpec( + id, + null, + dataSchema, + (KinesisSupervisorTuningConfig) tuningConfig, + (KinesisSupervisorIOConfig) ioConfig, + context, + suspended, + taskStorage, + taskMaster, + indexerMetadataStorageCoordinator, + (KinesisIndexTaskClientFactory) indexTaskClientFactory, + mapper, + emitter, + monitorSchedulerConfig, + rowIngestionMetersFactory, + awsCredentialsConfig, + supervisorStateManagerConfig + ); + } + } + @Override protected KinesisSupervisorSpec toggleSuspend(boolean suspend) { diff --git a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTuningConfig.java b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTuningConfig.java index 1a11f8d658b7..b3643e9bff46 100644 --- a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTuningConfig.java +++ b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTuningConfig.java @@ -29,6 +29,7 @@ import org.joda.time.Period; import javax.annotation.Nullable; +import java.util.Objects; public class KinesisSupervisorTuningConfig extends KinesisIndexTaskTuningConfig implements SeekableStreamSupervisorTuningConfig @@ -214,6 +215,40 @@ public boolean isUseListShards() return useListShards; } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (!super.equals(o)) { + return false; + } + KinesisSupervisorTuningConfig that = (KinesisSupervisorTuningConfig) o; + return useListShards == that.useListShards + && Objects.equals(workerThreads, that.workerThreads) + && Objects.equals(chatRetries, that.chatRetries) + && Objects.equals(httpTimeout, that.httpTimeout) + && Objects.equals(shutdownTimeout, that.shutdownTimeout) + && Objects.equals(repartitionTransitionDuration, that.repartitionTransitionDuration) + && Objects.equals(offsetFetchPeriod, that.offsetFetchPeriod); + } + + @Override + public int hashCode() + { + return Objects.hash( + super.hashCode(), + workerThreads, + chatRetries, + httpTimeout, + shutdownTimeout, + repartitionTransitionDuration, + offsetFetchPeriod, + useListShards + ); + } + @Override public String toString() { diff --git a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java index 6f4410f3a8be..b24f0385f1cd 100644 --- a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java +++ b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java @@ -33,7 +33,7 @@ import org.apache.druid.data.input.impl.TimestampSpec; import org.apache.druid.data.input.kinesis.KinesisRecordEntity; import org.apache.druid.indexer.granularity.UniformGranularitySpec; -import org.apache.druid.indexing.kinesis.supervisor.KinesisSupervisorIOConfig; +import org.apache.druid.indexing.kinesis.supervisor.KinesisIOConfigBuilder; import org.apache.druid.indexing.kinesis.supervisor.KinesisSupervisorSpec; import org.apache.druid.indexing.overlord.sampler.InputSourceSampler; import org.apache.druid.indexing.overlord.sampler.SamplerConfig; @@ -119,30 +119,11 @@ public void testSample() throws InterruptedException null, DATA_SCHEMA, null, - new KinesisSupervisorIOConfig( - STREAM, - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - null, - null, - null, - null, - null, - null, - null, - true, - null, - null, - null, - null, - null, - null, - null, - null, - null, - false, - null, - null - ), + new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withUseEarliestSequenceNumber(true) + .build(), null, null, null, @@ -175,30 +156,11 @@ public void testGetInputSourceResources() null, DATA_SCHEMA, null, - new KinesisSupervisorIOConfig( - STREAM, - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - null, - null, - null, - null, - null, - null, - null, - true, - null, - null, - null, - null, - null, - null, - null, - null, - null, - false, - null, - null - ), + new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withUseEarliestSequenceNumber(true) + .build(), null, null, null, diff --git a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java index a93bdf6871bc..4fb1fce99193 100644 --- a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java +++ b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java @@ -26,6 +26,7 @@ import org.apache.druid.jackson.DefaultObjectMapper; import org.hamcrest.CoreMatchers; import org.joda.time.Duration; +import org.joda.time.Period; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; @@ -204,4 +205,39 @@ public void testUnboundedModeByDefault() throws Exception Assert.assertNull(config.getBoundedStreamConfig()); } + private static KinesisIOConfigBuilder ioConfigBuilder() + { + return new KinesisIOConfigBuilder() + .withStream("stream") + .withEndpoint("awsEndpoint") + .withReplicas(1) + .withTaskCount(2) + .withTaskDuration(new Period("PT1H")); + } + + @Test + public void testEqualsAndHashCode() + { + final KinesisSupervisorIOConfig config = ioConfigBuilder().build(); + Assert.assertEquals(config, ioConfigBuilder().build()); + Assert.assertEquals(config.hashCode(), ioConfigBuilder().build().hashCode()); + Assert.assertNotEquals(config, null); + Assert.assertNotEquals(config, "not an io config"); + Assert.assertNotEquals(config, ioConfigBuilder().withEndpoint("other").build()); + Assert.assertNotEquals(config, ioConfigBuilder().withReplicas(9).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withTaskCount(9).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withFetchDelayMillis(999).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withDeaggregate(true).build()); + } + + @Test + public void testTuningConfigEqualsAndHashCode() + { + final KinesisSupervisorTuningConfig config = KinesisSupervisorTuningConfig.defaultConfig(); + Assert.assertEquals(config, KinesisSupervisorTuningConfig.defaultConfig()); + Assert.assertEquals(config.hashCode(), KinesisSupervisorTuningConfig.defaultConfig().hashCode()); + Assert.assertNotEquals(config, null); + Assert.assertNotEquals(config, "not a tuning config"); + } + } diff --git a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java index 3f6da8aa0532..2c6f8a24502a 100644 --- a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java +++ b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java @@ -448,30 +448,20 @@ public void testNoInitialStateWithAutoScaleIn() throws Exception @Test public void testRecordSupplier() { - KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - 1, - 1, - new Period("PT30M"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - 100, - 1000, - null, - null, - null, - false, - null, - null - ); + KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT30M")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withRecordsPerFetch(100) + .withFetchDelayMillis(1000) + .build(); KinesisIndexTaskClientFactory clientFactory = new KinesisIndexTaskClientFactory(null, OBJECT_MAPPER); KinesisSupervisor supervisor = new KinesisSupervisor( taskStorage, @@ -515,59 +505,40 @@ public void testRecordSupplier() public void testKinesisIOConfigInitAndAutoscalerConfigCreation() { // create KinesisSupervisorIOConfig with autoScalerConfig null - KinesisSupervisorIOConfig kinesisSupervisorIOConfigWithNullAutoScalerConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - 1, - 1, - new Period("PT30M"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - 100, - 1000, - null, - null, - null, - false, - null, - null - ); + KinesisSupervisorIOConfig kinesisSupervisorIOConfigWithNullAutoScalerConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT30M")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withRecordsPerFetch(100) + .withFetchDelayMillis(1000) + .build(); AutoScalerConfig autoscalerConfigNull = kinesisSupervisorIOConfigWithNullAutoScalerConfig.getAutoScalerConfig(); Assert.assertNull(autoscalerConfigNull); // create KinesisSupervisorIOConfig with autoScalerConfig Empty - KinesisSupervisorIOConfig kinesisSupervisorIOConfigWithEmptyAutoScalerConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - 1, - 1, - new Period("PT30M"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - 100, - 1000, - null, - null, - OBJECT_MAPPER.convertValue(new HashMap<>(), AutoScalerConfig.class), - false, - null, - null - ); + KinesisSupervisorIOConfig kinesisSupervisorIOConfigWithEmptyAutoScalerConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT30M")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withRecordsPerFetch(100) + .withFetchDelayMillis(1000) + .withAutoScalerConfig(OBJECT_MAPPER.convertValue(new HashMap<>(), AutoScalerConfig.class)) + .build(); AutoScalerConfig autoscalerConfig = kinesisSupervisorIOConfigWithEmptyAutoScalerConfig.getAutoScalerConfig(); Assert.assertNotNull(autoscalerConfig); @@ -4199,30 +4170,10 @@ public void testCorrectInputSources() null, dataSchema, null, - new KinesisSupervisorIOConfig( - STREAM, - null, - null, - null, - null, - null, - null, - null, - null, - true, - null, - null, - null, - null, - null, - null, - null, - null, - null, - false, - null, - null - ), + new KinesisIOConfigBuilder() + .withStream(STREAM) + .withUseEarliestSequenceNumber(true) + .build(), null, null, null, @@ -4741,30 +4692,18 @@ public void testBoundedModeCreateTasksWithCorrectOffsets() "shardId-000000000000", "49590338271534258098958632799622211348320767794297876514", "shardId-000000000001", "49590338271556258844158102930252531974520363696878520322" ); - final KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - 1, - 1, - new Period("PT30S"), - null, - new Period("PT30M"), - null, - null, - null, - null, - null, - null, - 0, - null, - null, - null, - true, - null, - new BoundedStreamConfig(startOffsets, endOffsets) - ); + final KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT30S")) + .withSupervisorRunPeriod(new Period("PT30M")) + .withFetchDelayMillis(0) + .withDeaggregate(true) + .withBoundedStreamConfig(new BoundedStreamConfig(startOffsets, endOffsets)) + .build(); Assert.assertTrue(kinesisSupervisorIOConfig.isBounded()); @@ -4841,30 +4780,18 @@ public void testBoundedMode_singleRecordRange_notEmpty() Map startOffsets = ImmutableMap.of(SHARD_ID0, singleOffset); Map endOffsets = ImmutableMap.of(SHARD_ID0, singleOffset); - final KinesisSupervisorIOConfig ioConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - 1, - 1, - new Period("PT1H"), - null, - new Period("PT30M"), - null, - null, - null, - null, - null, - null, - 0, - null, - null, - null, - true, - null, - new BoundedStreamConfig(startOffsets, endOffsets) - ); + final KinesisSupervisorIOConfig ioConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withSupervisorRunPeriod(new Period("PT30M")) + .withFetchDelayMillis(0) + .withDeaggregate(true) + .withBoundedStreamConfig(new BoundedStreamConfig(startOffsets, endOffsets)) + .build(); final KinesisIndexTaskClientFactory taskClientFactory = new KinesisIndexTaskClientFactory(null, null); final KinesisSupervisorSpec spec = new KinesisSupervisorSpec( @@ -5357,30 +5284,20 @@ private TestableKinesisSupervisor getTestableSupervisor( boolean suspended ) { - KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - replicas, - taskCount, - new Period(duration), - new Period("P1D"), - new Period("PT30S"), - useEarliestOffset, - new Period("PT30M"), - lateMessageRejectionPeriod, - earlyMessageRejectionPeriod, - null, - null, - null, - null, - null, - null, - false, - null, - null - ); + KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(useEarliestOffset) + .withCompletionTimeout(new Period("PT30M")) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .build(); KinesisIndexTaskClientFactory taskClientFactory = new KinesisIndexTaskClientFactory( null, @@ -5501,30 +5418,22 @@ private TestableKinesisSupervisor getTestableSupervisor( AutoScalerConfig autoScalerConfig ) { - KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - replicas, - taskCount, - new Period(duration), - new Period("P1D"), - new Period("PT30S"), - useEarliestOffset, - new Period("PT30M"), - lateMessageRejectionPeriod, - earlyMessageRejectionPeriod, - null, - null, - fetchDelayMillis, - null, - null, - autoScalerConfig, - false, - null, - null - ); + KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(useEarliestOffset) + .withCompletionTimeout(new Period("PT30M")) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .withFetchDelayMillis(fetchDelayMillis) + .withAutoScalerConfig(autoScalerConfig) + .build(); KinesisIndexTaskClientFactory taskClientFactory = new KinesisIndexTaskClientFactory( null, @@ -5589,30 +5498,21 @@ private TestableKinesisSupervisor getTestableSupervisorCustomIsTaskCurrent( boolean isTaskCurrentReturn ) { - KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - replicas, - taskCount, - new Period(duration), - new Period("P1D"), - new Period("PT30S"), - useEarliestOffset, - new Period("PT30M"), - lateMessageRejectionPeriod, - earlyMessageRejectionPeriod, - null, - null, - fetchDelayMillis, - null, - null, - null, - false, - null, - null - ); + KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(useEarliestOffset) + .withCompletionTimeout(new Period("PT30M")) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .withFetchDelayMillis(fetchDelayMillis) + .build(); KinesisIndexTaskClientFactory taskClientFactory = new KinesisIndexTaskClientFactory( null, @@ -5679,30 +5579,21 @@ private KinesisSupervisor createSupervisor( KinesisSupervisorTuningConfig tuningConfig ) { - KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisSupervisorIOConfig( - STREAM, - INPUT_FORMAT, - "awsEndpoint", - null, - replicas, - taskCount, - new Period(duration), - new Period("P1D"), - new Period("PT30S"), - useEarliestOffset, - new Period("PT30M"), - lateMessageRejectionPeriod, - earlyMessageRejectionPeriod, - null, - null, - fetchDelayMillis, - null, - null, - null, - false, - null, - null - ); + KinesisSupervisorIOConfig kinesisSupervisorIOConfig = new KinesisIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(INPUT_FORMAT) + .withEndpoint("awsEndpoint") + .withReplicas(replicas) + .withTaskCount(taskCount) + .withTaskDuration(new Period(duration)) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(useEarliestOffset) + .withCompletionTimeout(new Period("PT30M")) + .withLateMessageRejectionPeriod(lateMessageRejectionPeriod) + .withEarlyMessageRejectionPeriod(earlyMessageRejectionPeriod) + .withFetchDelayMillis(fetchDelayMillis) + .build(); KinesisIndexTaskClientFactory taskClientFactory = new KinesisIndexTaskClientFactory( null, diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java index fa7d96634ae6..a0c3cbbe304a 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java @@ -186,14 +186,69 @@ public boolean createOrUpdateAndStartSupervisor(SupervisorSpec spec) synchronized (lock) { Preconditions.checkState(started, "SupervisorManager not started"); - final boolean shouldUpdateSpec = shouldUpdateSupervisor(spec); + // Persist whenever the spec actually changed (or is new) — independent of whether a restart is + // required. This stops/recreates the supervisor regardless; persistence must not be gated on the + // restart decision, otherwise a no-restart change (e.g. taskCount under autoscaling) would be + // applied to the running supervisor but lost from the metadata store. + final boolean specChanged = isSpecChangedAndValidate(spec); SupervisorSpec existingSpec = possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); spec.merge(existingSpec); - createAndStartSupervisorInternal(spec, shouldUpdateSpec); - return shouldUpdateSpec; + createAndStartSupervisorInternal(spec, specChanged); + return specChanged; } } + /** + * Persists a changed spec and updates the in-memory spec reference without stopping or + * recreating the running supervisor. Used when a re-submitted spec does not require a restart (see + * {@link #shouldUpdateSupervisor}) but should still be recorded so the metadata store reflects the + * latest submission. No-op (returns false) if the spec is byte-identical to the running spec. + */ + public boolean updateSupervisorSpecWithoutRestart(SupervisorSpec spec) + { + Preconditions.checkState(started, "SupervisorManager not started"); + Preconditions.checkNotNull(spec, "spec"); + Preconditions.checkNotNull(spec.getId(), "spec.getId()"); + Preconditions.checkNotNull(spec.getDataSources(), "spec.getDatasources()"); + + synchronized (lock) { + Preconditions.checkState(started, "SupervisorManager not started"); + final Pair current = supervisors.get(spec.getId()); + if (current == null || current.rhs == null || !isSpecChangedAndValidate(spec)) { + return false; + } + spec.merge(current.rhs); + metadataSupervisorManager.insert(spec.getId(), spec); + supervisors.put(spec.getId(), Pair.of(current.lhs, spec)); + return true; + } + } + + /** + * Returns whether the submitted spec differs from the running spec (true if there is no running + * spec). When the spec differs, {@link SupervisorSpec#validateSpecUpdateTo} is invoked to reject + * disallowed updates. This is a pure "did anything change" check; it does not consider whether the + * change warrants a restart (see {@link #shouldUpdateSupervisor}). + */ + private boolean isSpecChangedAndValidate(SupervisorSpec spec) + { + final Pair current = supervisors.get(spec.getId()); + if (current == null || current.rhs == null) { + return true; + } + try { + if (Arrays.equals(jsonMapper.writeValueAsBytes(spec), jsonMapper.writeValueAsBytes(current.rhs))) { + return false; + } + } + catch (JsonProcessingException ex) { + log.warn("Failed to write spec as bytes for spec_id[%s]", spec.getId()); + return true; + } + current.rhs.validateSpecUpdateTo(spec); + return true; + } + /** * Checks whether the submitted SupervisorSpec differs from the current spec in SupervisorManager's supervisor list. * This is used in SupervisorResource specPost to determine whether the Supervisor needs to be restarted @@ -209,24 +264,18 @@ public boolean shouldUpdateSupervisor(SupervisorSpec spec) Preconditions.checkNotNull(spec.getDataSources(), "spec.getDatasources()"); synchronized (lock) { Preconditions.checkState(started, "SupervisorManager not started"); - try { - byte[] specAsBytes = jsonMapper.writeValueAsBytes(spec); - Pair currentSupervisor = supervisors.get(spec.getId()); - if (currentSupervisor == null || currentSupervisor.rhs == null) { - return true; - } else if (Arrays.equals(specAsBytes, jsonMapper.writeValueAsBytes(currentSupervisor.rhs))) { - return false; - } else { - // The spec bytes are different, so we need to check if the update is allowed - currentSupervisor.rhs.validateSpecUpdateTo(spec); - return true; - } + final Pair currentSupervisor = supervisors.get(spec.getId()); + if (currentSupervisor == null || currentSupervisor.rhs == null) { + return true; } - catch (JsonProcessingException ex) { - log.warn("Failed to write spec as bytes for spec_id[%s]", spec.getId()); + // Identical specs never require a restart. Otherwise (after validating the update is allowed) + // let the spec decide whether the differences actually warrant a restart — e.g. a taskCount-only + // change under autoscaling does not. + if (!isSpecChangedAndValidate(spec)) { + return false; } + return spec.requireRestart(currentSupervisor.rhs); } - return true; } public boolean stopAndRemoveSupervisor(String id) diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java index 8d0e04eb7988..9ba8fee4451a 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -166,6 +166,9 @@ public Response specPost( } if (Boolean.TRUE.equals(skipRestartIfUnmodified) && !manager.shouldUpdateSupervisor(spec)) { + // No restart needed, but still persist the spec if it actually changed (e.g. a taskCount + // change under autoscaling) so the metadata store reflects the latest submission. + manager.updateSupervisorSpecWithoutRestart(spec); return Response.ok(ImmutableMap.of("id", spec.getId(), "restarted", false)).build(); } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/IdleConfig.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/IdleConfig.java index 40734815aa62..011ff3c5ca40 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/IdleConfig.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/IdleConfig.java @@ -24,6 +24,7 @@ import com.google.common.base.Preconditions; import javax.annotation.Nullable; +import java.util.Objects; /** * Defines if and when {@link SeekableStreamSupervisor} can become idle. @@ -59,6 +60,25 @@ public Long getInactiveAfterMillis() return this.inactiveAfterMillis; } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + IdleConfig that = (IdleConfig) o; + return enabled == that.enabled && Objects.equals(inactiveAfterMillis, that.inactiveAfterMillis); + } + + @Override + public int hashCode() + { + return Objects.hash(enabled, inactiveAfterMillis); + } + @Override public String toString() { diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java index d84ad70aec93..f4a1369a2e1d 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java @@ -61,5 +61,19 @@ public LagStats aggregate(Map partition final long avgLag = partitionLags.isEmpty() ? 0 : totalLag / partitionLags.size(); return new LagStats(maxLag, totalLag, avgLag); } + + // Stateless: all instances are equal. Needed because Jackson creates fresh instances on + // deserialization rather than reusing the DEFAULT singleton. + @Override + public boolean equals(Object o) + { + return this == o || (o != null && getClass() == o.getClass()); + } + + @Override + public int hashCode() + { + return DefaultLagAggregator.class.hashCode(); + } } } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java index 3c7460d6d597..9b2dbd6d1e4c 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java @@ -35,6 +35,7 @@ import javax.annotation.Nullable; import java.util.Map; +import java.util.Objects; public abstract class SeekableStreamSupervisorIOConfig @@ -316,4 +317,71 @@ public boolean isBounded() { return boundedStreamConfig != null; } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SeekableStreamSupervisorIOConfig that = (SeekableStreamSupervisorIOConfig) o; + // taskCountExplicit and the derived autoScalerEnabled flag are intentionally excluded; they are + // transient construction hints, not part of the config's logical identity. + return taskCount == that.taskCount + && useEarliestSequenceNumber == that.useEarliestSequenceNumber + && Objects.equals(stream, that.stream) + && Objects.equals(inputFormat, that.inputFormat) + && Objects.equals(replicas, that.replicas) + && Objects.equals(taskDuration, that.taskDuration) + && Objects.equals(startDelay, that.startDelay) + && Objects.equals(period, that.period) + && Objects.equals(completionTimeout, that.completionTimeout) + && Objects.equals(lateMessageRejectionPeriod, that.lateMessageRejectionPeriod) + && Objects.equals(earlyMessageRejectionPeriod, that.earlyMessageRejectionPeriod) + && Objects.equals(lateMessageRejectionStartDateTime, that.lateMessageRejectionStartDateTime) + && Objects.equals(autoScalerConfig, that.autoScalerConfig) + && Objects.equals(idleConfig, that.idleConfig) + && Objects.equals(stopTaskCount, that.stopTaskCount) + && Objects.equals(serverPriorityToReplicas, that.serverPriorityToReplicas) + && Objects.equals(boundedStreamConfig, that.boundedStreamConfig) + && Objects.equals(lagAggregator, that.lagAggregator); + } + + @Override + public int hashCode() + { + return Objects.hash( + stream, + inputFormat, + replicas, + taskCount, + taskDuration, + startDelay, + period, + useEarliestSequenceNumber, + completionTimeout, + lateMessageRejectionPeriod, + earlyMessageRejectionPeriod, + lateMessageRejectionStartDateTime, + autoScalerConfig, + idleConfig, + stopTaskCount, + serverPriorityToReplicas, + boundedStreamConfig, + lagAggregator + ); + } + + /** + * Returns a builder pre-populated with this config's values, so callers can produce a modified + * copy (for example with a different {@code taskCount}) without mutating this instance. Subclasses + * override this to return their own builder type carrying stream-specific fields. + */ + public SupervisorIOConfigBuilder toBuilder() + { + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder().copyFromBase(this); + } } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java index 6fbf917b774c..1b172e73b07d 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java @@ -23,6 +23,8 @@ import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.druid.segment.indexing.DataSchema; +import java.util.Objects; + public abstract class SeekableStreamSupervisorIngestionSpec { private final DataSchema dataSchema; @@ -58,4 +60,27 @@ public SeekableStreamSupervisorTuningConfig getTuningConfig() { return tuningConfig; } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + // Compares via getters so subclasses (which only narrow the component types) are covered without + // their own overrides; the getClass() check keeps different stream types unequal. + SeekableStreamSupervisorIngestionSpec that = (SeekableStreamSupervisorIngestionSpec) o; + return Objects.equals(getDataSchema(), that.getDataSchema()) + && Objects.equals(getIOConfig(), that.getIOConfig()) + && Objects.equals(getTuningConfig(), that.getTuningConfig()); + } + + @Override + public int hashCode() + { + return Objects.hash(getDataSchema(), getIOConfig(), getTuningConfig()); + } } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java index ecbd51757c37..b9de5f60de0d 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java @@ -23,6 +23,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import org.apache.druid.annotations.SuppressFBWarnings; import org.apache.druid.common.config.Configs; import org.apache.druid.error.DruidException; import org.apache.druid.error.InvalidInput; @@ -45,6 +46,7 @@ import javax.annotation.Nullable; import java.util.List; import java.util.Map; +import java.util.Objects; public abstract class SeekableStreamSupervisorSpec implements SupervisorSpec { @@ -295,6 +297,72 @@ public void merge(@Nullable SupervisorSpec existingSpec) } } + /** + * Common restart decision shared by all seekable-stream supervisors. A copy of the proposed spec + * ({@code this}) is taken, differences that do not require a restart ("ignore cases") are + * neutralized on that copy, and the result is compared to the running {@code old} spec for + * equality. Returns {@code true} when a restart is needed. + *

+ * Ignore cases work on a copy so the proposed spec — which may be persisted — is never mutated: + *

    + *
  • {@code ioConfig.taskCount} when autoscaling is enabled on either spec, since it is + * overridden at runtime.
    • + *
+ * Everything else (dataSchema, tuningConfig, the rest of ioConfig, context, suspended) must match, + * as determined by {@link #equals(Object)}. Subclasses may add type-specific rules by calling + * {@code super.requireRestart(old)} first. + */ + @Override + public boolean requireRestart(SupervisorSpec old) + { + if (!(old instanceof SeekableStreamSupervisorSpec other)) { + return true; + } + + // Start from a builder initialized with this (proposed) spec, neutralize the ignore-case fields + // via builder setters, then build and compare. The builder produces a copy, so the proposed spec + // — which may be persisted — is never mutated. + final Builder proposed = toBuilder(); + + // Ignore case: taskCount is overridden at runtime when autoscaling is enabled, so align it to + // the existing spec's value. + if (isAutoScalerEnabled() || other.isAutoScalerEnabled()) { + proposed.taskCount(other.getIoConfig().getTaskCount()); + } + + return !proposed.build().equals(other); + } + + private boolean isAutoScalerEnabled() + { + final AutoScalerConfig autoScalerConfig = getIoConfig().getAutoScalerConfig(); + return autoScalerConfig != null && autoScalerConfig.getEnableTaskAutoScaler(); + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SeekableStreamSupervisorSpec that = (SeekableStreamSupervisorSpec) o; + // Injected services (taskStorage, mapper, emitter, etc.) are excluded; only the user-defined + // spec content determines equality. + return suspended == that.suspended + && Objects.equals(id, that.id) + && Objects.equals(ingestionSchema, that.ingestionSchema) + && Objects.equals(context, that.context); + } + + @Override + public int hashCode() + { + return Objects.hash(id, ingestionSchema, context, suspended); + } + protected abstract SeekableStreamSupervisorSpec toggleSuspend(boolean suspend); public abstract SeekableStreamSupervisorSpec createBackfillSpec( @@ -303,4 +371,112 @@ public abstract SeekableStreamSupervisorSpec createBackfillSpec( @Nullable Integer taskCount ); + /** + * Returns a builder pre-populated with this spec's values (including injected services), so callers + * can produce a modified copy without mutating this instance. Subclasses return their own builder. + */ + public abstract Builder toBuilder(); + + /** + * Self-typed builder for {@link SeekableStreamSupervisorSpec} and its subclasses. Holds the spec's + * components and injected services; subclasses implement {@link #self()} and {@link #build()} to + * reconstruct the concrete spec. Setter methods correspond to fields a restart comparison may + * neutralize (see {@link #requireRestart}). + */ + @SuppressFBWarnings( + value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", + justification = "Fields are populated via copyFrom() and read by build() in concrete subclasses, which " + + "live in other modules and so are invisible to SpotBugs' per-module analysis." + ) + public abstract static class Builder> + { + protected String id; + protected DataSchema dataSchema; + protected SeekableStreamSupervisorIOConfig ioConfig; + protected SeekableStreamSupervisorTuningConfig tuningConfig; + protected Map context; + protected Boolean suspended; + protected TaskStorage taskStorage; + protected TaskMaster taskMaster; + protected IndexerMetadataStorageCoordinator indexerMetadataStorageCoordinator; + protected SeekableStreamIndexTaskClientFactory indexTaskClientFactory; + protected ObjectMapper mapper; + protected ServiceEmitter emitter; + protected DruidMonitorSchedulerConfig monitorSchedulerConfig; + protected RowIngestionMetersFactory rowIngestionMetersFactory; + protected SupervisorStateManagerConfig supervisorStateManagerConfig; + + protected abstract T self(); + + public abstract SeekableStreamSupervisorSpec build(); + + /** + * Copies all fields (components and injected services) from an existing spec into this builder. + */ + public T copyFrom(SeekableStreamSupervisorSpec spec) + { + this.id = spec.id; + this.dataSchema = spec.getSpec().getDataSchema(); + this.ioConfig = spec.getIoConfig(); + this.tuningConfig = spec.getTuningConfig(); + this.context = spec.context; + this.suspended = spec.suspended; + this.taskStorage = spec.taskStorage; + this.taskMaster = spec.taskMaster; + this.indexerMetadataStorageCoordinator = spec.indexerMetadataStorageCoordinator; + this.indexTaskClientFactory = spec.indexTaskClientFactory; + this.mapper = spec.mapper; + this.emitter = spec.emitter; + this.monitorSchedulerConfig = spec.monitorSchedulerConfig; + this.rowIngestionMetersFactory = spec.rowIngestionMetersFactory; + this.supervisorStateManagerConfig = spec.supervisorStateManagerConfig; + return self(); + } + + public T id(String id) + { + this.id = id; + return self(); + } + + public T dataSchema(DataSchema dataSchema) + { + this.dataSchema = dataSchema; + return self(); + } + + public T ioConfig(SeekableStreamSupervisorIOConfig ioConfig) + { + this.ioConfig = ioConfig; + return self(); + } + + public T tuningConfig(SeekableStreamSupervisorTuningConfig tuningConfig) + { + this.tuningConfig = tuningConfig; + return self(); + } + + public T context(Map context) + { + this.context = context; + return self(); + } + + public T suspended(boolean suspended) + { + this.suspended = suspended; + return self(); + } + + /** + * Sets {@code ioConfig.taskCount} on a copy of the current ioConfig (never mutates the original). + */ + public T taskCount(int taskCount) + { + this.ioConfig = this.ioConfig.toBuilder().withTaskCount(taskCount).build(); + return self(); + } + } + } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java similarity index 54% rename from indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java rename to indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java index bcfb33e9d123..c592429b9bd0 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java @@ -23,10 +23,15 @@ import org.apache.druid.data.input.impl.JsonInputFormat; import org.apache.druid.indexing.seekablestream.supervisor.autoscaler.AutoScalerConfig; import org.joda.time.DateTime; +import org.joda.time.Duration; import org.joda.time.Period; +import java.util.Map; + /** - * Builder for a {@link SeekableStreamSupervisorIOConfig}. + * Builder for a {@link SeekableStreamSupervisorIOConfig}. Subclasses add stream-specific fields and + * implement {@link #build()}. {@link #copyFromBase(SeekableStreamSupervisorIOConfig)} populates the + * base-class fields from an existing config, enabling {@code toBuilder()}-style copies. * * @param Type of this builder itself * @param Type of the IOConfig created by this builder @@ -51,6 +56,8 @@ public abstract class SupervisorIOConfigBuilder< protected DateTime lateMessageRejectionStartDateTime; protected IdleConfig idleConfig; protected Integer stopTaskCount; + protected Map serverPriorityToReplicas; + protected BoundedStreamConfig boundedStreamConfig; public Self withStream(String stream) { @@ -154,11 +161,94 @@ public Self withStopTaskCount(Integer stopTaskCount) return self(); } + public Self withServerPriorityToReplicas(Map serverPriorityToReplicas) + { + this.serverPriorityToReplicas = serverPriorityToReplicas; + return self(); + } + + public Self withBoundedStreamConfig(BoundedStreamConfig boundedStreamConfig) + { + this.boundedStreamConfig = boundedStreamConfig; + return self(); + } + + /** + * Populates this builder's base-class fields from an existing config. Stores the constructor's + * input types ({@link Period} rather than the normalized {@link Duration}) so {@link #build()} + * round-trips through the public constructor. + */ + public Self copyFromBase(SeekableStreamSupervisorIOConfig io) + { + this.stream = io.getStream(); + this.inputFormat = io.getInputFormat(); + this.replicas = io.getReplicas(); + this.taskCount = io.getTaskCount(); + this.taskDuration = toPeriod(io.getTaskDuration()); + this.startDelay = toPeriod(io.getStartDelay()); + this.period = toPeriod(io.getPeriod()); + this.useEarliestSequenceNumber = io.isUseEarliestSequenceNumber(); + this.completionTimeout = toPeriod(io.getCompletionTimeout()); + this.lateMessageRejectionPeriod = io.getLateMessageRejectionPeriod().isPresent() + ? toPeriod(io.getLateMessageRejectionPeriod().get()) : null; + this.earlyMessageRejectionPeriod = io.getEarlyMessageRejectionPeriod().isPresent() + ? toPeriod(io.getEarlyMessageRejectionPeriod().get()) : null; + this.lateMessageRejectionStartDateTime = io.getLateMessageRejectionStartDateTime().orNull(); + this.autoScalerConfig = io.getAutoScalerConfig(); + this.lagAggregator = io.getLagAggregator(); + this.idleConfig = io.getIdleConfig(); + this.stopTaskCount = io.getStopTaskCount(); + this.serverPriorityToReplicas = io.getServerPriorityToReplicas(); + this.boundedStreamConfig = io.getBoundedStreamConfig(); + return self(); + } + public abstract C build(); + protected static Period toPeriod(Duration duration) + { + return duration == null ? null : new Period(duration); + } + @SuppressWarnings("unchecked") - private Self self() + protected Self self() { return (Self) this; } + + /** + * Concrete builder producing a plain {@link SeekableStreamSupervisorIOConfig}. All instances are of + * the single anonymous class defined in {@link #build()}, so builder-produced configs share a + * runtime class and compare correctly via {@link SeekableStreamSupervisorIOConfig#equals(Object)}. + */ + public static final class DefaultSupervisorIOConfigBuilder + extends SupervisorIOConfigBuilder + { + @Override + public SeekableStreamSupervisorIOConfig build() + { + return new SeekableStreamSupervisorIOConfig( + stream, + inputFormat, + replicas, + taskCount, + taskDuration, + startDelay, + period, + useEarliestSequenceNumber, + completionTimeout, + lateMessageRejectionPeriod, + earlyMessageRejectionPeriod, + autoScalerConfig, + lagAggregator, + lateMessageRejectionStartDateTime, + idleConfig, + stopTaskCount, + serverPriorityToReplicas, + boundedStreamConfig + ) + { + }; + } + } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java index 199e004b4243..7292e3019c31 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java @@ -254,6 +254,42 @@ public void testShouldUpdateSupervisor() Assert.assertTrue(manager.shouldUpdateSupervisor(new NoopSupervisorSpec("id1", null))); } + @Test + public void testUpdateSupervisorSpecWithoutRestartPersistsChangedSpecAndDoesNotRestart() + { + final Capture capturedInsert = Capture.newInstance(); + final Map existingSpecs = + ImmutableMap.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1)); + EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + // Persist is expected; supervisor1.stop(...) is NOT expected, so verifyAll() asserts no restart. + metadataSupervisorManager.insert(EasyMock.eq("id1"), EasyMock.capture(capturedInsert)); + replayAll(); + + manager.start(); + final SupervisorSpec updated = new VersionedTestSupervisorSpec("id1", supervisor1, 2); + Assert.assertTrue(manager.updateSupervisorSpecWithoutRestart(updated)); + Assert.assertSame(updated, capturedInsert.getValue()); + Assert.assertSame(updated, manager.getSupervisorSpec("id1").get()); + verifyAll(); + } + + @Test + public void testUpdateSupervisorSpecWithoutRestartIsNoopForIdenticalSpec() + { + final Map existingSpecs = ImmutableMap.of("id1", new TestSupervisorSpec("id1", supervisor1)); + EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + // No insert expected for an identical spec. + replayAll(); + + manager.start(); + Assert.assertFalse(manager.updateSupervisorSpecWithoutRestart(new TestSupervisorSpec("id1", supervisor1))); + verifyAll(); + } + @Test public void testShouldUpdateSupervisorIllegalEvolution() { @@ -1392,6 +1428,12 @@ public SeekableStreamSupervisorIOConfig getIoConfig() return getSpec().getIOConfig(); } + @Override + public Builder toBuilder() + { + throw new UnsupportedOperationException(); + } + @JsonTypeName("testBackfillIngestionSpec") static class IngestionSpec extends SeekableStreamSupervisorIngestionSpec { @@ -1427,4 +1469,26 @@ static class IOConfig extends SeekableStreamSupervisorIOConfig } } } + + /** + * A {@link TestSupervisorSpec} with an explicitly-serialized {@code version} field, so two specs + * sharing the same id can still differ in their serialized form (the base spec serializes only + * id-derived properties). + */ + private static class VersionedTestSupervisorSpec extends TestSupervisorSpec + { + private final int version; + + VersionedTestSupervisorSpec(String id, Supervisor supervisor, int version) + { + super(id, supervisor); + this.version = version; + } + + @JsonProperty + public int getVersion() + { + return version; + } + } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java index 31e0d604a222..8d2d4cb919af 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java @@ -239,6 +239,8 @@ public List getDataSources() EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); EasyMock.expect(supervisorManager.shouldUpdateSupervisor(spec)).andReturn(false); + // No restart, but the changed spec is still persisted. + EasyMock.expect(supervisorManager.updateSupervisorSpecWithoutRestart(spec)).andReturn(true); setupMockRequest(); @@ -1773,6 +1775,12 @@ public SeekableStreamSupervisorSpec createBackfillSpec( return null; } + @Override + public Builder toBuilder() + { + throw new UnsupportedOperationException(); + } + @JsonIgnore @Nonnull @Override diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java index ec60d5753f74..1e04af890c17 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java @@ -19,6 +19,7 @@ package org.apache.druid.indexing.seekablestream.supervisor; +import nl.jqno.equalsverifier.EqualsVerifier; import org.apache.druid.data.input.InputFormat; import org.apache.druid.error.DruidException; import org.apache.druid.error.DruidExceptionMatcher; @@ -571,4 +572,45 @@ public void testBoundedModeWithNullConfig() Assert.assertFalse(config.isBounded()); Assert.assertNull(config.getBoundedStreamConfig()); } + + private static SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder ioConfigBuilder() + { + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withReplicas(1) + .withTaskCount(2) + .withTaskDuration(new Period("PT1H")) + .withLagAggregator(LagAggregator.DEFAULT); + } + + @Test + public void testEqualsAndHashCode() + { + final SeekableStreamSupervisorIOConfig config = ioConfigBuilder().build(); + Assert.assertEquals(config, ioConfigBuilder().build()); + Assert.assertEquals(config.hashCode(), ioConfigBuilder().build().hashCode()); + Assert.assertNotEquals(config, null); + Assert.assertNotEquals(config, "not an io config"); + Assert.assertNotEquals(config, ioConfigBuilder().withStream("other").build()); + Assert.assertNotEquals(config, ioConfigBuilder().withReplicas(9).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withTaskCount(9).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withStopTaskCount(7).build()); + Assert.assertNotEquals(config, ioConfigBuilder().withIdleConfig(new IdleConfig(true, 5L)).build()); + } + + @Test + public void testIdleConfigEqualsAndHashCode() + { + EqualsVerifier.forClass(IdleConfig.class).usingGetClass().verify(); + } + + @Test + public void testDefaultLagAggregatorEquals() + { + final LagAggregator aggregator = new LagAggregator.DefaultLagAggregator(); + Assert.assertEquals(aggregator, new LagAggregator.DefaultLagAggregator()); + Assert.assertEquals(aggregator.hashCode(), new LagAggregator.DefaultLagAggregator().hashCode()); + Assert.assertNotEquals(aggregator, null); + Assert.assertNotEquals(aggregator, "not a lag aggregator"); + } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java index 80120d07fdf5..0280f2164915 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java @@ -26,6 +26,7 @@ import org.apache.druid.data.input.impl.JsonInputFormat; import org.apache.druid.error.DruidException; import org.apache.druid.error.DruidExceptionMatcher; +import org.apache.druid.indexing.overlord.supervisor.NoopSupervisorSpec; import org.apache.druid.indexing.overlord.supervisor.Supervisor; import org.apache.druid.indexing.overlord.supervisor.SupervisorManager; import org.apache.druid.indexing.overlord.supervisor.SupervisorStateManager; @@ -672,28 +673,18 @@ public void testSeekableStreamSupervisorSpecWithScaleInThresholdGreaterThanParti EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); // Use an ioConfig whose autoScalerConfig matches the test's intent (min=15, max=20) so the // supervisor's bounds and the scaler's bounds agree. - EasyMock.expect(spec.getIoConfig()).andReturn(new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - null, - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - misconfiguredAutoScalerConfig, - LagAggregator.DEFAULT, - null, - null, - null, - null, - null - ) - { - }).anyTimes(); + EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withAutoScalerConfig(misconfiguredAutoScalerConfig) + .withLagAggregator(LagAggregator.DEFAULT) + .build()).anyTimes(); EasyMock.expect(spec.getTuningConfig()).andReturn(getTuningConfig()).anyTimes(); EasyMock.expect(spec.getEmitter()).andReturn(emitter).anyTimes(); EasyMock.expect(spec.isSuspended()).andReturn(false).anyTimes(); @@ -749,28 +740,18 @@ public void testSeekableStreamSupervisorSpecWithScaleInAlreadyAtMin() throws Int // taskCountMin=2 so scaler's floored output (1) is below min and triggers the clamp. final Map scaleInProps = getScaleInProperties(); scaleInProps.put("taskCountMin", 2); - final SeekableStreamSupervisorIOConfig customIoConfig = new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - null, - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - mapper.convertValue(scaleInProps, AutoScalerConfig.class), - LagAggregator.DEFAULT, - null, - null, - null, - null, - null - ) - { - }; + final SeekableStreamSupervisorIOConfig customIoConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withAutoScalerConfig(mapper.convertValue(scaleInProps, AutoScalerConfig.class)) + .withLagAggregator(LagAggregator.DEFAULT) + .build(); EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); EasyMock.expect(spec.getIoConfig()).andReturn(customIoConfig).anyTimes(); @@ -829,33 +810,23 @@ public int getActiveTaskGroupsCount() @Test public void testSeekableStreamSupervisorSpecWithScaleDisable() throws InterruptedException { - SeekableStreamSupervisorIOConfig seekableStreamSupervisorIOConfig = new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - null, - null, - null, - null - ) - { - }; + SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .build(); EasyMock.expect(spec.getId()).andReturn(SUPERVISOR).anyTimes(); EasyMock.expect(spec.getSupervisorStateManagerConfig()).andReturn(supervisorConfig).anyTimes(); EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); - EasyMock.expect(spec.getIoConfig()).andReturn(seekableStreamSupervisorIOConfig).anyTimes(); + EasyMock.expect(spec.getIoConfig()).andReturn(ioConfig).anyTimes(); EasyMock.expect(spec.getTuningConfig()).andReturn(getTuningConfig()).anyTimes(); EasyMock.expect(spec.getEmitter()).andReturn(emitter).anyTimes(); EasyMock.expect(spec.isSuspended()).andReturn(false).anyTimes(); @@ -888,30 +859,21 @@ public void testSeekableStreamSupervisorSpecWithScaleDisable() throws Interrupte @Test public void testEnablingIdleBeviourPerSupervisorWithOverlordConfigEnabled() { - SeekableStreamSupervisorIOConfig seekableStreamSupervisorIOConfig = new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - new IdleConfig(true, null), - null, - null, - null - ) - { - }; - - EasyMock.expect(ingestionSchema.getIOConfig()).andReturn(seekableStreamSupervisorIOConfig).anyTimes(); + SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .withIdleConfig(new IdleConfig(true, null)) + .build(); + + EasyMock.expect(ingestionSchema.getIOConfig()).andReturn(ioConfig).anyTimes(); EasyMock.expect(ingestionSchema.getDataSchema()).andReturn(dataSchema).anyTimes(); EasyMock.replay(ingestionSchema); spec = new SeekableStreamSupervisorSpec( @@ -952,6 +914,12 @@ public SeekableStreamSupervisorSpec createBackfillSpec( return null; } + @Override + public Builder toBuilder() + { + throw new UnsupportedOperationException(); + } + @Override public String getType() { @@ -1454,6 +1422,73 @@ public void testMerge_withNullExistingSpec_appliesTaskCountStartOnFirstPost() assertMergeResult(null, spec(null, 2, 8, null), 2); // taskCountMin from constructor } + @Test + public void testRequireRestart_identicalSpecsDoNotRestart() + { + final TestSeekableStreamSupervisorSpec seed = + buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); + final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); + final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().build(); + Assert.assertFalse(newSpec.requireRestart(oldSpec)); + } + + @Test + public void testRequireRestart_taskCountChangeWithAutoscalerDoesNotRestart() + { + final TestSeekableStreamSupervisorSpec seed = + buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); + final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().taskCount(2).build(); + final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().taskCount(5).build(); + Assert.assertFalse(newSpec.requireRestart(oldSpec)); + } + + @Test + public void testRequireRestart_taskCountChangeWithoutAutoscalerRestarts() + { + final TestSeekableStreamSupervisorSpec seed = buildSpecWithIoConfig("id", createIOConfig(2, null)); + final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().taskCount(2).build(); + final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().taskCount(5).build(); + Assert.assertTrue(newSpec.requireRestart(oldSpec)); + } + + @Test + public void testRequireRestart_contextChangeRestarts() + { + final TestSeekableStreamSupervisorSpec seed = + buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); + final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); + final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().context(ImmutableMap.of("k", "v")).build(); + Assert.assertTrue(newSpec.requireRestart(oldSpec)); + } + + @Test + public void testRequireRestart_suspendedChangeRestarts() + { + final TestSeekableStreamSupervisorSpec seed = + buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); + final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().suspended(false).build(); + final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().suspended(true).build(); + Assert.assertTrue(newSpec.requireRestart(oldSpec)); + } + + @Test + public void testRequireRestart_dataSchemaChangeRestarts() + { + final TestSeekableStreamSupervisorSpec seed = + buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); + final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); + final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().dataSchema(getDataSchema("other-datasource")).build(); + Assert.assertTrue(newSpec.requireRestart(oldSpec)); + } + + @Test + public void testRequireRestart_differentSpecTypeRestarts() + { + final TestSeekableStreamSupervisorSpec seed = + buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); + Assert.assertTrue(seed.toBuilder().build().requireRestart(new NoopSupervisorSpec("id", ImmutableList.of("ds")))); + } + private void assertMergeResult( @Nullable TestSeekableStreamSupervisorSpec existingSpec, TestSeekableStreamSupervisorSpec newSpec, @@ -1527,53 +1562,24 @@ private SeekableStreamSupervisorIOConfig getIOConfig(boolean scaleOut) private SeekableStreamSupervisorIOConfig getIOConfig(boolean scaleOut, int maxTaskCount) { - if (scaleOut) { - return new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - null, // autoscaler uses taskCountStart/taskCountMin for the initial value - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - mapper.convertValue(getScaleOutProperties(maxTaskCount), AutoScalerConfig.class), - LagAggregator.DEFAULT, - null, - null, - null, - null, - null - ) - { - }; - } else { - return new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - null, // autoscaler uses taskCountStart/taskCountMin for the initial value - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - mapper.convertValue(getScaleInProperties(), AutoScalerConfig.class), - LagAggregator.DEFAULT, - null, - null, - null, - null, - null - ) - { - }; - } + final AutoScalerConfig autoScalerConfig = mapper.convertValue( + scaleOut ? getScaleOutProperties(maxTaskCount) : getScaleInProperties(), + AutoScalerConfig.class + ); + // taskCount is left unset; with autoscaling enabled the ioConfig derives it from + // taskCountStart/taskCountMin. + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withAutoScalerConfig(autoScalerConfig) + .withLagAggregator(LagAggregator.DEFAULT) + .build(); } private static DataSchema getDataSchema() @@ -1630,28 +1636,19 @@ public void testBoundedStreamSupervisorSpec_runsWithBoundedConfig() Map endOffsets = ImmutableMap.of("0", 100L); BoundedStreamConfig boundedConfig = new BoundedStreamConfig(startOffsets, endOffsets); - SeekableStreamSupervisorIOConfig boundedIoConfig = new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - null, - null, - null, - boundedConfig - ) - { - }; + SeekableStreamSupervisorIOConfig boundedIoConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .withBoundedStreamConfig(boundedConfig) + .build(); EasyMock.expect(spec.getId()).andReturn(SUPERVISOR).anyTimes(); EasyMock.expect(spec.getSupervisorStateManagerConfig()).andReturn(supervisorConfig).anyTimes(); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java index 9e45920ad719..32139471fc62 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java @@ -685,28 +685,19 @@ public void testIdleStateTransition() throws Exception EasyMock.reset(spec); EasyMock.expect(spec.isSuspended()).andReturn(false).anyTimes(); EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); - EasyMock.expect(spec.getIoConfig()).andReturn(new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("PT1S"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - new IdleConfig(true, 200L), - null, - null, - null - ) - { - }).anyTimes(); + EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("PT1S")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .withIdleConfig(new IdleConfig(true, 200L)) + .build()).anyTimes(); EasyMock.expect(spec.getId()).andReturn(SUPERVISOR_ID).anyTimes(); EasyMock.expect(spec.getTuningConfig()).andReturn(getTuningConfig()).anyTimes(); EasyMock.expect(spec.getEmitter()).andReturn(emitter).anyTimes(); @@ -794,28 +785,19 @@ public void testIdleOnStartUpAndTurnsToRunningAfterLagUpdates() EasyMock.expect(spec.isSuspended()).andReturn(false).anyTimes(); EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); EasyMock.expect(spec.getContextValue("tags")).andReturn("").anyTimes(); - EasyMock.expect(spec.getIoConfig()).andReturn(new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("PT1S"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - new IdleConfig(true, 200L), - null, - null, - null - ) - { - }).anyTimes(); + EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("PT1S")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .withIdleConfig(new IdleConfig(true, 200L)) + .build()).anyTimes(); EasyMock.expect(spec.getTuningConfig()).andReturn(getTuningConfig()).anyTimes(); EasyMock.expect(spec.getEmitter()).andReturn(emitter).anyTimes(); EasyMock.expect(spec.getMonitorSchedulerConfig()).andReturn(new DruidMonitorSchedulerConfig() @@ -1095,26 +1077,19 @@ public void testStoppingGracefully() public void testCheckpointForActiveTaskGroup() throws InterruptedException, JsonProcessingException { DateTime startTime = DateTimes.nowUtc(); - SeekableStreamSupervisorIOConfig ioConfig = new SeekableStreamSupervisorIOConfig( - STREAM, - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("PT1S"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - new IdleConfig(true, 200L), - null, - null, - null - ) {}; + SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("PT1S")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .withIdleConfig(new IdleConfig(true, 200L)) + .build(); EasyMock.reset(spec); EasyMock.expect(spec.getId()).andReturn(SUPERVISOR_ID).anyTimes(); @@ -1315,28 +1290,20 @@ public void testEarlyStoppingOfTaskGroupBasedOnStopTaskCount() throws Interrupte DateTime startTime = DateTimes.nowUtc().minusHours(2); // Configure supervisor to stop only one task at a time int stopTaskCount = 1; - SeekableStreamSupervisorIOConfig ioConfig = new SeekableStreamSupervisorIOConfig( - STREAM, - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 3, - new Period("PT1H"), - new Period("PT1S"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - new IdleConfig(true, 200L), - stopTaskCount, - null, - null - ) - { - }; + SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(3) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("PT1S")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .withIdleConfig(new IdleConfig(true, 200L)) + .withStopTaskCount(stopTaskCount) + .build(); EasyMock.reset(spec); EasyMock.expect(spec.getId()).andReturn(SUPERVISOR_ID).anyTimes(); @@ -1552,28 +1519,19 @@ public Duration getEmissionDuration() public void testSupervisorStopTaskGroupEarly() throws JsonProcessingException, InterruptedException { DateTime startTime = DateTimes.nowUtc(); - SeekableStreamSupervisorIOConfig ioConfig = new SeekableStreamSupervisorIOConfig( - STREAM, - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("PT1S"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - new IdleConfig(true, 200L), - null, - null, - null - ) - { - }; + SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("PT1S")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .withIdleConfig(new IdleConfig(true, 200L)) + .build(); EasyMock.reset(spec); EasyMock.expect(spec.getId()).andReturn(SUPERVISOR_ID).anyTimes(); @@ -2734,28 +2692,18 @@ private void expectEmitterSupervisor(boolean suspended) EasyMock.expect(spec.getId()).andReturn(SUPERVISOR_ID).anyTimes(); EasyMock.expect(spec.getSupervisorStateManagerConfig()).andReturn(supervisorConfig).anyTimes(); EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); - EasyMock.expect(spec.getIoConfig()).andReturn(new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - 1, - new Period("PT1H"), - new Period("PT1S"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - null, - LagAggregator.DEFAULT, - null, - null, - null, - null, - null - ) - { - }).anyTimes(); + EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("PT1S")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withLagAggregator(LagAggregator.DEFAULT) + .build()).anyTimes(); EasyMock.expect(spec.getTuningConfig()).andReturn(getTuningConfig()).anyTimes(); EasyMock.expect(spec.getEmitter()).andReturn(emitter).anyTimes(); EasyMock.expect(spec.getMonitorSchedulerConfig()).andReturn(new DruidMonitorSchedulerConfig() { @@ -2801,28 +2749,18 @@ public void testMaxAllowedStopsWithStopTaskCountRatio() null, 0.4 ); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( - "stream", - null, - 1, - null, - new Period("PT1H"), - new Period("PT1S"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - autoScalerConfig, - lagAggregator, - null, - null, - 1, // ensure this is overridden - null, - null - ) - { - }; + SeekableStreamSupervisorIOConfig config = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withReplicas(1) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("PT1S")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withAutoScalerConfig(autoScalerConfig) + .withLagAggregator(lagAggregator) + .withStopTaskCount(1) // ensure this is overridden + .build(); Assert.assertEquals(2, config.getMaxAllowedStops()); config.setTaskCount(30); @@ -3120,28 +3058,22 @@ private static SeekableStreamSupervisorIOConfig createSupervisorIOConfig( @Nullable Map serverPriorityToReplicas ) { - return new SeekableStreamSupervisorIOConfig( - "stream", - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - serverPriorityToReplicas == null ? 1 : serverPriorityToReplicas.values().stream().mapToInt(Integer::intValue).sum(), - taskCount, - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - autoScalerConfig, - LagAggregator.DEFAULT, - null, - null, - null, - serverPriorityToReplicas, - null - ) - { - }; + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream("stream") + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(serverPriorityToReplicas == null + ? 1 + : serverPriorityToReplicas.values().stream().mapToInt(Integer::intValue).sum()) + .withTaskCount(taskCount) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withAutoScalerConfig(autoScalerConfig) + .withLagAggregator(LagAggregator.DEFAULT) + .withServerPriorityToReplicas(serverPriorityToReplicas) + .build(); } private static Map getProperties() diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java index c96a64211b97..8282e18f9a35 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java @@ -446,6 +446,54 @@ public SeekableStreamSupervisorSpec createBackfillSpec( { return null; } + + @Override + public Builder toBuilder() + { + return new Builder().supervisor(supervisor).copyFrom(this); + } + + static class Builder extends SeekableStreamSupervisorSpec.Builder + { + private SeekableStreamSupervisor supervisor; + + Builder supervisor(SeekableStreamSupervisor supervisor) + { + this.supervisor = supervisor; + return this; + } + + @Override + protected Builder self() + { + return this; + } + + @Override + public TestSeekableStreamSupervisorSpec build() + { + final SeekableStreamSupervisorIngestionSpec ingestionSchema = + new SeekableStreamSupervisorIngestionSpec(dataSchema, ioConfig, tuningConfig) + { + }; + return new TestSeekableStreamSupervisorSpec( + ingestionSchema, + context, + suspended, + taskStorage, + taskMaster, + indexerMetadataStorageCoordinator, + indexTaskClientFactory, + mapper, + emitter, + monitorSchedulerConfig, + rowIngestionMetersFactory, + supervisorStateManagerConfig, + supervisor, + id + ); + } + } } protected static SeekableStreamSupervisorTuningConfig getTuningConfig() @@ -559,28 +607,19 @@ public static SeekableStreamSupervisorIOConfig createIOConfig( AutoScalerConfig autoScalerConfig ) { - return new SeekableStreamSupervisorIOConfig( - STREAM, - new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false), - 1, - taskCount, - new Period("PT1H"), - new Period("P1D"), - new Period("PT30S"), - false, - new Period("PT30M"), - null, - null, - autoScalerConfig, - LagAggregator.DEFAULT, - null, - null, - null, - null, - null - ) - { - }; + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() + .withStream(STREAM) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withReplicas(1) + .withTaskCount(taskCount) + .withTaskDuration(new Period("PT1H")) + .withStartDelay(new Period("P1D")) + .withSupervisorRunPeriod(new Period("PT30S")) + .withUseEarliestSequenceNumber(false) + .withCompletionTimeout(new Period("PT30M")) + .withAutoScalerConfig(autoScalerConfig) + .withLagAggregator(LagAggregator.DEFAULT) + .build(); } public static AutoScalerConfig lagBasedAutoScalerConfig(int taskCountMin, int taskCountMax, Integer taskCountStart) diff --git a/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java b/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java index 67575057ceb7..61caa7421b76 100644 --- a/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java +++ b/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java @@ -139,4 +139,23 @@ default void merge(@Nullable SupervisorSpec existingSpec) { // No-op by default } + + /** + * Given the currently-running {@code old} spec, returns whether replacing it with this spec + * requires the supervisor to be restarted (re-submitted). This is distinct from asking whether + * the two specs are equal: implementations may return {@code false} for differences that do not + * affect the running supervisor (for example, a {@code taskCount} change while autoscaling is + * enabled, since that field is overridden at runtime). + *

+ * Only consulted once the two specs are known to differ (see + * {@link SupervisorManager#shouldUpdateSupervisor}); the conservative default treats any + * such difference as requiring a restart. + * + * @param old the currently-running supervisor spec + * @return true if the supervisor must be restarted to apply this spec + */ + default boolean requireRestart(SupervisorSpec old) + { + return true; + } } From 835a2ac99edc8bd17ff2b74960b835d9eefd819a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesse=20Tu=C4=9Flu?= Date: Tue, 2 Jun 2026 13:05:10 -0700 Subject: [PATCH 2/4] more stuff --- .../RabbitStreamIndexTaskTuningConfig.java | 25 ++++++++ ...RabbitStreamIndexTaskTuningConfigTest.java | 17 +++++ .../RabbitStreamSupervisorIOConfigTest.java | 37 +++++++++++ .../KafkaSupervisorIOConfigTest.java | 42 ++++++++++++ .../KinesisSupervisorIOConfigTest.java | 37 +++++++++++ .../supervisor/SupervisorManager.java | 54 ++++++++++++---- .../supervisor/SupervisorResource.java | 47 ++++++++------ .../SeekableStreamSupervisorIOConfig.java | 8 ++- .../supervisor/SupervisorManagerTest.java | 64 +++++++++++++++++-- .../supervisor/SupervisorResourceTest.java | 19 ++++-- .../SeekableStreamSupervisorIOConfigTest.java | 21 ++++++ .../SeekableStreamSupervisorSpecTest.java | 24 +++++++ 12 files changed, 349 insertions(+), 46 deletions(-) diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfig.java b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfig.java index dd5bc84d5239..24bdea6816bf 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfig.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfig.java @@ -32,6 +32,7 @@ import javax.annotation.Nullable; import java.io.File; +import java.util.Objects; public class RabbitStreamIndexTaskTuningConfig extends SeekableStreamIndexTaskTuningConfig { @@ -238,6 +239,30 @@ public RabbitStreamIndexTaskTuningConfig withBasePersistDirectory(File dir) ); } + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + if (!super.equals(o)) { + return false; + } + RabbitStreamIndexTaskTuningConfig that = (RabbitStreamIndexTaskTuningConfig) o; + return recordBufferOfferTimeout == that.recordBufferOfferTimeout + && Objects.equals(recordBufferSize, that.recordBufferSize) + && Objects.equals(maxRecordsPerPoll, that.maxRecordsPerPoll); + } + + @Override + public int hashCode() + { + return Objects.hash(super.hashCode(), recordBufferSize, recordBufferOfferTimeout, maxRecordsPerPoll); + } + @Override public String toString() { diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfigTest.java b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfigTest.java index 19930d1f9d54..c054e80357a3 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfigTest.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/RabbitStreamIndexTaskTuningConfigTest.java @@ -21,6 +21,8 @@ import com.fasterxml.jackson.databind.Module; import com.fasterxml.jackson.databind.ObjectMapper; +import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.segment.incremental.OnheapIncrementalIndex; import org.apache.druid.segment.indexing.TuningConfig; @@ -189,4 +191,19 @@ public void testtoString() throws Exception Assert.assertEquals(resStr, config.toString()); } + + /** + * Drift guard for the fields this class adds ({@code recordBufferSize}, {@code recordBufferOfferTimeout}, + * {@code maxRecordsPerPoll}): if one were dropped from {@code equals}, a tuning change would persist + * without restarting the supervisor. + */ + @Test + public void testEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(RabbitStreamIndexTaskTuningConfig.class) + .usingGetClass() + .withRedefinedSuperclass() + .suppress(Warning.NONFINAL_FIELDS) + .verify(); + } } diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java index 57e5e9f7fece..0b48d2397030 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorIOConfigTest.java @@ -22,7 +22,13 @@ import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.Module; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Optional; +import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; +import org.apache.druid.data.input.InputFormat; import org.apache.druid.indexing.rabbitstream.RabbitStreamIndexTaskModule; +import org.apache.druid.indexing.seekablestream.supervisor.LagAggregator; +import org.apache.druid.indexing.seekablestream.supervisor.autoscaler.AutoScalerConfig; import org.apache.druid.jackson.DefaultObjectMapper; import org.hamcrest.CoreMatchers; import org.joda.time.Duration; @@ -32,6 +38,8 @@ import org.junit.Test; import org.junit.rules.ExpectedException; +import static org.easymock.EasyMock.createMock; + public class RabbitStreamSupervisorIOConfigTest { private final ObjectMapper mapper; @@ -247,4 +255,33 @@ public void testTuningConfigEqualsAndHashCode() Assert.assertNotEquals(config, "not a tuning config"); } + /** + * Drift guard for this class's own fields (base fields are covered by SeekableStreamSupervisorIOConfigTest): + * a field omitted from {@code equals} would let a changed spec persist without restarting the supervisor. + */ + @Test + public void testEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(RabbitStreamSupervisorIOConfig.class) + .usingGetClass() + .withRedefinedSuperclass() + .withIgnoredFields("taskCountExplicit", "autoScalerEnabled") + .suppress(Warning.NONFINAL_FIELDS) + .withPrefabValues(Optional.class, Optional.of("a"), Optional.of("b")) + .withPrefabValues(InputFormat.class, createMock(InputFormat.class), createMock(InputFormat.class)) + .withPrefabValues(AutoScalerConfig.class, createMock(AutoScalerConfig.class), createMock(AutoScalerConfig.class)) + .withPrefabValues(LagAggregator.class, createMock(LagAggregator.class), createMock(LagAggregator.class)) + .verify(); + } + + @Test + public void testTuningConfigEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(RabbitStreamSupervisorTuningConfig.class) + .usingGetClass() + .withRedefinedSuperclass() + .suppress(Warning.NONFINAL_FIELDS) + .verify(); + } + } diff --git a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java index a56086adaab8..d372aa7e9e36 100644 --- a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java +++ b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java @@ -22,14 +22,20 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Optional; import com.google.common.collect.ImmutableMap; +import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; +import org.apache.druid.data.input.InputFormat; import org.apache.druid.error.DruidException; import org.apache.druid.indexing.kafka.KafkaConsumerConfigs; import org.apache.druid.indexing.kafka.KafkaIndexTaskModule; import org.apache.druid.indexing.kafka.KafkaRecordSupplier; +import org.apache.druid.indexing.seekablestream.extension.KafkaConfigOverrides; import org.apache.druid.indexing.seekablestream.supervisor.BoundedStreamConfig; import org.apache.druid.indexing.seekablestream.supervisor.IdleConfig; import org.apache.druid.indexing.seekablestream.supervisor.LagAggregator; +import org.apache.druid.indexing.seekablestream.supervisor.autoscaler.AutoScalerConfig; import org.apache.druid.indexing.seekablestream.supervisor.autoscaler.LagBasedAutoScalerConfig; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.DateTimes; @@ -45,6 +51,8 @@ import java.util.Map; import java.util.Properties; +import static org.easymock.EasyMock.createMock; + public class KafkaSupervisorIOConfigTest { private final ObjectMapper mapper; @@ -659,4 +667,38 @@ public void testTuningConfigEqualsAndHashCode() Assert.assertNotEquals(config, new KafkaTuningConfigBuilder().withShutdownTimeout(new Period("PT99M")).build()); Assert.assertNotEquals(config, new KafkaTuningConfigBuilder().withOffsetFetchPeriod(new Period("PT99S")).build()); } + + /** + * Drift guard for this class's own fields (base fields are covered by SeekableStreamSupervisorIOConfigTest): + * a field omitted from {@code equals} would let a changed spec persist without restarting the supervisor. + */ + @Test + public void testEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(KafkaSupervisorIOConfig.class) + .usingGetClass() + .withRedefinedSuperclass() + .withIgnoredFields("taskCountExplicit", "autoScalerEnabled") + .suppress(Warning.NONFINAL_FIELDS) + .withPrefabValues(Optional.class, Optional.of("a"), Optional.of("b")) + .withPrefabValues(InputFormat.class, createMock(InputFormat.class), createMock(InputFormat.class)) + .withPrefabValues(AutoScalerConfig.class, createMock(AutoScalerConfig.class), createMock(AutoScalerConfig.class)) + .withPrefabValues(LagAggregator.class, createMock(LagAggregator.class), createMock(LagAggregator.class)) + .withPrefabValues( + KafkaConfigOverrides.class, + createMock(KafkaConfigOverrides.class), + createMock(KafkaConfigOverrides.class) + ) + .verify(); + } + + @Test + public void testTuningConfigEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(KafkaSupervisorTuningConfig.class) + .usingGetClass() + .withRedefinedSuperclass() + .suppress(Warning.NONFINAL_FIELDS) + .verify(); + } } diff --git a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java index 4fb1fce99193..9ea1f71ed15b 100644 --- a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java +++ b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorIOConfigTest.java @@ -21,8 +21,14 @@ import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Optional; +import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; +import org.apache.druid.data.input.InputFormat; import org.apache.druid.indexing.kinesis.KinesisIndexingServiceModule; import org.apache.druid.indexing.kinesis.KinesisRegion; +import org.apache.druid.indexing.seekablestream.supervisor.LagAggregator; +import org.apache.druid.indexing.seekablestream.supervisor.autoscaler.AutoScalerConfig; import org.apache.druid.jackson.DefaultObjectMapper; import org.hamcrest.CoreMatchers; import org.joda.time.Duration; @@ -32,6 +38,8 @@ import org.junit.Test; import org.junit.rules.ExpectedException; +import static org.easymock.EasyMock.createMock; + public class KinesisSupervisorIOConfigTest { private final ObjectMapper mapper; @@ -240,4 +248,33 @@ public void testTuningConfigEqualsAndHashCode() Assert.assertNotEquals(config, "not a tuning config"); } + /** + * Drift guard for this class's own fields (base fields are covered by SeekableStreamSupervisorIOConfigTest): + * a field omitted from {@code equals} would let a changed spec persist without restarting the supervisor. + */ + @Test + public void testEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(KinesisSupervisorIOConfig.class) + .usingGetClass() + .withRedefinedSuperclass() + .withIgnoredFields("taskCountExplicit", "autoScalerEnabled") + .suppress(Warning.NONFINAL_FIELDS) + .withPrefabValues(Optional.class, Optional.of("a"), Optional.of("b")) + .withPrefabValues(InputFormat.class, createMock(InputFormat.class), createMock(InputFormat.class)) + .withPrefabValues(AutoScalerConfig.class, createMock(AutoScalerConfig.class), createMock(AutoScalerConfig.class)) + .withPrefabValues(LagAggregator.class, createMock(LagAggregator.class), createMock(LagAggregator.class)) + .verify(); + } + + @Test + public void testTuningConfigEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(KinesisSupervisorTuningConfig.class) + .usingGetClass() + .withRedefinedSuperclass() + .suppress(Warning.NONFINAL_FIELDS) + .verify(); + } + } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java index a0c3cbbe304a..b349236a02f8 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java @@ -199,12 +199,27 @@ public boolean createOrUpdateAndStartSupervisor(SupervisorSpec spec) } /** - * Persists a changed spec and updates the in-memory spec reference without stopping or - * recreating the running supervisor. Used when a re-submitted spec does not require a restart (see - * {@link #shouldUpdateSupervisor}) but should still be recorded so the metadata store reflects the - * latest submission. No-op (returns false) if the spec is byte-identical to the running spec. + * Outcome of {@link #createOrUpdateAndStartSupervisor(SupervisorSpec, boolean)}. */ - public boolean updateSupervisorSpecWithoutRestart(SupervisorSpec spec) + public enum SpecUpdateOutcome + { + /** Spec was byte-identical to the running spec and {@code skipRestartIfUnmodified} was set: nothing done. */ + UNCHANGED, + /** Spec changed but did not require a restart: persisted to metadata, running supervisor left in place. */ + PERSISTED_WITHOUT_RESTART, + /** Supervisor was (re)created and started; spec persisted if it changed. */ + RESTARTED + } + + /** + * Decides whether the submitted spec needs a restart and applies it under a single lock, so the decision + * cannot go stale between deciding and acting (which would let a concurrent POST drop a write or persist a + * spec that the running supervisor needs to be recreated for). With {@code skipRestartIfUnmodified} set, an + * unchanged spec is a no-op and a changed spec whose {@link SupervisorSpec#requireRestart} is false (e.g. a + * taskCount change under autoscaling) is persisted without recreating the supervisor; otherwise the + * supervisor is stopped and recreated (the only behavior when the flag is false). + */ + public SpecUpdateOutcome createOrUpdateAndStartSupervisor(SupervisorSpec spec, boolean skipRestartIfUnmodified) { Preconditions.checkState(started, "SupervisorManager not started"); Preconditions.checkNotNull(spec, "spec"); @@ -214,13 +229,28 @@ public boolean updateSupervisorSpecWithoutRestart(SupervisorSpec spec) synchronized (lock) { Preconditions.checkState(started, "SupervisorManager not started"); final Pair current = supervisors.get(spec.getId()); - if (current == null || current.rhs == null || !isSpecChangedAndValidate(spec)) { - return false; + final boolean isNew = current == null || current.rhs == null; + final boolean specChanged = isSpecChangedAndValidate(spec); + + if (skipRestartIfUnmodified && !isNew) { + if (!specChanged) { + return SpecUpdateOutcome.UNCHANGED; + } + if (!spec.requireRestart(current.rhs)) { + // Changed but no restart needed: persist and swap the in-memory reference, leaving the running + // supervisor in place. + spec.merge(current.rhs); + metadataSupervisorManager.insert(spec.getId(), spec); + supervisors.put(spec.getId(), Pair.of(current.lhs, spec)); + return SpecUpdateOutcome.PERSISTED_WITHOUT_RESTART; + } } - spec.merge(current.rhs); - metadataSupervisorManager.insert(spec.getId(), spec); - supervisors.put(spec.getId(), Pair.of(current.lhs, spec)); - return true; + + // Restart path: stop+recreate, persisting the spec when it changed. + final SupervisorSpec existingSpec = possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); + spec.merge(existingSpec); + createAndStartSupervisorInternal(spec, specChanged); + return SpecUpdateOutcome.RESTARTED; } } @@ -242,6 +272,8 @@ private boolean isSpecChangedAndValidate(SupervisorSpec spec) } } catch (JsonProcessingException ex) { + // Treat an unserializable spec as changed (and skip validateSpecUpdateTo, since we cannot diff); + // matches prior behavior. log.warn("Failed to write spec as bytes for spec_id[%s]", spec.getId()); return true; } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java index 9ba8fee4451a..83355e86adbc 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -165,32 +165,39 @@ public Response specPost( .build(); } - if (Boolean.TRUE.equals(skipRestartIfUnmodified) && !manager.shouldUpdateSupervisor(spec)) { - // No restart needed, but still persist the spec if it actually changed (e.g. a taskCount - // change under autoscaling) so the metadata store reflects the latest submission. - manager.updateSupervisorSpecWithoutRestart(spec); - return Response.ok(ImmutableMap.of("id", spec.getId(), "restarted", false)).build(); - } + // Decide and apply atomically so the restart decision cannot go stale under a concurrent POST. + final SupervisorManager.SpecUpdateOutcome outcome = + manager.createOrUpdateAndStartSupervisor(spec, Boolean.TRUE.equals(skipRestartIfUnmodified)); - manager.createOrUpdateAndStartSupervisor(spec); - - final String auditPayload - = StringUtils.format("Update supervisor[%s] for datasource[%s]", spec.getId(), spec.getDataSources()); - auditManager.doAudit( - AuditEntry.builder() - .key(spec.getId()) - .type("supervisor") - .auditInfo(AuthorizationUtils.buildAuditInfo(req)) - .request(AuthorizationUtils.buildRequestInfo("overlord", req)) - .payload(auditPayload) - .build() - ); + // Audit any path that mutated the persisted spec; a no-op UNCHANGED submission is not audited. + if (outcome != SupervisorManager.SpecUpdateOutcome.UNCHANGED) { + auditSupervisorUpdate(spec, req); + } - return Response.ok(ImmutableMap.of("id", spec.getId(), "restarted", true)).build(); + final boolean restarted = outcome == SupervisorManager.SpecUpdateOutcome.RESTARTED; + return Response.ok(ImmutableMap.of("id", spec.getId(), "restarted", restarted)).build(); } ); } + /** + * Records a supervisor-update audit entry, for every {@link #specPost} path that mutates the persisted spec. + */ + private void auditSupervisorUpdate(final SupervisorSpec spec, final HttpServletRequest req) + { + final String auditPayload + = StringUtils.format("Update supervisor[%s] for datasource[%s]", spec.getId(), spec.getDataSources()); + auditManager.doAudit( + AuditEntry.builder() + .key(spec.getId()) + .type("supervisor") + .auditInfo(AuthorizationUtils.buildAuditInfo(req)) + .request(AuthorizationUtils.buildRequestInfo("overlord", req)) + .payload(auditPayload) + .build() + ); + } + private Set getNeededResourceActionsForTask(final SupervisorSpec spec) { final Set resourceActions = diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java index 9b2dbd6d1e4c..58ef5a1dc4f9 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java @@ -376,9 +376,11 @@ public int hashCode() } /** - * Returns a builder pre-populated with this config's values, so callers can produce a modified - * copy (for example with a different {@code taskCount}) without mutating this instance. Subclasses - * override this to return their own builder type carrying stream-specific fields. + * Returns a builder pre-populated with this config's values, for producing a modified copy without + * mutating this instance. Subclasses must override to return their own builder: the + * default builder's {@code build()} yields a generic instance whose {@code getClass()} differs from any + * subclass, so a non-overriding subclass would never {@link #equals} its original and {@link + * SeekableStreamSupervisorSpec#requireRestart} would conservatively force a restart (safe, but unnecessary). */ public SupervisorIOConfigBuilder toBuilder() { diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java index 7292e3019c31..511135232b6c 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java @@ -255,7 +255,7 @@ public void testShouldUpdateSupervisor() } @Test - public void testUpdateSupervisorSpecWithoutRestartPersistsChangedSpecAndDoesNotRestart() + public void testCreateOrUpdateSkipRestart_changedNoRestart_persistsWithoutRestart() { final Capture capturedInsert = Capture.newInstance(); final Map existingSpecs = @@ -268,25 +268,62 @@ public void testUpdateSupervisorSpecWithoutRestartPersistsChangedSpecAndDoesNotR replayAll(); manager.start(); + // version differs (byte change) but requireRestart() is false. final SupervisorSpec updated = new VersionedTestSupervisorSpec("id1", supervisor1, 2); - Assert.assertTrue(manager.updateSupervisorSpecWithoutRestart(updated)); + Assert.assertEquals( + SupervisorManager.SpecUpdateOutcome.PERSISTED_WITHOUT_RESTART, + manager.createOrUpdateAndStartSupervisor(updated, true) + ); Assert.assertSame(updated, capturedInsert.getValue()); Assert.assertSame(updated, manager.getSupervisorSpec("id1").get()); verifyAll(); } @Test - public void testUpdateSupervisorSpecWithoutRestartIsNoopForIdenticalSpec() + public void testCreateOrUpdateSkipRestart_identicalSpec_isNoop() { final Map existingSpecs = ImmutableMap.of("id1", new TestSupervisorSpec("id1", supervisor1)); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); - // No insert expected for an identical spec. + // No insert and no stop expected for an identical spec. replayAll(); manager.start(); - Assert.assertFalse(manager.updateSupervisorSpecWithoutRestart(new TestSupervisorSpec("id1", supervisor1))); + Assert.assertEquals( + SupervisorManager.SpecUpdateOutcome.UNCHANGED, + manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec("id1", supervisor1), true) + ); + verifyAll(); + } + + @Test + public void testCreateOrUpdateSkipRestart_changedRequiringRestart_restartsAndPersists() + { + final Map existingSpecs = + ImmutableMap.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1)); + EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + replayAll(); + manager.start(); + verifyAll(); + + resetAll(); + // A changed spec that requires a restart must be fully applied (stop + recreate + persist), even on the + // skip-restart path — it must never be silently dropped or persisted without restarting. + supervisor1.stop(true); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + metadataSupervisorManager.insert(EasyMock.eq("id1"), EasyMock.anyObject()); + replayAll(); + + final SupervisorSpec updated = new VersionedTestSupervisorSpec("id1", supervisor1, 2, true); + Assert.assertEquals( + SupervisorManager.SpecUpdateOutcome.RESTARTED, + manager.createOrUpdateAndStartSupervisor(updated, true) + ); + Assert.assertSame(updated, manager.getSupervisorSpec("id1").get()); verifyAll(); } @@ -1478,11 +1515,18 @@ static class IOConfig extends SeekableStreamSupervisorIOConfig private static class VersionedTestSupervisorSpec extends TestSupervisorSpec { private final int version; + private final boolean requireRestart; VersionedTestSupervisorSpec(String id, Supervisor supervisor, int version) + { + this(id, supervisor, version, false); + } + + VersionedTestSupervisorSpec(String id, Supervisor supervisor, int version, boolean requireRestart) { super(id, supervisor); this.version = version; + this.requireRestart = requireRestart; } @JsonProperty @@ -1490,5 +1534,15 @@ public int getVersion() { return version; } + + /** + * Lets a test model either a no-restart change (default) or a restart-requiring change. (The default + * {@link SupervisorSpec#requireRestart} is conservative and returns true.) + */ + @Override + public boolean requireRestart(SupervisorSpec old) + { + return requireRestart; + } } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java index 8d2d4cb919af..8236c5629d20 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java @@ -164,7 +164,8 @@ public List getDataSources() }; EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); - EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec)).andReturn(true); + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, false)) + .andReturn(SupervisorManager.SpecUpdateOutcome.RESTARTED); setupMockRequest(); setupMockRequestForAudit(); @@ -238,13 +239,16 @@ public List getDataSources() }; EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); - EasyMock.expect(supervisorManager.shouldUpdateSupervisor(spec)).andReturn(false); - // No restart, but the changed spec is still persisted. - EasyMock.expect(supervisorManager.updateSupervisorSpecWithoutRestart(spec)).andReturn(true); + // Changed but no restart needed: persisted without restarting — and the persist must be audited. + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, true)) + .andReturn(SupervisorManager.SpecUpdateOutcome.PERSISTED_WITHOUT_RESTART); setupMockRequest(); + setupMockRequestForAudit(); EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true); + auditManager.doAudit(EasyMock.anyObject()); + EasyMock.expectLastCall().once(); replayAll(); Response response = supervisorResource.specPost(spec, true, request); @@ -256,8 +260,8 @@ public List getDataSources() resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); - EasyMock.expect(supervisorManager.shouldUpdateSupervisor(spec)).andReturn(true); - EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec)).andReturn(true); + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, true)) + .andReturn(SupervisorManager.SpecUpdateOutcome.RESTARTED); setupMockRequest(); setupMockRequestForAudit(); @@ -289,7 +293,8 @@ public List getDataSources() }; EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); - EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec)).andReturn(true); + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, false)) + .andReturn(SupervisorManager.SpecUpdateOutcome.RESTARTED); setupMockRequest(); setupMockRequestForAudit(); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java index 1e04af890c17..3eadd7514f3d 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java @@ -20,6 +20,7 @@ package org.apache.druid.indexing.seekablestream.supervisor; import nl.jqno.equalsverifier.EqualsVerifier; +import nl.jqno.equalsverifier.Warning; import org.apache.druid.data.input.InputFormat; import org.apache.druid.error.DruidException; import org.apache.druid.error.DruidExceptionMatcher; @@ -604,6 +605,26 @@ public void testIdleConfigEqualsAndHashCode() EqualsVerifier.forClass(IdleConfig.class).usingGetClass().verify(); } + /** + * Drift guard: the supervisor restart decision is equality-based, so any field omitted from + * {@code equals} would let a changed spec persist without restarting. EqualsVerifier reflects over the + * fields and fails automatically on a newly-added unused field — only abstract field types need prefab + * values. {@code taskCountExplicit}/{@code autoScalerEnabled} are derived hints (ignored); {@code taskCount} + * is mutable (NONFINAL_FIELDS suppressed). + */ + @Test + public void testEqualsContractCoversAllFields() + { + EqualsVerifier.forClass(SeekableStreamSupervisorIOConfig.class) + .usingGetClass() + .withIgnoredFields("taskCountExplicit", "autoScalerEnabled") + .suppress(Warning.NONFINAL_FIELDS) + .withPrefabValues(InputFormat.class, mock(InputFormat.class), mock(InputFormat.class)) + .withPrefabValues(AutoScalerConfig.class, mock(AutoScalerConfig.class), mock(AutoScalerConfig.class)) + .withPrefabValues(LagAggregator.class, mock(LagAggregator.class), mock(LagAggregator.class)) + .verify(); + } + @Test public void testDefaultLagAggregatorEquals() { diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java index 0280f2164915..23208e74c826 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java @@ -1489,6 +1489,30 @@ public void testRequireRestart_differentSpecTypeRestarts() Assert.assertTrue(seed.toBuilder().build().requireRestart(new NoopSupervisorSpec("id", ImmutableList.of("ds")))); } + @Test + public void testRequireRestart_disablingAutoscalerWithTaskCountChangeRestarts() + { + // The old spec has autoscaling enabled, so taskCount is neutralized in the comparison. The new + // spec disables autoscaling and bumps taskCount; the autoScalerConfig difference must still force + // a restart even though the taskCount difference alone would not. + final SeekableStreamSupervisorSpec oldSpec = + buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))).toBuilder().build(); + final SeekableStreamSupervisorSpec newSpec = + buildSpecWithIoConfig("id", createIOConfig(5, null)).toBuilder().build(); + Assert.assertTrue(newSpec.requireRestart(oldSpec)); + } + + @Test + public void testRequireRestart_tuningConfigChangeRestarts() + { + // No autoscaler, so taskCount is not neutralized; only the tuningConfig differs between the specs. + final TestSeekableStreamSupervisorSpec seed = buildSpecWithIoConfig("id", createIOConfig(2, null)); + final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); + final SeekableStreamSupervisorSpec newSpec = + seed.toBuilder().tuningConfig(EasyMock.mock(SeekableStreamSupervisorTuningConfig.class)).build(); + Assert.assertTrue(newSpec.requireRestart(oldSpec)); + } + private void assertMergeResult( @Nullable TestSeekableStreamSupervisorSpec existingSpec, TestSeekableStreamSupervisorSpec newSpec, From ef7bb59ef43831cec1ca6bec3d30ecde7ba93008 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesse=20Tu=C4=9Flu?= Date: Wed, 3 Jun 2026 14:18:45 -0700 Subject: [PATCH 3/4] more stuff --- .../embedded/indexing/IngestionSmokeTest.java | 2 +- .../RabbitStreamSupervisorSpec.java | 18 +- ...abbitStreamSupervisorTuningConfigTest.java | 73 +++++++ .../kafka/supervisor/KafkaSupervisorSpec.java | 18 +- .../supervisor/KinesisSupervisorSpec.java | 18 +- .../CompactionSupervisorManager.java | 5 +- .../supervisor/SupervisorManager.java | 138 ++++--------- .../supervisor/SupervisorResource.java | 19 +- .../SupervisorSpecUpdateResult.java | 50 +++++ .../supervisor/LagAggregator.java | 37 ++-- .../SeekableStreamSupervisorIOConfig.java | 15 +- ...SeekableStreamSupervisorIngestionSpec.java | 2 - .../SeekableStreamSupervisorSpec.java | 77 ++------ .../supervisor/SupervisorIOConfigBuilder.java | 5 + .../SegmentTransactionalInsertActionTest.java | 2 +- .../http/OverlordCompactionResourceTest.java | 7 +- .../supervisor/SupervisorManagerTest.java | 187 ++++++++++++++---- .../supervisor/SupervisorResourceTest.java | 54 ++++- .../SeekableStreamSamplerSpecTest.java | 7 + .../SeekableStreamSupervisorIOConfigTest.java | 90 +++++++-- .../SeekableStreamSupervisorSpecTest.java | 24 +-- .../SeekableStreamSupervisorTestBase.java | 18 +- .../overlord/supervisor/SupervisorSpec.java | 16 +- 23 files changed, 552 insertions(+), 330 deletions(-) create mode 100644 indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpecUpdateResult.java diff --git a/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java index 934f67b0dda4..5266e4aafd19 100644 --- a/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java +++ b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java @@ -424,7 +424,7 @@ private void waitForSegmentsToBeQueryable(int numSegments) protected void validateSupervisorUpdateResponse(Map startSupervisorResult, String supervisorId) { - Assertions.assertEquals(Map.of("id", supervisorId, "restarted", "true"), startSupervisorResult); + Assertions.assertEquals(Map.of("id", supervisorId, "modified", "true", "restarted", "true"), startSupervisorResult); } protected void waitForNextCoordinatorCacheSync() diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java index 6097c186fb27..c92e3f5a5537 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorSpec.java @@ -242,15 +242,15 @@ public RabbitStreamSupervisorSpec build() (RabbitStreamSupervisorIOConfig) ioConfig, context, suspended, - taskStorage, - taskMaster, - indexerMetadataStorageCoordinator, - (RabbitStreamIndexTaskClientFactory) indexTaskClientFactory, - mapper, - emitter, - monitorSchedulerConfig, - rowIngestionMetersFactory, - supervisorStateManagerConfig + null, + null, + null, + null, + null, + null, + null, + null, + null ); } } diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java index 697306b175f3..ef6aadeb3047 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java @@ -21,9 +21,17 @@ import com.fasterxml.jackson.databind.Module; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.apache.druid.data.input.impl.DimensionsSpec; +import org.apache.druid.data.input.impl.TimestampSpec; +import org.apache.druid.indexer.granularity.UniformGranularitySpec; import org.apache.druid.indexing.rabbitstream.RabbitStreamIndexTaskModule; import org.apache.druid.jackson.DefaultObjectMapper; +import org.apache.druid.java.util.common.granularity.Granularities; +import org.apache.druid.query.aggregation.CountAggregatorFactory; import org.apache.druid.segment.incremental.OnheapIncrementalIndex; +import org.apache.druid.segment.indexing.DataSchema; import org.apache.druid.segment.indexing.TuningConfig; import org.joda.time.Duration; import org.joda.time.Period; @@ -45,6 +53,17 @@ public RabbitStreamSupervisorTuningConfigTest() @Rule public final ExpectedException exception = ExpectedException.none(); + @Test + 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)))); + } + @Test public void testSerdeWithDefaults() throws Exception { @@ -122,4 +141,58 @@ public void testSerdeWithNonDefaults() throws Exception Assert.assertEquals(Duration.standardSeconds(120), config.getRepartitionTransitionDuration()); } + private RabbitStreamSupervisorSpec supervisorSpec(final RabbitStreamSupervisorTuningConfig tuningConfig) + { + return new RabbitStreamSupervisorSpec.Builder() + .id("id") + .dataSchema(dataSchema()) + .ioConfig(ioConfig()) + .tuningConfig(tuningConfig) + .build(); + } + + private DataSchema dataSchema() + { + return DataSchema.builder() + .withDataSource("testDS") + .withTimestamp(new TimestampSpec("timestamp", "iso", null)) + .withDimensions(DimensionsSpec.EMPTY) + .withAggregators(new CountAggregatorFactory("rows")) + .withGranularity( + new UniformGranularitySpec( + Granularities.HOUR, + Granularities.NONE, + ImmutableList.of() + ) + ) + .build(); + } + + private RabbitStreamSupervisorIOConfig ioConfig() + { + return new RabbitStreamIOConfigBuilder() + .withStream("stream") + .withUri("rabbit://localhost") + .withTaskCount(1) + .withTaskDuration(new Period("PT1H")) + .build(); + } + + private RabbitStreamSupervisorTuningConfig tuningConfig( + final Integer recordBufferSize, + final Integer recordBufferOfferTimeout, + final Integer maxRecordsPerPoll + ) + { + return mapper.convertValue( + ImmutableMap.of( + "type", "rabbit", + "recordBufferSize", recordBufferSize, + "recordBufferOfferTimeout", recordBufferOfferTimeout, + "maxRecordsPerPoll", maxRecordsPerPoll + ), + RabbitStreamSupervisorTuningConfig.class + ); + } + } diff --git a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java index 5a92f58b9215..eb29ca58d0f3 100644 --- a/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java +++ b/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorSpec.java @@ -293,15 +293,15 @@ public KafkaSupervisorSpec build() (KafkaSupervisorIOConfig) ioConfig, context, suspended, - taskStorage, - taskMaster, - indexerMetadataStorageCoordinator, - (KafkaIndexTaskClientFactory) indexTaskClientFactory, - mapper, - emitter, - monitorSchedulerConfig, - rowIngestionMetersFactory, - supervisorStateManagerConfig + null, + null, + null, + null, + null, + null, + null, + null, + null ); } } diff --git a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java index 45170c645770..215aade5c8c9 100644 --- a/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java +++ b/extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorSpec.java @@ -204,16 +204,16 @@ public KinesisSupervisorSpec build() (KinesisSupervisorIOConfig) ioConfig, context, suspended, - taskStorage, - taskMaster, - indexerMetadataStorageCoordinator, - (KinesisIndexTaskClientFactory) indexTaskClientFactory, - mapper, - emitter, - monitorSchedulerConfig, - rowIngestionMetersFactory, + null, + null, + null, + null, + null, + null, + null, + null, awsCredentialsConfig, - supervisorStateManagerConfig + null ); } } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/CompactionSupervisorManager.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/CompactionSupervisorManager.java index 4482b5e688af..c588bb482657 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/CompactionSupervisorManager.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/CompactionSupervisorManager.java @@ -73,8 +73,8 @@ public boolean updateCompactionSupervisor( ) { return performIfLeader(manager -> { - // Check if the spec needs to be updated - if (manager.shouldUpdateSupervisor(spec) && manager.createOrUpdateAndStartSupervisor(spec)) { + final SupervisorSpecUpdateResult result = manager.createOrUpdateAndStartSupervisor(spec, true); + if (result.isModified()) { final String auditPayload = StringUtils.format("Update supervisor[%s] for datasource[%s]", spec.getId(), spec.getDataSources()); auditManager.doAudit( @@ -87,7 +87,6 @@ public boolean updateCompactionSupervisor( .build() ); } - return true; }); } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java index b349236a02f8..5dbf7617c0ae 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java @@ -172,12 +172,14 @@ public boolean handoffTaskGroupsEarly(String id, List taskGroupIds) } /** - * Creates or updates a supervisor and then starts it. - * If no change has been made to the supervisor spec, it is only restarted. - * - * @return true if the supervisor was updated, false otherwise + * Applies {@code spec} under a single lock. With {@code skipRestartIfUnmodified=true}, an unchanged spec + * is a no-op; a changed spec is persisted without restart when {@link SupervisorSpec#requireRestart} is + * false. With {@code skipRestartIfUnmodified=false}, the supervisor is always recreated (legacy behavior). */ - public boolean createOrUpdateAndStartSupervisor(SupervisorSpec spec) + public SupervisorSpecUpdateResult createOrUpdateAndStartSupervisor( + final SupervisorSpec spec, + final boolean skipRestartIfUnmodified + ) { Preconditions.checkState(started, "SupervisorManager not started"); Preconditions.checkNotNull(spec, "spec"); @@ -186,80 +188,49 @@ public boolean createOrUpdateAndStartSupervisor(SupervisorSpec spec) synchronized (lock) { Preconditions.checkState(started, "SupervisorManager not started"); - // Persist whenever the spec actually changed (or is new) — independent of whether a restart is - // required. This stops/recreates the supervisor regardless; persistence must not be gated on the - // restart decision, otherwise a no-restart change (e.g. taskCount under autoscaling) would be - // applied to the running supervisor but lost from the metadata store. - final boolean specChanged = isSpecChangedAndValidate(spec); - SupervisorSpec existingSpec = possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); - spec.merge(existingSpec); - createAndStartSupervisorInternal(spec, specChanged); - return specChanged; - } - } - /** - * Outcome of {@link #createOrUpdateAndStartSupervisor(SupervisorSpec, boolean)}. - */ - public enum SpecUpdateOutcome - { - /** Spec was byte-identical to the running spec and {@code skipRestartIfUnmodified} was set: nothing done. */ - UNCHANGED, - /** Spec changed but did not require a restart: persisted to metadata, running supervisor left in place. */ - PERSISTED_WITHOUT_RESTART, - /** Supervisor was (re)created and started; spec persisted if it changed. */ - RESTARTED - } - - /** - * Decides whether the submitted spec needs a restart and applies it under a single lock, so the decision - * cannot go stale between deciding and acting (which would let a concurrent POST drop a write or persist a - * spec that the running supervisor needs to be recreated for). With {@code skipRestartIfUnmodified} set, an - * unchanged spec is a no-op and a changed spec whose {@link SupervisorSpec#requireRestart} is false (e.g. a - * taskCount change under autoscaling) is persisted without recreating the supervisor; otherwise the - * supervisor is stopped and recreated (the only behavior when the flag is false). - */ - public SpecUpdateOutcome createOrUpdateAndStartSupervisor(SupervisorSpec spec, boolean skipRestartIfUnmodified) - { - Preconditions.checkState(started, "SupervisorManager not started"); - Preconditions.checkNotNull(spec, "spec"); - Preconditions.checkNotNull(spec.getId(), "spec.getId()"); - Preconditions.checkNotNull(spec.getDataSources(), "spec.getDatasources()"); + if (!skipRestartIfUnmodified) { + // Always stop/recreate, persisting whenever the spec actually changed (or is new). + final boolean specChanged = isSpecChangedAndValidate(spec); + final SupervisorSpec existingSpec = possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); + spec.merge(existingSpec); + createAndStartSupervisorInternal(spec, specChanged); + return SupervisorSpecUpdateResult.of(specChanged, true); + } - synchronized (lock) { - Preconditions.checkState(started, "SupervisorManager not started"); final Pair current = supervisors.get(spec.getId()); final boolean isNew = current == null || current.rhs == null; - final boolean specChanged = isSpecChangedAndValidate(spec); - if (skipRestartIfUnmodified && !isNew) { - if (!specChanged) { - return SpecUpdateOutcome.UNCHANGED; - } - if (!spec.requireRestart(current.rhs)) { - // Changed but no restart needed: persist and swap the in-memory reference, leaving the running - // supervisor in place. - spec.merge(current.rhs); - metadataSupervisorManager.insert(spec.getId(), spec); - supervisors.put(spec.getId(), Pair.of(current.lhs, spec)); - return SpecUpdateOutcome.PERSISTED_WITHOUT_RESTART; - } + if (isNew) { + spec.merge(null); + createAndStartSupervisorInternal(spec, true); + return SupervisorSpecUpdateResult.of(true, true); + } + + if (!isSpecChangedAndValidate(spec)) { + return SupervisorSpecUpdateResult.of(false, false); + } + + // merge() may carry forward omitted fields (e.g. taskCount); compare the effective spec. + spec.merge(current.rhs); + final boolean specChanged = isSpecChangedAndValidate(spec); + if (!specChanged) { + return SupervisorSpecUpdateResult.of(false, false); + } + if (!current.rhs.requireRestart(spec)) { + metadataSupervisorManager.insert(spec.getId(), spec); + supervisors.put(spec.getId(), Pair.of(current.lhs, spec)); + return SupervisorSpecUpdateResult.of(true, false); } // Restart path: stop+recreate, persisting the spec when it changed. - final SupervisorSpec existingSpec = possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); - spec.merge(existingSpec); + possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); createAndStartSupervisorInternal(spec, specChanged); - return SpecUpdateOutcome.RESTARTED; + return SupervisorSpecUpdateResult.of(specChanged, true); } } - /** - * Returns whether the submitted spec differs from the running spec (true if there is no running - * spec). When the spec differs, {@link SupervisorSpec#validateSpecUpdateTo} is invoked to reject - * disallowed updates. This is a pure "did anything change" check; it does not consider whether the - * change warrants a restart (see {@link #shouldUpdateSupervisor}). - */ + /** Byte-level diff against the running spec; validates allowed updates when bytes differ. */ private boolean isSpecChangedAndValidate(SupervisorSpec spec) { final Pair current = supervisors.get(spec.getId()); @@ -272,8 +243,6 @@ private boolean isSpecChangedAndValidate(SupervisorSpec spec) } } catch (JsonProcessingException ex) { - // Treat an unserializable spec as changed (and skip validateSpecUpdateTo, since we cannot diff); - // matches prior behavior. log.warn("Failed to write spec as bytes for spec_id[%s]", spec.getId()); return true; } @@ -281,35 +250,6 @@ private boolean isSpecChangedAndValidate(SupervisorSpec spec) return true; } - /** - * Checks whether the submitted SupervisorSpec differs from the current spec in SupervisorManager's supervisor list. - * This is used in SupervisorResource specPost to determine whether the Supervisor needs to be restarted - * - * @param spec The spec submitted - * @return boolean - true only if the spec has been modified, false otherwise - */ - public boolean shouldUpdateSupervisor(SupervisorSpec spec) - { - Preconditions.checkState(started, "SupervisorManager not started"); - Preconditions.checkNotNull(spec, "spec"); - Preconditions.checkNotNull(spec.getId(), "spec.getId()"); - Preconditions.checkNotNull(spec.getDataSources(), "spec.getDatasources()"); - synchronized (lock) { - Preconditions.checkState(started, "SupervisorManager not started"); - final Pair currentSupervisor = supervisors.get(spec.getId()); - if (currentSupervisor == null || currentSupervisor.rhs == null) { - return true; - } - // Identical specs never require a restart. Otherwise (after validating the update is allowed) - // let the spec decide whether the differences actually warrant a restart — e.g. a taskCount-only - // change under autoscaling does not. - if (!isSpecChangedAndValidate(spec)) { - return false; - } - return spec.requireRestart(currentSupervisor.rhs); - } - } - public boolean stopAndRemoveSupervisor(String id) { Preconditions.checkState(started, "SupervisorManager not started"); @@ -509,7 +449,7 @@ public Map resetToLatestAndBackfill(String id, @Nullable Integer Map normalizedEndOffsets = jsonMapper.readValue(jsonMapper.writeValueAsString(endOffsets), Map.class); BoundedStreamConfig boundedStreamConfig = new BoundedStreamConfig(normalizedStartOffsets, normalizedEndOffsets); SupervisorSpec backfillSpec = streamSpec.createBackfillSpec(backfillSupervisorId, boundedStreamConfig, backfillTaskCount); - createOrUpdateAndStartSupervisor(backfillSpec); + createOrUpdateAndStartSupervisor(backfillSpec, false); } catch (JsonProcessingException e) { throw new ISE(e, "Failed to serialize offsets for backfill supervisor[%s]", backfillSupervisorId); diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java index 83355e86adbc..863ab5719ef2 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -165,24 +165,25 @@ public Response specPost( .build(); } - // Decide and apply atomically so the restart decision cannot go stale under a concurrent POST. - final SupervisorManager.SpecUpdateOutcome outcome = + final SupervisorSpecUpdateResult updateResult = manager.createOrUpdateAndStartSupervisor(spec, Boolean.TRUE.equals(skipRestartIfUnmodified)); - // Audit any path that mutated the persisted spec; a no-op UNCHANGED submission is not audited. - if (outcome != SupervisorManager.SpecUpdateOutcome.UNCHANGED) { + if (updateResult.isModified() || updateResult.isRestarted()) { auditSupervisorUpdate(spec, req); } - final boolean restarted = outcome == SupervisorManager.SpecUpdateOutcome.RESTARTED; - return Response.ok(ImmutableMap.of("id", spec.getId(), "restarted", restarted)).build(); + return Response.ok( + ImmutableMap.of( + "id", spec.getId(), + "modified", updateResult.isModified(), + "restarted", updateResult.isRestarted() + ) + ).build(); } ); } - /** - * Records a supervisor-update audit entry, for every {@link #specPost} path that mutates the persisted spec. - */ + /** Audits supervisor spec submissions that changed or restarted the supervisor. */ private void auditSupervisorUpdate(final SupervisorSpec spec, final HttpServletRequest req) { final String auditPayload diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpecUpdateResult.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpecUpdateResult.java new file mode 100644 index 000000000000..bf14917672c0 --- /dev/null +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpecUpdateResult.java @@ -0,0 +1,50 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.indexing.overlord.supervisor; + +/** + * Outcome of {@link SupervisorManager#createOrUpdateAndStartSupervisor(SupervisorSpec, boolean)}. + */ +public final class SupervisorSpecUpdateResult +{ + private final boolean modified; + private final boolean restarted; + + private SupervisorSpecUpdateResult(final boolean modified, final boolean restarted) + { + this.modified = modified; + this.restarted = restarted; + } + + public static SupervisorSpecUpdateResult of(final boolean modified, final boolean restarted) + { + return new SupervisorSpecUpdateResult(modified, restarted); + } + + public boolean isModified() + { + return modified; + } + + public boolean isRestarted() + { + return restarted; + } +} diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java index f4a1369a2e1d..2f5d09792f5f 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/LagAggregator.java @@ -19,6 +19,7 @@ package org.apache.druid.indexing.seekablestream.supervisor; +import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonSubTypes; import com.fasterxml.jackson.annotation.JsonTypeInfo; import org.apache.druid.indexing.overlord.supervisor.autoscaler.LagStats; @@ -48,26 +49,20 @@ public interface LagAggregator */ class DefaultLagAggregator implements LagAggregator { - @Override - public LagStats aggregate(Map partitionLags) + public DefaultLagAggregator() { - long maxLag = 0, totalLag = 0; - for (long lag : partitionLags.values()) { - if (lag > maxLag) { - maxLag = lag; - } - totalLag += Math.max(lag, 0); - } - final long avgLag = partitionLags.isEmpty() ? 0 : totalLag / partitionLags.size(); - return new LagStats(maxLag, totalLag, avgLag); } - // Stateless: all instances are equal. Needed because Jackson creates fresh instances on - // deserialization rather than reusing the DEFAULT singleton. + @JsonCreator + public static DefaultLagAggregator instance() + { + return (DefaultLagAggregator) DEFAULT; + } + @Override public boolean equals(Object o) { - return this == o || (o != null && getClass() == o.getClass()); + return o instanceof DefaultLagAggregator; } @Override @@ -75,5 +70,19 @@ public int hashCode() { return DefaultLagAggregator.class.hashCode(); } + + @Override + public LagStats aggregate(Map partitionLags) + { + long maxLag = 0, totalLag = 0; + for (long lag : partitionLags.values()) { + if (lag > maxLag) { + maxLag = lag; + } + totalLag += Math.max(lag, 0); + } + final long avgLag = partitionLags.isEmpty() ? 0 : totalLag / partitionLags.size(); + return new LagStats(maxLag, totalLag, avgLag); + } } } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java index 58ef5a1dc4f9..7b3bd1df76a6 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfig.java @@ -328,8 +328,7 @@ public boolean equals(Object o) return false; } SeekableStreamSupervisorIOConfig that = (SeekableStreamSupervisorIOConfig) o; - // taskCountExplicit and the derived autoScalerEnabled flag are intentionally excluded; they are - // transient construction hints, not part of the config's logical identity. + // taskCountExplicit and autoScalerEnabled are construction hints, not part of logical identity. return taskCount == that.taskCount && useEarliestSequenceNumber == that.useEarliestSequenceNumber && Objects.equals(stream, that.stream) @@ -375,15 +374,5 @@ public int hashCode() ); } - /** - * Returns a builder pre-populated with this config's values, for producing a modified copy without - * mutating this instance. Subclasses must override to return their own builder: the - * default builder's {@code build()} yields a generic instance whose {@code getClass()} differs from any - * subclass, so a non-overriding subclass would never {@link #equals} its original and {@link - * SeekableStreamSupervisorSpec#requireRestart} would conservatively force a restart (safe, but unnecessary). - */ - public SupervisorIOConfigBuilder toBuilder() - { - return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder().copyFromBase(this); - } + public abstract SupervisorIOConfigBuilder toBuilder(); } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java index 1b172e73b07d..dad3dbe25575 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIngestionSpec.java @@ -70,8 +70,6 @@ public boolean equals(Object o) if (o == null || getClass() != o.getClass()) { return false; } - // Compares via getters so subclasses (which only narrow the component types) are covered without - // their own overrides; the getClass() check keeps different stream types unequal. SeekableStreamSupervisorIngestionSpec that = (SeekableStreamSupervisorIngestionSpec) o; return Objects.equals(getDataSchema(), that.getDataSchema()) && Objects.equals(getIOConfig(), that.getIOConfig()) diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java index b9de5f60de0d..9fd58a92ed8f 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpec.java @@ -297,40 +297,26 @@ public void merge(@Nullable SupervisorSpec existingSpec) } } - /** - * Common restart decision shared by all seekable-stream supervisors. A copy of the proposed spec - * ({@code this}) is taken, differences that do not require a restart ("ignore cases") are - * neutralized on that copy, and the result is compared to the running {@code old} spec for - * equality. Returns {@code true} when a restart is needed. - *

- * Ignore cases work on a copy so the proposed spec — which may be persisted — is never mutated: - *

    - *
  • {@code ioConfig.taskCount} when autoscaling is enabled on either spec, since it is - * overridden at runtime.
    • - *
- * Everything else (dataSchema, tuningConfig, the rest of ioConfig, context, suspended) must match, - * as determined by {@link #equals(Object)}. Subclasses may add type-specific rules by calling - * {@code super.requireRestart(old)} first. - */ @Override - public boolean requireRestart(SupervisorSpec old) + public boolean requireRestart(SupervisorSpec proposedSpec) { - if (!(old instanceof SeekableStreamSupervisorSpec other)) { + if (!(proposedSpec instanceof SeekableStreamSupervisorSpec proposed)) { return true; } - // Start from a builder initialized with this (proposed) spec, neutralize the ignore-case fields - // via builder setters, then build and compare. The builder produces a copy, so the proposed spec - // — which may be persisted — is never mutated. - final Builder proposed = toBuilder(); + final Builder proposedCopy; + try { + proposedCopy = proposed.toBuilder(); + } + catch (UnsupportedOperationException e) { + return true; + } - // Ignore case: taskCount is overridden at runtime when autoscaling is enabled, so align it to - // the existing spec's value. - if (isAutoScalerEnabled() || other.isAutoScalerEnabled()) { - proposed.taskCount(other.getIoConfig().getTaskCount()); + if (isAutoScalerEnabled() || proposed.isAutoScalerEnabled()) { + proposedCopy.taskCount(getIoConfig().getTaskCount()); } - return !proposed.build().equals(other); + return !proposedCopy.build().equals(this); } private boolean isAutoScalerEnabled() @@ -349,8 +335,6 @@ public boolean equals(Object o) return false; } SeekableStreamSupervisorSpec that = (SeekableStreamSupervisorSpec) o; - // Injected services (taskStorage, mapper, emitter, etc.) are excluded; only the user-defined - // spec content determines equality. return suspended == that.suspended && Objects.equals(id, that.id) && Objects.equals(ingestionSchema, that.ingestionSchema) @@ -372,17 +356,13 @@ public abstract SeekableStreamSupervisorSpec createBackfillSpec( ); /** - * Returns a builder pre-populated with this spec's values (including injected services), so callers - * can produce a modified copy without mutating this instance. Subclasses return their own builder. + * Copy builder for restart comparison. Subclasses override; default requires restart on any change. */ - public abstract Builder toBuilder(); + public Builder toBuilder() + { + throw new UnsupportedOperationException("toBuilder() not implemented"); + } - /** - * Self-typed builder for {@link SeekableStreamSupervisorSpec} and its subclasses. Holds the spec's - * components and injected services; subclasses implement {@link #self()} and {@link #build()} to - * reconstruct the concrete spec. Setter methods correspond to fields a restart comparison may - * neutralize (see {@link #requireRestart}). - */ @SuppressFBWarnings( value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", justification = "Fields are populated via copyFrom() and read by build() in concrete subclasses, which " @@ -396,23 +376,11 @@ public abstract static class Builder> protected SeekableStreamSupervisorTuningConfig tuningConfig; protected Map context; protected Boolean suspended; - protected TaskStorage taskStorage; - protected TaskMaster taskMaster; - protected IndexerMetadataStorageCoordinator indexerMetadataStorageCoordinator; - protected SeekableStreamIndexTaskClientFactory indexTaskClientFactory; - protected ObjectMapper mapper; - protected ServiceEmitter emitter; - protected DruidMonitorSchedulerConfig monitorSchedulerConfig; - protected RowIngestionMetersFactory rowIngestionMetersFactory; - protected SupervisorStateManagerConfig supervisorStateManagerConfig; protected abstract T self(); public abstract SeekableStreamSupervisorSpec build(); - /** - * Copies all fields (components and injected services) from an existing spec into this builder. - */ public T copyFrom(SeekableStreamSupervisorSpec spec) { this.id = spec.id; @@ -421,15 +389,6 @@ public T copyFrom(SeekableStreamSupervisorSpec spec) this.tuningConfig = spec.getTuningConfig(); this.context = spec.context; this.suspended = spec.suspended; - this.taskStorage = spec.taskStorage; - this.taskMaster = spec.taskMaster; - this.indexerMetadataStorageCoordinator = spec.indexerMetadataStorageCoordinator; - this.indexTaskClientFactory = spec.indexTaskClientFactory; - this.mapper = spec.mapper; - this.emitter = spec.emitter; - this.monitorSchedulerConfig = spec.monitorSchedulerConfig; - this.rowIngestionMetersFactory = spec.rowIngestionMetersFactory; - this.supervisorStateManagerConfig = spec.supervisorStateManagerConfig; return self(); } @@ -470,7 +429,7 @@ public T suspended(boolean suspended) } /** - * Sets {@code ioConfig.taskCount} on a copy of the current ioConfig (never mutates the original). + * Sets {@code ioConfig.taskCount} on a copy (does not mutate the builder's current ioConfig reference). */ public T taskCount(int taskCount) { diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java index c592429b9bd0..d0ebe35a1fb1 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SupervisorIOConfigBuilder.java @@ -248,6 +248,11 @@ public SeekableStreamSupervisorIOConfig build() boundedStreamConfig ) { + @Override + public SupervisorIOConfigBuilder toBuilder() + { + return new DefaultSupervisorIOConfigBuilder().copyFromBase(this); + } }; } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/SegmentTransactionalInsertActionTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/SegmentTransactionalInsertActionTest.java index 217aae3f2580..49428928a56d 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/SegmentTransactionalInsertActionTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/actions/SegmentTransactionalInsertActionTest.java @@ -194,7 +194,7 @@ public void test_fail_transactionalUpdateDataSourceMetadata() throws Exception acquireTimeChunkLock(TaskLockType.EXCLUSIVE, task, INTERVAL, 5000); final NoopSupervisorSpec supervisorSpec = new NoopSupervisorSpec(SUPERVISOR_ID, List.of(DATA_SOURCE)); - actionTestKit.getSupervisorManager().createOrUpdateAndStartSupervisor(supervisorSpec); + actionTestKit.getSupervisorManager().createOrUpdateAndStartSupervisor(supervisorSpec, false); SegmentPublishResult result = SegmentTransactionalInsertAction.appendAction( ImmutableSet.of(SEGMENT1), diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResourceTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResourceTest.java index 3518e1dea409..e936d923e4dd 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResourceTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResourceTest.java @@ -31,6 +31,7 @@ import org.apache.druid.indexing.overlord.TaskMaster; import org.apache.druid.indexing.overlord.supervisor.CompactionSupervisorManager; import org.apache.druid.indexing.overlord.supervisor.SupervisorManager; +import org.apache.druid.indexing.overlord.supervisor.SupervisorSpecUpdateResult; import org.apache.druid.indexing.overlord.supervisor.VersionedSupervisorSpec; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.segment.TestDataSource; @@ -315,10 +316,8 @@ public void test_updateDatasourceCompactionConfig() final CompactionSupervisorSpec supervisorSpec = new CompactionSupervisorSpec(wikiConfig, false, validator); - EasyMock.expect(supervisorManager.shouldUpdateSupervisor(supervisorSpec)) - .andReturn(true).once(); - EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(supervisorSpec)) - .andReturn(true).once(); + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(supervisorSpec, true)) + .andReturn(SupervisorSpecUpdateResult.of(true, true)).once(); EasyMock.expect(scheduler.validateCompactionConfig(wikiConfig)) .andReturn(CompactionConfigValidationResult.success()).once(); replayAll(); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java index 511135232b6c..e9bc897c05c9 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java @@ -46,6 +46,7 @@ import org.apache.druid.indexing.seekablestream.supervisor.SeekableStreamSupervisorIOConfig; import org.apache.druid.indexing.seekablestream.supervisor.SeekableStreamSupervisorIngestionSpec; import org.apache.druid.indexing.seekablestream.supervisor.SeekableStreamSupervisorSpec; +import org.apache.druid.indexing.seekablestream.supervisor.SupervisorIOConfigBuilder; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; @@ -127,7 +128,7 @@ public void testCreateUpdateAndRemoveSupervisor() manager.start(); Assert.assertEquals(1, manager.getSupervisorIds().size()); - manager.createOrUpdateAndStartSupervisor(spec); + manager.createOrUpdateAndStartSupervisor(spec, false); Assert.assertEquals(2, manager.getSupervisorIds().size()); Assert.assertEquals(spec, manager.getSupervisorSpec("id1").get()); verifyAll(); @@ -138,7 +139,7 @@ public void testCreateUpdateAndRemoveSupervisor() supervisor1.stop(true); replayAll(); - manager.createOrUpdateAndStartSupervisor(spec2); + manager.createOrUpdateAndStartSupervisor(spec2, false); Assert.assertEquals(2, manager.getSupervisorIds().size()); Assert.assertEquals(spec2, manager.getSupervisorSpec("id1").get()); verifyAll(); @@ -190,7 +191,7 @@ public void validateSpecUpdateTo(SupervisorSpec proposedSpec) throws DruidExcept manager.start(); Assert.assertEquals(0, manager.getSupervisorIds().size()); - manager.createOrUpdateAndStartSupervisor(spec); + manager.createOrUpdateAndStartSupervisor(spec, false); Assert.assertEquals(1, manager.getSupervisorIds().size()); Assert.assertEquals(spec, manager.getSupervisorSpec("id1").get()); verifyAll(); @@ -199,7 +200,7 @@ public void validateSpecUpdateTo(SupervisorSpec proposedSpec) throws DruidExcept exception.expect(DruidException.class); replayAll(); - manager.createOrUpdateAndStartSupervisor(spec2); + manager.createOrUpdateAndStartSupervisor(spec2, false); verifyAll(); } @@ -207,7 +208,7 @@ public void validateSpecUpdateTo(SupervisorSpec proposedSpec) throws DruidExcept public void testCreateOrUpdateAndStartSupervisorNotStarted() { exception.expect(IllegalStateException.class); - manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec("id", null)); + manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec("id", null), false); } @Test @@ -219,7 +220,7 @@ public void testCreateOrUpdateAndStartSupervisorNullSpec() exception.expect(NullPointerException.class); manager.start(); - manager.createOrUpdateAndStartSupervisor(null); + manager.createOrUpdateAndStartSupervisor(null, false); verifyAll(); } @@ -232,26 +233,49 @@ public void testCreateOrUpdateAndStartSupervisorNullSpecId() exception.expect(NullPointerException.class); manager.start(); - manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec(null, null)); + manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec(null, null), false); verifyAll(); } @Test - public void testShouldUpdateSupervisor() + public void testUnchangedSpecDoesNotRestart() { SupervisorSpec spec = new TestSupervisorSpec("id1", supervisor1); + Map existingSpecs = ImmutableMap.of("id1", spec); + EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + replayAll(); + manager.start(); + final SupervisorSpecUpdateResult result = manager.createOrUpdateAndStartSupervisor(spec, true); + Assert.assertFalse(result.isModified()); + Assert.assertFalse(result.isRestarted()); + verifyAll(); + } + + @Test + public void testNewSupervisorRequiresRestart() + { SupervisorSpec spec2 = new TestSupervisorSpec("id2", supervisor2); Map existingSpecs = ImmutableMap.of( - "id1", spec + "id1", new TestSupervisorSpec("id1", supervisor1) ); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); replayAll(); manager.start(); - Assert.assertFalse(manager.shouldUpdateSupervisor(spec)); - Assert.assertTrue(manager.shouldUpdateSupervisor(spec2)); - Assert.assertTrue(manager.shouldUpdateSupervisor(new NoopSupervisorSpec("id1", null))); + verifyAll(); + + resetAll(); + supervisor2.start(); + EasyMock.expect(supervisor2.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + metadataSupervisorManager.insert(EasyMock.eq("id2"), EasyMock.anyObject()); + replayAll(); + final SupervisorSpecUpdateResult result = manager.createOrUpdateAndStartSupervisor(spec2, true); + Assert.assertTrue(result.isModified()); + Assert.assertTrue(result.isRestarted()); + verifyAll(); } @Test @@ -270,10 +294,9 @@ public void testCreateOrUpdateSkipRestart_changedNoRestart_persistsWithoutRestar manager.start(); // version differs (byte change) but requireRestart() is false. final SupervisorSpec updated = new VersionedTestSupervisorSpec("id1", supervisor1, 2); - Assert.assertEquals( - SupervisorManager.SpecUpdateOutcome.PERSISTED_WITHOUT_RESTART, - manager.createOrUpdateAndStartSupervisor(updated, true) - ); + final SupervisorSpecUpdateResult updateResult = manager.createOrUpdateAndStartSupervisor(updated, true); + Assert.assertTrue(updateResult.isModified()); + Assert.assertFalse(updateResult.isRestarted()); Assert.assertSame(updated, capturedInsert.getValue()); Assert.assertSame(updated, manager.getSupervisorSpec("id1").get()); verifyAll(); @@ -290,18 +313,19 @@ public void testCreateOrUpdateSkipRestart_identicalSpec_isNoop() replayAll(); manager.start(); - Assert.assertEquals( - SupervisorManager.SpecUpdateOutcome.UNCHANGED, - manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec("id1", supervisor1), true) - ); + final SupervisorSpecUpdateResult updateResult = + manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec("id1", supervisor1), true); + Assert.assertFalse(updateResult.isModified()); + Assert.assertFalse(updateResult.isRestarted()); verifyAll(); } @Test public void testCreateOrUpdateSkipRestart_changedRequiringRestart_restartsAndPersists() { + // The restart decision belongs to the running spec, so requireRestart=true is set on the existing spec. final Map existingSpecs = - ImmutableMap.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1)); + ImmutableMap.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1, true)); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); @@ -318,15 +342,60 @@ public void testCreateOrUpdateSkipRestart_changedRequiringRestart_restartsAndPer metadataSupervisorManager.insert(EasyMock.eq("id1"), EasyMock.anyObject()); replayAll(); - final SupervisorSpec updated = new VersionedTestSupervisorSpec("id1", supervisor1, 2, true); - Assert.assertEquals( - SupervisorManager.SpecUpdateOutcome.RESTARTED, - manager.createOrUpdateAndStartSupervisor(updated, true) - ); + final SupervisorSpec updated = new VersionedTestSupervisorSpec("id1", supervisor1, 2); + final SupervisorSpecUpdateResult updateResult = manager.createOrUpdateAndStartSupervisor(updated, true); + Assert.assertTrue(updateResult.isModified()); + Assert.assertTrue(updateResult.isRestarted()); Assert.assertSame(updated, manager.getSupervisorSpec("id1").get()); verifyAll(); } + @Test + public void testCreateOrUpdateSkipRestart_mergesBeforeDiffAndRestartDecision() + { + final MergingVersionedTestSupervisorSpec existing = + new MergingVersionedTestSupervisorSpec("id1", supervisor1, 1); + final Map existingSpecs = ImmutableMap.of("id1", existing); + EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + replayAll(); + + manager.start(); + final MergingVersionedTestSupervisorSpec updated = + new MergingVersionedTestSupervisorSpec("id1", supervisor1, null); + final SupervisorSpecUpdateResult updateResult = manager.createOrUpdateAndStartSupervisor(updated, true); + Assert.assertFalse(updateResult.isModified()); + Assert.assertFalse(updateResult.isRestarted()); + Assert.assertEquals(Integer.valueOf(1), updated.getVersion()); + Assert.assertSame(existing, manager.getSupervisorSpec("id1").get()); + verifyAll(); + } + + @Test + public void testCreateOrUpdateWithoutSkipRestart_forcesRestartWhenSpecUnchanged() + { + final Map existingSpecs = ImmutableMap.of("id1", new TestSupervisorSpec("id1", supervisor1)); + EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + replayAll(); + manager.start(); + verifyAll(); + + resetAll(); + supervisor1.stop(true); + supervisor1.start(); + EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); + replayAll(); + + final SupervisorSpecUpdateResult updateResult = + manager.createOrUpdateAndStartSupervisor(new TestSupervisorSpec("id1", supervisor1), false); + Assert.assertFalse(updateResult.isModified()); + Assert.assertTrue(updateResult.isRestarted()); + verifyAll(); + } + @Test public void testShouldUpdateSupervisorIllegalEvolution() { @@ -348,7 +417,7 @@ public void validateSpecUpdateTo(SupervisorSpec proposedSpec) throws DruidExcept exception.expect(DruidException.class); replayAll(); manager.start(); - manager.shouldUpdateSupervisor(spec2); + manager.createOrUpdateAndStartSupervisor(spec2, true); verifyAll(); } @@ -664,8 +733,8 @@ public void testNoPersistOnFailedStart() manager.start(); try { - manager.createOrUpdateAndStartSupervisor(testSpecOld); - manager.createOrUpdateAndStartSupervisor(testSpecNew); + manager.createOrUpdateAndStartSupervisor(testSpecOld, false); + manager.createOrUpdateAndStartSupervisor(testSpecNew, false); } catch (Exception e) { Assert.assertEquals(testSpecOld, capturedInsert.getValue()); @@ -791,7 +860,7 @@ public void testCreateSuspendResumeAndStopSupervisor() manager.start(); Assert.assertEquals(1, manager.getSupervisorIds().size()); - manager.createOrUpdateAndStartSupervisor(spec); + manager.createOrUpdateAndStartSupervisor(spec, false); Assert.assertEquals(2, manager.getSupervisorIds().size()); Assert.assertEquals(spec, manager.getSupervisorSpec("id1").get()); verifyAll(); @@ -934,22 +1003,22 @@ public void testGetActiveSupervisorIdForDatasourceWithAppendLock() Assert.assertFalse(manager.getActiveSupervisorIdForDatasourceWithAppendLock("nonExistent").isPresent()); - manager.createOrUpdateAndStartSupervisor(noopSupervisorSpec); + manager.createOrUpdateAndStartSupervisor(noopSupervisorSpec, false); Assert.assertFalse(manager.getActiveSupervisorIdForDatasourceWithAppendLock("noopDS").isPresent()); - manager.createOrUpdateAndStartSupervisor(suspendedSpec); + manager.createOrUpdateAndStartSupervisor(suspendedSpec, false); Assert.assertFalse(manager.getActiveSupervisorIdForDatasourceWithAppendLock("suspendedDS").isPresent()); - manager.createOrUpdateAndStartSupervisor(activeSpec); + manager.createOrUpdateAndStartSupervisor(activeSpec, false); Assert.assertFalse(manager.getActiveSupervisorIdForDatasourceWithAppendLock("activeDS").isPresent()); - manager.createOrUpdateAndStartSupervisor(activeAppendSpec); + manager.createOrUpdateAndStartSupervisor(activeAppendSpec, false); Assert.assertTrue(manager.getActiveSupervisorIdForDatasourceWithAppendLock("activeAppendDS").isPresent()); - manager.createOrUpdateAndStartSupervisor(activeSpecWithConcurrentLocks); + manager.createOrUpdateAndStartSupervisor(activeSpecWithConcurrentLocks, false); Assert.assertTrue(manager.getActiveSupervisorIdForDatasourceWithAppendLock("activeConcurrentLocksDS").isPresent()); - manager.createOrUpdateAndStartSupervisor(specWithUseConcurrentLocksFalse); + manager.createOrUpdateAndStartSupervisor(specWithUseConcurrentLocksFalse, false); Assert.assertFalse( manager.getActiveSupervisorIdForDatasourceWithAppendLock("dsWithUseConcurrentLocksFalse").isPresent() ); @@ -995,10 +1064,10 @@ public void testRegisterUpgradedPendingSegmentOnSupervisor() ); manager.start(); - manager.createOrUpdateAndStartSupervisor(noopSpec); + manager.createOrUpdateAndStartSupervisor(noopSpec, false); Assert.assertFalse(manager.registerUpgradedPendingSegmentOnSupervisor("noop", pendingSegment)); - manager.createOrUpdateAndStartSupervisor(streamingSpec); + manager.createOrUpdateAndStartSupervisor(streamingSpec, false); Assert.assertTrue(manager.registerUpgradedPendingSegmentOnSupervisor("sss", pendingSegment)); verifyAll(); @@ -1504,6 +1573,12 @@ static class IOConfig extends SeekableStreamSupervisorIOConfig { super(stream, null, 1, taskCount, null, null, null, false, null, null, null, null, LagAggregator.DEFAULT, null, null, null, null, boundedStreamConfig); } + + @Override + public SupervisorIOConfigBuilder toBuilder() + { + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder().copyFromBase(this); + } } } @@ -1536,13 +1611,45 @@ public int getVersion() } /** - * Lets a test model either a no-restart change (default) or a restart-requiring change. (The default + * Lets a test model either a no-restart change (default) or a restart-requiring change. Invoked on the + * running spec, so this models whether the running supervisor requires a restart. (The default * {@link SupervisorSpec#requireRestart} is conservative and returns true.) */ @Override - public boolean requireRestart(SupervisorSpec old) + public boolean requireRestart(SupervisorSpec proposedSpec) { return requireRestart; } } + + private static class MergingVersionedTestSupervisorSpec extends TestSupervisorSpec + { + @Nullable + private Integer version; + + MergingVersionedTestSupervisorSpec( + final String id, + final Supervisor supervisor, + @Nullable final Integer version + ) + { + super(id, supervisor); + this.version = version; + } + + @JsonProperty + @Nullable + public Integer getVersion() + { + return version; + } + + @Override + public void merge(@Nullable final SupervisorSpec existingSpec) + { + if (version == null && existingSpec instanceof MergingVersionedTestSupervisorSpec) { + version = ((MergingVersionedTestSupervisorSpec) existingSpec).version; + } + } + } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java index 8236c5629d20..a0036a08d911 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java @@ -165,7 +165,7 @@ public List getDataSources() EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, false)) - .andReturn(SupervisorManager.SpecUpdateOutcome.RESTARTED); + .andReturn(SupervisorSpecUpdateResult.of(true, true)); setupMockRequest(); setupMockRequestForAudit(); @@ -180,7 +180,27 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "restarted", true), response.getEntity()); + Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); + resetAll(); + + EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, false)) + .andReturn(SupervisorSpecUpdateResult.of(false, true)); + + setupMockRequest(); + setupMockRequestForAudit(); + + EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true); + auditManager.doAudit(EasyMock.anyObject()); + EasyMock.expectLastCall().once(); + + replayAll(); + + response = supervisorResource.specPost(spec, false, request); + verifyAll(); + + Assert.assertEquals(200, response.getStatus()); + Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", false, "restarted", true), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent()); @@ -241,7 +261,7 @@ public List getDataSources() EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); // Changed but no restart needed: persisted without restarting — and the persist must be audited. EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, true)) - .andReturn(SupervisorManager.SpecUpdateOutcome.PERSISTED_WITHOUT_RESTART); + .andReturn(SupervisorSpecUpdateResult.of(true, false)); setupMockRequest(); setupMockRequestForAudit(); @@ -255,13 +275,31 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "restarted", false), response.getEntity()); + Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", false), response.getEntity()); + + resetAll(); + + EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, true)) + .andReturn(SupervisorSpecUpdateResult.of(false, false)); + + setupMockRequest(); + + EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true); + + replayAll(); + + response = supervisorResource.specPost(spec, true, request); + verifyAll(); + + Assert.assertEquals(200, response.getStatus()); + Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", false, "restarted", false), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, true)) - .andReturn(SupervisorManager.SpecUpdateOutcome.RESTARTED); + .andReturn(SupervisorSpecUpdateResult.of(true, true)); setupMockRequest(); setupMockRequestForAudit(); @@ -276,7 +314,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "restarted", true), response.getEntity()); + Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); } @Test @@ -294,7 +332,7 @@ public List getDataSources() EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, false)) - .andReturn(SupervisorManager.SpecUpdateOutcome.RESTARTED); + .andReturn(SupervisorSpecUpdateResult.of(true, true)); setupMockRequest(); setupMockRequestForAudit(); @@ -308,7 +346,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "restarted", true), response.getEntity()); + Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent()); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSamplerSpecTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSamplerSpecTest.java index 6fd6307e50c7..4a5ba773e892 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSamplerSpecTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamSamplerSpecTest.java @@ -42,6 +42,7 @@ import org.apache.druid.indexing.seekablestream.supervisor.LagAggregator; import org.apache.druid.indexing.seekablestream.supervisor.SeekableStreamSupervisorIOConfig; import org.apache.druid.indexing.seekablestream.supervisor.SeekableStreamSupervisorSpec; +import org.apache.druid.indexing.seekablestream.supervisor.SupervisorIOConfigBuilder; import org.apache.druid.indexing.seekablestream.supervisor.autoscaler.AutoScalerConfig; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.StringUtils; @@ -337,5 +338,11 @@ private TestableSeekableStreamSupervisorIOConfig( null ); } + + @Override + public SupervisorIOConfigBuilder toBuilder() + { + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder().copyFromBase(this); + } } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java index 3eadd7514f3d..d9837e784e86 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorIOConfigTest.java @@ -29,6 +29,7 @@ import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.StringUtils; import org.hamcrest.MatcherAssert; +import org.joda.time.DateTime; import org.joda.time.Duration; import org.joda.time.Period; import org.junit.Assert; @@ -54,7 +55,7 @@ public void testAllDefaults() LagAggregator lagAggregator = mock(LagAggregator.class); InputFormat inputFormat = mock(InputFormat.class); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", inputFormat, null, @@ -131,7 +132,7 @@ private static void assertTaskCount( boolean expectedExplicit ) { - final SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + final SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, 2, @@ -164,7 +165,7 @@ public void testBothLateMessageRejectionPeriodAndStartDateTime() IAE ex = Assert.assertThrows( IAE.class, - () -> new SeekableStreamSupervisorIOConfig( + () -> new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -200,7 +201,7 @@ public void testNullAggregatorThrows() { DruidException ex = Assert.assertThrows( DruidException.class, - () -> new SeekableStreamSupervisorIOConfig( + () -> new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -234,7 +235,7 @@ public void testGetMaxAllowedStopsScalingDisabled() LagAggregator lagAggregator = mock(LagAggregator.class); // Autoscaler disabled, stopTaskCount unset - SeekableStreamSupervisorIOConfig config1 = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config1 = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -259,7 +260,7 @@ public void testGetMaxAllowedStopsScalingDisabled() Assert.assertEquals(7, config1.getMaxAllowedStops()); // Autoscaler disabled, stopTaskCount set - SeekableStreamSupervisorIOConfig config2 = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config2 = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -296,7 +297,7 @@ public void testGetMaxAllowedStopsScalingEnabled() when(autoScalerConfig.getTaskCountStart()).thenReturn(10); when(autoScalerConfig.getStopTaskCountRatio()).thenReturn(0.5); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -330,7 +331,7 @@ public void testGetMaxAllowedStopsScalingEnabled() when(autoScalerConfig.getTaskCountStart()).thenReturn(10); when(autoScalerConfig.getStopTaskCountRatio()).thenReturn(null); - SeekableStreamSupervisorIOConfig config2 = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config2 = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -361,7 +362,7 @@ public void testGetMaxAllowedStopsScalingEnabled() when(autoScalerConfig.getTaskCountStart()).thenReturn(10); when(autoScalerConfig.getStopTaskCountRatio()).thenReturn(null); - SeekableStreamSupervisorIOConfig config3 = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config3 = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -449,7 +450,7 @@ public void testNegativeReplicasThrowsException() private SeekableStreamSupervisorIOConfig makeSeekableStreamSupervisorIOConfig(@Nullable Integer replicas, @Nullable Map serverPriorityToReplicas) { - return new SeekableStreamSupervisorIOConfig( + return new TestableSeekableStreamSupervisorIOConfig( "stream", null, replicas, @@ -482,7 +483,7 @@ public void testBoundedModeWithValidConfig() LagAggregator lagAggregator = mock(LagAggregator.class); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -515,7 +516,7 @@ public void testUnboundedModeByDefault() { LagAggregator lagAggregator = mock(LagAggregator.class); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -547,7 +548,7 @@ public void testBoundedModeWithNullConfig() { LagAggregator lagAggregator = mock(LagAggregator.class); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -628,10 +629,67 @@ public void testEqualsContractCoversAllFields() @Test public void testDefaultLagAggregatorEquals() { - final LagAggregator aggregator = new LagAggregator.DefaultLagAggregator(); - Assert.assertEquals(aggregator, new LagAggregator.DefaultLagAggregator()); - Assert.assertEquals(aggregator.hashCode(), new LagAggregator.DefaultLagAggregator().hashCode()); + // The default aggregator is a stateless singleton; instance() always returns DEFAULT. + final LagAggregator aggregator = LagAggregator.DefaultLagAggregator.instance(); + Assert.assertSame(LagAggregator.DEFAULT, aggregator); + Assert.assertEquals(aggregator, LagAggregator.DefaultLagAggregator.instance()); Assert.assertNotEquals(aggregator, null); Assert.assertNotEquals(aggregator, "not a lag aggregator"); } + + /** + * Concrete subclass for tests that exercise the abstract base directly. Implements the now-abstract + * {@link SeekableStreamSupervisorIOConfig#toBuilder()} via the generic builder. + */ + private static class TestableSeekableStreamSupervisorIOConfig extends SeekableStreamSupervisorIOConfig + { + TestableSeekableStreamSupervisorIOConfig( + String stream, + InputFormat inputFormat, + Integer replicas, + Integer taskCount, + Period taskDuration, + Period startDelay, + Period period, + Boolean useEarliestSequenceNumber, + Period completionTimeout, + Period lateMessageRejectionPeriod, + Period earlyMessageRejectionPeriod, + AutoScalerConfig autoScalerConfig, + LagAggregator lagAggregator, + DateTime lateMessageRejectionStartDateTime, + IdleConfig idleConfig, + Integer stopTaskCount, + Map serverPriorityToReplicas, + BoundedStreamConfig boundedStreamConfig + ) + { + super( + stream, + inputFormat, + replicas, + taskCount, + taskDuration, + startDelay, + period, + useEarliestSequenceNumber, + completionTimeout, + lateMessageRejectionPeriod, + earlyMessageRejectionPeriod, + autoScalerConfig, + lagAggregator, + lateMessageRejectionStartDateTime, + idleConfig, + stopTaskCount, + serverPriorityToReplicas, + boundedStreamConfig + ); + } + + @Override + public SupervisorIOConfigBuilder toBuilder() + { + return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder().copyFromBase(this); + } + } } diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java index 23208e74c826..75a78f36a6fc 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java @@ -899,16 +899,16 @@ public Supervisor createSupervisor() } @Override - protected SeekableStreamSupervisorSpec toggleSuspend(boolean suspend) + protected SeekableStreamSupervisorSpec toggleSuspend(final boolean suspend) { return null; } @Override public SeekableStreamSupervisorSpec createBackfillSpec( - String backfillId, - BoundedStreamConfig boundedStreamConfig, - @Nullable Integer taskCount + final String backfillId, + final BoundedStreamConfig boundedStreamConfig, + @Nullable final Integer taskCount ) { return null; @@ -1429,7 +1429,7 @@ public void testRequireRestart_identicalSpecsDoNotRestart() buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().build(); - Assert.assertFalse(newSpec.requireRestart(oldSpec)); + Assert.assertFalse(oldSpec.requireRestart(newSpec)); } @Test @@ -1439,7 +1439,7 @@ public void testRequireRestart_taskCountChangeWithAutoscalerDoesNotRestart() buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().taskCount(2).build(); final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().taskCount(5).build(); - Assert.assertFalse(newSpec.requireRestart(oldSpec)); + Assert.assertFalse(oldSpec.requireRestart(newSpec)); } @Test @@ -1448,7 +1448,7 @@ public void testRequireRestart_taskCountChangeWithoutAutoscalerRestarts() final TestSeekableStreamSupervisorSpec seed = buildSpecWithIoConfig("id", createIOConfig(2, null)); final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().taskCount(2).build(); final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().taskCount(5).build(); - Assert.assertTrue(newSpec.requireRestart(oldSpec)); + Assert.assertTrue(oldSpec.requireRestart(newSpec)); } @Test @@ -1458,7 +1458,7 @@ public void testRequireRestart_contextChangeRestarts() buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().context(ImmutableMap.of("k", "v")).build(); - Assert.assertTrue(newSpec.requireRestart(oldSpec)); + Assert.assertTrue(oldSpec.requireRestart(newSpec)); } @Test @@ -1468,7 +1468,7 @@ public void testRequireRestart_suspendedChangeRestarts() buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().suspended(false).build(); final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().suspended(true).build(); - Assert.assertTrue(newSpec.requireRestart(oldSpec)); + Assert.assertTrue(oldSpec.requireRestart(newSpec)); } @Test @@ -1478,7 +1478,7 @@ public void testRequireRestart_dataSchemaChangeRestarts() buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().dataSchema(getDataSchema("other-datasource")).build(); - Assert.assertTrue(newSpec.requireRestart(oldSpec)); + Assert.assertTrue(oldSpec.requireRestart(newSpec)); } @Test @@ -1499,7 +1499,7 @@ public void testRequireRestart_disablingAutoscalerWithTaskCountChangeRestarts() buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))).toBuilder().build(); final SeekableStreamSupervisorSpec newSpec = buildSpecWithIoConfig("id", createIOConfig(5, null)).toBuilder().build(); - Assert.assertTrue(newSpec.requireRestart(oldSpec)); + Assert.assertTrue(oldSpec.requireRestart(newSpec)); } @Test @@ -1510,7 +1510,7 @@ public void testRequireRestart_tuningConfigChangeRestarts() final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().tuningConfig(EasyMock.mock(SeekableStreamSupervisorTuningConfig.class)).build(); - Assert.assertTrue(newSpec.requireRestart(oldSpec)); + Assert.assertTrue(oldSpec.requireRestart(newSpec)); } private void assertMergeResult( diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java index 8282e18f9a35..45f07d4a2891 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java @@ -480,15 +480,15 @@ public TestSeekableStreamSupervisorSpec build() ingestionSchema, context, suspended, - taskStorage, - taskMaster, - indexerMetadataStorageCoordinator, - indexTaskClientFactory, - mapper, - emitter, - monitorSchedulerConfig, - rowIngestionMetersFactory, - supervisorStateManagerConfig, + null, + null, + null, + null, + null, + null, + null, + null, + null, supervisor, id ); diff --git a/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java b/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java index 61caa7421b76..95e20c7858cd 100644 --- a/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java +++ b/server/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorSpec.java @@ -141,20 +141,10 @@ default void merge(@Nullable SupervisorSpec existingSpec) } /** - * Given the currently-running {@code old} spec, returns whether replacing it with this spec - * requires the supervisor to be restarted (re-submitted). This is distinct from asking whether - * the two specs are equal: implementations may return {@code false} for differences that do not - * affect the running supervisor (for example, a {@code taskCount} change while autoscaling is - * enabled, since that field is overridden at runtime). - *

- * Only consulted once the two specs are known to differ (see - * {@link SupervisorManager#shouldUpdateSupervisor}); the conservative default treats any - * such difference as requiring a restart. - * - * @param old the currently-running supervisor spec - * @return true if the supervisor must be restarted to apply this spec + * Returns true if replacing this (running) spec with {@code proposedSpec} requires a supervisor restart. + * Invoked on the running spec; default is conservative (always restart). */ - default boolean requireRestart(SupervisorSpec old) + default boolean requireRestart(SupervisorSpec proposedSpec) { return true; } From f16d41b8f1b1b07d1beb4e8c7aea156348df7560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jesse=20Tu=C4=9Flu?= Date: Tue, 30 Jun 2026 00:24:35 -0700 Subject: [PATCH 4/4] more cleanup --- ...estionBackwardCompatibilityDockerTest.java | 8 ++++ .../embedded/indexing/IngestionSmokeTest.java | 43 +++++++++++++++++++ ...abbitStreamSupervisorTuningConfigTest.java | 9 ++-- .../KafkaSupervisorIOConfigTest.java | 4 +- .../kinesis/KinesisSamplerSpecTest.java | 5 ++- .../supervisor/SupervisorManager.java | 41 ++++++++++++------ .../supervisor/SupervisorResource.java | 2 +- .../supervisor/SupervisorManagerTest.java | 12 +++--- .../supervisor/SupervisorResourceTest.java | 12 +++--- .../SeekableStreamSupervisorSpecTest.java | 17 ++++---- .../SeekableStreamSupervisorStateTest.java | 14 +++--- .../SeekableStreamSupervisorTestBase.java | 3 +- 12 files changed, 119 insertions(+), 51 deletions(-) diff --git a/embedded-tests/src/test/java/org/apache/druid/testing/embedded/docker/IngestionBackwardCompatibilityDockerTest.java b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/docker/IngestionBackwardCompatibilityDockerTest.java index ad3855efacd9..00912129fd2b 100644 --- a/embedded-tests/src/test/java/org/apache/druid/testing/embedded/docker/IngestionBackwardCompatibilityDockerTest.java +++ b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/docker/IngestionBackwardCompatibilityDockerTest.java @@ -36,6 +36,7 @@ import org.jboss.netty.handler.codec.http.HttpMethod; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import java.util.Map; @@ -120,6 +121,13 @@ protected void validateSupervisorUpdateResponse(Map startSupervi Assertions.assertEquals(Map.of("id", supervisorId), startSupervisorResult); } + @Override + @Disabled("modified/restarted response semantics and skipRestartIfUnmodified are not supported by the old Overlord") + public void test_kafkaSupervisor_modifiedAndRestartedCombinations() + { + // No-op: the older Overlord returns only {id} and always restarts on submission. + } + @Override protected void waitForNextCoordinatorCacheSync() { diff --git a/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java index 5266e4aafd19..e9033a5375a4 100644 --- a/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java +++ b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/indexing/IngestionSmokeTest.java @@ -59,6 +59,7 @@ import org.apache.kafka.clients.producer.ProducerRecord; import org.joda.time.DateTime; import org.joda.time.Interval; +import org.joda.time.Period; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -325,6 +326,37 @@ public void test_runKafkaSupervisor() Assertions.assertTrue(supervisorStatus.isSuspended()); } + @Test + public void test_kafkaSupervisor_modifiedAndRestartedCombinations() + { + final String topic = dataSource; + kafkaServer.createTopicWithPartitions(topic, 2); + + final String supervisorId = dataSource; + + // (1) First submission of a new supervisor: persisted and started. + Assertions.assertEquals( + Map.of("id", supervisorId, "modified", "true", "restarted", "true"), + postKafkaSupervisor(createKafkaSupervisor(topic, Period.millis(500))) + ); + + // (2) Resubmitting a byte-identical spec is a no-op (the client always sets skipRestartIfUnmodified). + Assertions.assertEquals( + Map.of("id", supervisorId, "modified", "false", "restarted", "false"), + postKafkaSupervisor(createKafkaSupervisor(topic, Period.millis(500))) + ); + + // (3) A restart-requiring change (taskDuration): persisted and restarted. + Assertions.assertEquals( + Map.of("id", supervisorId, "modified", "true", "restarted", "true"), + postKafkaSupervisor(createKafkaSupervisor(topic, Period.seconds(10))) + ); + + // The (modified=true, restarted=false) case needs the autoscaler to mutate taskCount at runtime + // (a plain resubmit is reset to taskCountStart by merge()), so it is covered deterministically in + // SupervisorManagerTest#testCreateOrUpdateSkipRestart_changedNoRestart_persistsWithoutRestart. + } + @Test public void test_streamLogs_ofCancelledTask() throws Exception { @@ -368,6 +400,11 @@ public void test_streamLogs_ofCancelledTask() throws Exception } private KafkaSupervisorSpec createKafkaSupervisor(String topic) + { + return createKafkaSupervisor(topic, Period.millis(500)); + } + + private KafkaSupervisorSpec createKafkaSupervisor(String topic, Period taskDuration) { return MoreResources.Supervisor.KAFKA_JSON .get() @@ -376,11 +413,17 @@ private KafkaSupervisorSpec createKafkaSupervisor(String topic) ioConfig -> ioConfig .withConsumerProperties(kafkaServer.consumerProperties()) .withInputFormat(new CsvInputFormat(List.of("timestamp", "item"), null, null, false, 0, false)) + .withTaskDuration(taskDuration) ) .withTuningConfig(tuningConfig -> tuningConfig.withMaxRowsPerSegment(1)) .build(dataSource, topic); } + private Map postKafkaSupervisor(KafkaSupervisorSpec spec) + { + return cluster.callApi().onLeaderOverlord(o -> o.postSupervisor(spec)); + } + private List> generateRecordsForTopic( String topic, int numRecords, diff --git a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java index ef6aadeb3047..f4846573d3e3 100644 --- a/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java +++ b/extensions-contrib/rabbit-stream-indexing-service/src/test/java/org/apache/druid/indexing/rabbitstream/supervisor/RabbitStreamSupervisorTuningConfigTest.java @@ -21,8 +21,6 @@ import com.fasterxml.jackson.databind.Module; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import org.apache.druid.data.input.impl.DimensionsSpec; import org.apache.druid.data.input.impl.TimestampSpec; import org.apache.druid.indexer.granularity.UniformGranularitySpec; @@ -40,6 +38,9 @@ import org.junit.Test; import org.junit.rules.ExpectedException; +import java.util.List; +import java.util.Map; + public class RabbitStreamSupervisorTuningConfigTest { private final ObjectMapper mapper; @@ -162,7 +163,7 @@ private DataSchema dataSchema() new UniformGranularitySpec( Granularities.HOUR, Granularities.NONE, - ImmutableList.of() + List.of() ) ) .build(); @@ -185,7 +186,7 @@ private RabbitStreamSupervisorTuningConfig tuningConfig( ) { return mapper.convertValue( - ImmutableMap.of( + Map.of( "type", "rabbit", "recordBufferSize", recordBufferSize, "recordBufferOfferTimeout", recordBufferOfferTimeout, diff --git a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java index d372aa7e9e36..0bf2e7a30e24 100644 --- a/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java +++ b/extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorIOConfigTest.java @@ -631,7 +631,7 @@ private static KafkaIOConfigBuilder ioConfigBuilder() { return new KafkaIOConfigBuilder() .withTopic("topic") - .withConsumerProperties(ImmutableMap.of("bootstrap.servers", "localhost:9092")) + .withConsumerProperties(Map.of("bootstrap.servers", "localhost:9092")) .withReplicas(1) .withTaskCount(2) .withTaskDuration(new Period("PT1H")); @@ -650,7 +650,7 @@ public void testEqualsAndHashCode() Assert.assertNotEquals(config, ioConfigBuilder().withTaskCount(9).build()); Assert.assertNotEquals( config, - ioConfigBuilder().withConsumerProperties(ImmutableMap.of("bootstrap.servers", "other:9092")).build() + ioConfigBuilder().withConsumerProperties(Map.of("bootstrap.servers", "other:9092")).build() ); Assert.assertNotEquals(config, ioConfigBuilder().withEmitTimeLagMetrics(true).build()); } diff --git a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java index b24f0385f1cd..5513a7ddd2f3 100644 --- a/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java +++ b/extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/KinesisSamplerSpecTest.java @@ -62,6 +62,7 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; +import java.util.Map; public class KinesisSamplerSpecTest extends EasyMockSupport { @@ -121,7 +122,7 @@ public void testSample() throws InterruptedException null, new KinesisIOConfigBuilder() .withStream(STREAM) - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withUseEarliestSequenceNumber(true) .build(), null, @@ -158,7 +159,7 @@ public void testGetInputSourceResources() null, new KinesisIOConfigBuilder() .withStream(STREAM) - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withUseEarliestSequenceNumber(true) .build(), null, diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java index 5dbf7617c0ae..9f6f24ed74f1 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java @@ -207,26 +207,29 @@ public SupervisorSpecUpdateResult createOrUpdateAndStartSupervisor( return SupervisorSpecUpdateResult.of(true, true); } - if (!isSpecChangedAndValidate(spec)) { + if (!isSpecChanged(spec, current.rhs)) { return SupervisorSpecUpdateResult.of(false, false); } // merge() may carry forward omitted fields (e.g. taskCount); compare the effective spec. spec.merge(current.rhs); - final boolean specChanged = isSpecChangedAndValidate(spec); - if (!specChanged) { + if (!isSpecChanged(spec, current.rhs)) { return SupervisorSpecUpdateResult.of(false, false); } + + // The effective (merged) spec is what will be persisted, so validate that transition exactly once. + current.rhs.validateSpecUpdateTo(spec); + if (!current.rhs.requireRestart(spec)) { metadataSupervisorManager.insert(spec.getId(), spec); supervisors.put(spec.getId(), Pair.of(current.lhs, spec)); return SupervisorSpecUpdateResult.of(true, false); } - // Restart path: stop+recreate, persisting the spec when it changed. + // Restart path: stop+recreate, persisting the changed spec. possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); - createAndStartSupervisorInternal(spec, specChanged); - return SupervisorSpecUpdateResult.of(specChanged, true); + createAndStartSupervisorInternal(spec, true); + return SupervisorSpecUpdateResult.of(true, true); } } @@ -234,20 +237,32 @@ public SupervisorSpecUpdateResult createOrUpdateAndStartSupervisor( private boolean isSpecChangedAndValidate(SupervisorSpec spec) { final Pair current = supervisors.get(spec.getId()); - if (current == null || current.rhs == null) { + final SupervisorSpec currentSpec = current == null ? null : current.rhs; + if (!isSpecChanged(spec, currentSpec)) { + return false; + } + if (currentSpec != null) { + currentSpec.validateSpecUpdateTo(spec); + } + return true; + } + + /** + * Byte-level diff of {@code spec} against {@code against}. A missing {@code against} (new supervisor) + * counts as changed. Does not validate the transition. + */ + private boolean isSpecChanged(SupervisorSpec spec, @Nullable SupervisorSpec against) + { + if (against == null) { return true; } try { - if (Arrays.equals(jsonMapper.writeValueAsBytes(spec), jsonMapper.writeValueAsBytes(current.rhs))) { - return false; - } + return !Arrays.equals(jsonMapper.writeValueAsBytes(spec), jsonMapper.writeValueAsBytes(against)); } catch (JsonProcessingException ex) { - log.warn("Failed to write spec as bytes for spec_id[%s]", spec.getId()); + log.warn(ex, "Failed to write spec as bytes for spec_id[%s]", spec.getId()); return true; } - current.rhs.validateSpecUpdateTo(spec); - return true; } public boolean stopAndRemoveSupervisor(String id) diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java index 863ab5719ef2..10297e25a88e 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -173,7 +173,7 @@ public Response specPost( } return Response.ok( - ImmutableMap.of( + Map.of( "id", spec.getId(), "modified", updateResult.isModified(), "restarted", updateResult.isRestarted() diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java index e9bc897c05c9..742dd43e5ac4 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManagerTest.java @@ -241,7 +241,7 @@ public void testCreateOrUpdateAndStartSupervisorNullSpecId() public void testUnchangedSpecDoesNotRestart() { SupervisorSpec spec = new TestSupervisorSpec("id1", supervisor1); - Map existingSpecs = ImmutableMap.of("id1", spec); + Map existingSpecs = Map.of("id1", spec); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); @@ -283,7 +283,7 @@ public void testCreateOrUpdateSkipRestart_changedNoRestart_persistsWithoutRestar { final Capture capturedInsert = Capture.newInstance(); final Map existingSpecs = - ImmutableMap.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1)); + Map.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1)); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); @@ -305,7 +305,7 @@ public void testCreateOrUpdateSkipRestart_changedNoRestart_persistsWithoutRestar @Test public void testCreateOrUpdateSkipRestart_identicalSpec_isNoop() { - final Map existingSpecs = ImmutableMap.of("id1", new TestSupervisorSpec("id1", supervisor1)); + final Map existingSpecs = Map.of("id1", new TestSupervisorSpec("id1", supervisor1)); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); @@ -325,7 +325,7 @@ public void testCreateOrUpdateSkipRestart_changedRequiringRestart_restartsAndPer { // The restart decision belongs to the running spec, so requireRestart=true is set on the existing spec. final Map existingSpecs = - ImmutableMap.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1, true)); + Map.of("id1", new VersionedTestSupervisorSpec("id1", supervisor1, 1, true)); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); @@ -355,7 +355,7 @@ public void testCreateOrUpdateSkipRestart_mergesBeforeDiffAndRestartDecision() { final MergingVersionedTestSupervisorSpec existing = new MergingVersionedTestSupervisorSpec("id1", supervisor1, 1); - final Map existingSpecs = ImmutableMap.of("id1", existing); + final Map existingSpecs = Map.of("id1", existing); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); @@ -375,7 +375,7 @@ public void testCreateOrUpdateSkipRestart_mergesBeforeDiffAndRestartDecision() @Test public void testCreateOrUpdateWithoutSkipRestart_forcesRestartWhenSpecUnchanged() { - final Map existingSpecs = ImmutableMap.of("id1", new TestSupervisorSpec("id1", supervisor1)); + final Map existingSpecs = Map.of("id1", new TestSupervisorSpec("id1", supervisor1)); EasyMock.expect(metadataSupervisorManager.getLatest()).andReturn(existingSpecs); supervisor1.start(); EasyMock.expect(supervisor1.createAutoscaler(EasyMock.anyObject())).andReturn(null).anyTimes(); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java index a0036a08d911..1ed1214368b8 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResourceTest.java @@ -180,7 +180,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); + Assert.assertEquals(Map.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); @@ -200,7 +200,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", false, "restarted", true), response.getEntity()); + Assert.assertEquals(Map.of("id", "my-id", "modified", false, "restarted", true), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent()); @@ -275,7 +275,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", false), response.getEntity()); + Assert.assertEquals(Map.of("id", "my-id", "modified", true, "restarted", false), response.getEntity()); resetAll(); @@ -293,7 +293,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", false, "restarted", false), response.getEntity()); + Assert.assertEquals(Map.of("id", "my-id", "modified", false, "restarted", false), response.getEntity()); resetAll(); @@ -314,7 +314,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); + Assert.assertEquals(Map.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); } @Test @@ -346,7 +346,7 @@ public List getDataSources() verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); + Assert.assertEquals(Map.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent()); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java index 75a78f36a6fc..2b54f720e529 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorSpecTest.java @@ -58,6 +58,7 @@ import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -675,7 +676,7 @@ public void testSeekableStreamSupervisorSpecWithScaleInThresholdGreaterThanParti // supervisor's bounds and the scaler's bounds agree. EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskDuration(new Period("PT1H")) .withStartDelay(new Period("P1D")) @@ -742,7 +743,7 @@ public void testSeekableStreamSupervisorSpecWithScaleInAlreadyAtMin() throws Int scaleInProps.put("taskCountMin", 2); final SeekableStreamSupervisorIOConfig customIoConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskDuration(new Period("PT1H")) .withStartDelay(new Period("P1D")) @@ -812,7 +813,7 @@ public void testSeekableStreamSupervisorSpecWithScaleDisable() throws Interrupte { SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) @@ -861,7 +862,7 @@ public void testEnablingIdleBeviourPerSupervisorWithOverlordConfigEnabled() { SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) @@ -1457,7 +1458,7 @@ public void testRequireRestart_contextChangeRestarts() final TestSeekableStreamSupervisorSpec seed = buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); final SeekableStreamSupervisorSpec oldSpec = seed.toBuilder().build(); - final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().context(ImmutableMap.of("k", "v")).build(); + final SeekableStreamSupervisorSpec newSpec = seed.toBuilder().context(Map.of("k", "v")).build(); Assert.assertTrue(oldSpec.requireRestart(newSpec)); } @@ -1486,7 +1487,7 @@ public void testRequireRestart_differentSpecTypeRestarts() { final TestSeekableStreamSupervisorSpec seed = buildSpecWithIoConfig("id", createIOConfig(2, lagBasedAutoScalerConfig(1, 8, null))); - Assert.assertTrue(seed.toBuilder().build().requireRestart(new NoopSupervisorSpec("id", ImmutableList.of("ds")))); + Assert.assertTrue(seed.toBuilder().build().requireRestart(new NoopSupervisorSpec("id", List.of("ds")))); } @Test @@ -1594,7 +1595,7 @@ private SeekableStreamSupervisorIOConfig getIOConfig(boolean scaleOut, int maxTa // taskCountStart/taskCountMin. return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskDuration(new Period("PT1H")) .withStartDelay(new Period("P1D")) @@ -1662,7 +1663,7 @@ public void testBoundedStreamSupervisorSpec_runsWithBoundedConfig() SeekableStreamSupervisorIOConfig boundedIoConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java index 32139471fc62..0dc0598f285f 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorStateTest.java @@ -687,7 +687,7 @@ public void testIdleStateTransition() throws Exception EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) @@ -787,7 +787,7 @@ public void testIdleOnStartUpAndTurnsToRunningAfterLagUpdates() EasyMock.expect(spec.getContextValue("tags")).andReturn("").anyTimes(); EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) @@ -1079,7 +1079,7 @@ public void testCheckpointForActiveTaskGroup() throws InterruptedException, Json DateTime startTime = DateTimes.nowUtc(); SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream(STREAM) - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) @@ -1292,7 +1292,7 @@ public void testEarlyStoppingOfTaskGroupBasedOnStopTaskCount() throws Interrupte int stopTaskCount = 1; SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream(STREAM) - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(3) .withTaskDuration(new Period("PT1H")) @@ -1521,7 +1521,7 @@ public void testSupervisorStopTaskGroupEarly() throws JsonProcessingException, I DateTime startTime = DateTimes.nowUtc(); SeekableStreamSupervisorIOConfig ioConfig = new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream(STREAM) - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) @@ -2694,7 +2694,7 @@ private void expectEmitterSupervisor(boolean suspended) EasyMock.expect(spec.getDataSchema()).andReturn(getDataSchema()).anyTimes(); EasyMock.expect(spec.getIoConfig()).andReturn(new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(1) .withTaskDuration(new Period("PT1H")) @@ -3060,7 +3060,7 @@ private static SeekableStreamSupervisorIOConfig createSupervisorIOConfig( { return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream("stream") - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(serverPriorityToReplicas == null ? 1 : serverPriorityToReplicas.values().stream().mapToInt(Integer::intValue).sum()) diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java index 45f07d4a2891..968e8a75092d 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisorTestBase.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import org.apache.druid.data.input.impl.ByteEntity; import org.apache.druid.data.input.impl.DimensionSchema; import org.apache.druid.data.input.impl.JsonInputFormat; @@ -609,7 +608,7 @@ public static SeekableStreamSupervisorIOConfig createIOConfig( { return new SupervisorIOConfigBuilder.DefaultSupervisorIOConfigBuilder() .withStream(STREAM) - .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, ImmutableList.of()), ImmutableMap.of(), false, false, false)) + .withInputFormat(new JsonInputFormat(new JSONPathSpec(true, List.of()), Map.of(), false, false, false)) .withReplicas(1) .withTaskCount(taskCount) .withTaskDuration(new Period("PT1H"))