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 934f67b0dda4..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, @@ -424,7 +467,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/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/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..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 @@ -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, + null, + null, + null, + null, + null, + null, + null, + null, + null + ); + } + } } 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/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 0dca7f084f0a..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,15 +22,24 @@ 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; +import org.joda.time.Period; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import static org.easymock.EasyMock.createMock; + public class RabbitStreamSupervisorIOConfigTest { private final ObjectMapper mapper; @@ -212,4 +221,67 @@ 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"); + } + + /** + * 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-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-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..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,9 +21,15 @@ import com.fasterxml.jackson.databind.Module; import com.fasterxml.jackson.databind.ObjectMapper; +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; @@ -32,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; @@ -45,6 +54,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 +142,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, + List.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( + Map.of( + "type", "rabbit", + "recordBufferSize", recordBufferSize, + "recordBufferOfferTimeout", recordBufferOfferTimeout, + "maxRecordsPerPoll", maxRecordsPerPoll + ), + RabbitStreamSupervisorTuningConfig.class + ); + } + } 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..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 @@ -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, + null, + null, + null, + null, + null, + null, + null, + null, + null + ); + } + } } 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..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 @@ -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; @@ -618,4 +626,79 @@ 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(Map.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(Map.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()); + } + + /** + * 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/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..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 @@ -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, + null, + null, + null, + null, + null, + null, + null, + null, + awsCredentialsConfig, + null + ); + } + } + @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..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 @@ -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; @@ -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 { @@ -119,30 +120,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, List.of()), Map.of(), false, false, false)) + .withUseEarliestSequenceNumber(true) + .build(), null, null, null, @@ -175,30 +157,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, List.of()), Map.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..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,16 +21,25 @@ 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; +import org.joda.time.Period; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import static org.easymock.EasyMock.createMock; + public class KinesisSupervisorIOConfigTest { private final ObjectMapper mapper; @@ -204,4 +213,68 @@ 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"); + } + + /** + * 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/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/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 fa7d96634ae6..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 @@ -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,47 +188,81 @@ public boolean createOrUpdateAndStartSupervisor(SupervisorSpec spec) synchronized (lock) { Preconditions.checkState(started, "SupervisorManager not started"); - final boolean shouldUpdateSpec = shouldUpdateSupervisor(spec); - SupervisorSpec existingSpec = possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); - spec.merge(existingSpec); - createAndStartSupervisorInternal(spec, shouldUpdateSpec); - return shouldUpdateSpec; + + 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); + } + + final Pair current = supervisors.get(spec.getId()); + final boolean isNew = current == null || current.rhs == null; + + if (isNew) { + spec.merge(null); + createAndStartSupervisorInternal(spec, true); + return SupervisorSpecUpdateResult.of(true, true); + } + + 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); + 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 changed spec. + possiblyStopAndRemoveSupervisorInternal(spec.getId(), false); + createAndStartSupervisorInternal(spec, true); + return SupervisorSpecUpdateResult.of(true, true); } } + /** 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()); + final SupervisorSpec currentSpec = current == null ? null : current.rhs; + if (!isSpecChanged(spec, currentSpec)) { + return false; + } + if (currentSpec != null) { + currentSpec.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 - * - * @param spec The spec submitted - * @return boolean - true only if the spec has been modified, false otherwise + * Byte-level diff of {@code spec} against {@code against}. A missing {@code against} (new supervisor) + * counts as changed. Does not validate the transition. */ - public boolean shouldUpdateSupervisor(SupervisorSpec spec) + private boolean isSpecChanged(SupervisorSpec spec, @Nullable SupervisorSpec against) { - 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"); - 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; - } - } - catch (JsonProcessingException ex) { - log.warn("Failed to write spec as bytes for spec_id[%s]", spec.getId()); - } + if (against == null) { + return true; + } + try { + return !Arrays.equals(jsonMapper.writeValueAsBytes(spec), jsonMapper.writeValueAsBytes(against)); + } + catch (JsonProcessingException ex) { + log.warn(ex, "Failed to write spec as bytes for spec_id[%s]", spec.getId()); + return true; } - return true; } public boolean stopAndRemoveSupervisor(String id) @@ -428,7 +464,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 8d0e04eb7988..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 @@ -165,29 +165,40 @@ public Response specPost( .build(); } - if (Boolean.TRUE.equals(skipRestartIfUnmodified) && !manager.shouldUpdateSupervisor(spec)) { - return Response.ok(ImmutableMap.of("id", spec.getId(), "restarted", false)).build(); - } + final SupervisorSpecUpdateResult updateResult = + 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() - ); + if (updateResult.isModified() || updateResult.isRestarted()) { + auditSupervisorUpdate(spec, req); + } - return Response.ok(ImmutableMap.of("id", spec.getId(), "restarted", true)).build(); + return Response.ok( + Map.of( + "id", spec.getId(), + "modified", updateResult.isModified(), + "restarted", updateResult.isRestarted() + ) + ).build(); } ); } + /** Audits supervisor spec submissions that changed or restarted the supervisor. */ + 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/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/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..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,6 +49,28 @@ public interface LagAggregator */ class DefaultLagAggregator implements LagAggregator { + public DefaultLagAggregator() + { + } + + @JsonCreator + public static DefaultLagAggregator instance() + { + return (DefaultLagAggregator) DEFAULT; + } + + @Override + public boolean equals(Object o) + { + return o instanceof DefaultLagAggregator; + } + + @Override + public int hashCode() + { + return DefaultLagAggregator.class.hashCode(); + } + @Override public LagStats aggregate(Map partitionLags) { 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..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 @@ -35,6 +35,7 @@ import javax.annotation.Nullable; import java.util.Map; +import java.util.Objects; public abstract class SeekableStreamSupervisorIOConfig @@ -316,4 +317,62 @@ 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 autoScalerEnabled are construction hints, not part of 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 + ); + } + + 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 6fbf917b774c..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 @@ -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,25 @@ public SeekableStreamSupervisorTuningConfig getTuningConfig() { return tuningConfig; } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + 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..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 @@ -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,56 @@ public void merge(@Nullable SupervisorSpec existingSpec) } } + @Override + public boolean requireRestart(SupervisorSpec proposedSpec) + { + if (!(proposedSpec instanceof SeekableStreamSupervisorSpec proposed)) { + return true; + } + + final Builder proposedCopy; + try { + proposedCopy = proposed.toBuilder(); + } + catch (UnsupportedOperationException e) { + return true; + } + + if (isAutoScalerEnabled() || proposed.isAutoScalerEnabled()) { + proposedCopy.taskCount(getIoConfig().getTaskCount()); + } + + return !proposedCopy.build().equals(this); + } + + 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; + 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 +355,87 @@ public abstract SeekableStreamSupervisorSpec createBackfillSpec( @Nullable Integer taskCount ); + /** + * Copy builder for restart comparison. Subclasses override; default requires restart on any change. + */ + public Builder toBuilder() + { + throw new UnsupportedOperationException("toBuilder() not implemented"); + } + + @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 abstract T self(); + + public abstract SeekableStreamSupervisorSpec build(); + + 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; + 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 (does not mutate the builder's current ioConfig reference). + */ + 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 53% 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..d0ebe35a1fb1 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,99 @@ 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 + ) + { + @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 199e004b4243..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 @@ -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,167 @@ 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 = Map.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 + public void testCreateOrUpdateSkipRestart_changedNoRestart_persistsWithoutRestart() + { + final Capture capturedInsert = Capture.newInstance(); + final Map existingSpecs = + 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(); + // Persist is expected; supervisor1.stop(...) is NOT expected, so verifyAll() asserts no restart. + metadataSupervisorManager.insert(EasyMock.eq("id1"), EasyMock.capture(capturedInsert)); + replayAll(); + + manager.start(); + // version differs (byte change) but requireRestart() is false. + final SupervisorSpec updated = new VersionedTestSupervisorSpec("id1", supervisor1, 2); + 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(); + } + + @Test + public void testCreateOrUpdateSkipRestart_identicalSpec_isNoop() + { + 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(); + // No insert and no stop expected for an identical spec. + replayAll(); + + manager.start(); + 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 = + 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(); + 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); + 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 = Map.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 = Map.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 @@ -275,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(); } @@ -591,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()); @@ -718,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(); @@ -861,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() ); @@ -922,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(); @@ -1392,6 +1534,12 @@ public SeekableStreamSupervisorIOConfig getIoConfig() return getSpec().getIOConfig(); } + @Override + public Builder toBuilder() + { + throw new UnsupportedOperationException(); + } + @JsonTypeName("testBackfillIngestionSpec") static class IngestionSpec extends SeekableStreamSupervisorIngestionSpec { @@ -1425,6 +1573,83 @@ 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); + } + } + } + + /** + * 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; + 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 + public int getVersion() + { + return version; + } + + /** + * 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 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 31e0d604a222..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 @@ -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(SupervisorSpecUpdateResult.of(true, true)); setupMockRequest(); setupMockRequestForAudit(); @@ -179,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(Map.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(Map.of("id", "my-id", "modified", false, "restarted", true), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent()); @@ -238,24 +259,47 @@ public List getDataSources() }; EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.of(supervisorManager)); - EasyMock.expect(supervisorManager.shouldUpdateSupervisor(spec)).andReturn(false); + // Changed but no restart needed: persisted without restarting — and the persist must be audited. + EasyMock.expect(supervisorManager.createOrUpdateAndStartSupervisor(spec, true)) + .andReturn(SupervisorSpecUpdateResult.of(true, false)); setupMockRequest(); + setupMockRequestForAudit(); EasyMock.expect(authConfig.isEnableInputSourceSecurity()).andReturn(true); + auditManager.doAudit(EasyMock.anyObject()); + EasyMock.expectLastCall().once(); replayAll(); Response response = supervisorResource.specPost(spec, true, request); verifyAll(); Assert.assertEquals(200, response.getStatus()); - Assert.assertEquals(ImmutableMap.of("id", "my-id", "restarted", false), response.getEntity()); + Assert.assertEquals(Map.of("id", "my-id", "modified", true, "restarted", false), response.getEntity()); 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(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(Map.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(SupervisorSpecUpdateResult.of(true, true)); setupMockRequest(); setupMockRequestForAudit(); @@ -270,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(Map.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); } @Test @@ -287,7 +331,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(SupervisorSpecUpdateResult.of(true, true)); setupMockRequest(); setupMockRequestForAudit(); @@ -301,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(Map.of("id", "my-id", "modified", true, "restarted", true), response.getEntity()); resetAll(); EasyMock.expect(taskMaster.getSupervisorManager()).andReturn(Optional.absent()); @@ -1773,6 +1818,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/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 ec60d5753f74..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 @@ -19,6 +19,8 @@ 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; @@ -27,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; @@ -52,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, @@ -129,7 +132,7 @@ private static void assertTaskCount( boolean expectedExplicit ) { - final SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + final SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, 2, @@ -162,7 +165,7 @@ public void testBothLateMessageRejectionPeriodAndStartDateTime() IAE ex = Assert.assertThrows( IAE.class, - () -> new SeekableStreamSupervisorIOConfig( + () -> new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -198,7 +201,7 @@ public void testNullAggregatorThrows() { DruidException ex = Assert.assertThrows( DruidException.class, - () -> new SeekableStreamSupervisorIOConfig( + () -> new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -232,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, @@ -257,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, @@ -294,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, @@ -328,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, @@ -359,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, @@ -447,7 +450,7 @@ public void testNegativeReplicasThrowsException() private SeekableStreamSupervisorIOConfig makeSeekableStreamSupervisorIOConfig(@Nullable Integer replicas, @Nullable Map serverPriorityToReplicas) { - return new SeekableStreamSupervisorIOConfig( + return new TestableSeekableStreamSupervisorIOConfig( "stream", null, replicas, @@ -480,7 +483,7 @@ public void testBoundedModeWithValidConfig() LagAggregator lagAggregator = mock(LagAggregator.class); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -513,7 +516,7 @@ public void testUnboundedModeByDefault() { LagAggregator lagAggregator = mock(LagAggregator.class); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -545,7 +548,7 @@ public void testBoundedModeWithNullConfig() { LagAggregator lagAggregator = mock(LagAggregator.class); - SeekableStreamSupervisorIOConfig config = new SeekableStreamSupervisorIOConfig( + SeekableStreamSupervisorIOConfig config = new TestableSeekableStreamSupervisorIOConfig( "stream", null, null, @@ -571,4 +574,122 @@ 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(); + } + + /** + * 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() + { + // 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 80120d07fdf5..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 @@ -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; @@ -57,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; @@ -672,28 +674,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, List.of()), Map.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 +741,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, List.of()), Map.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 +811,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, List.of()), Map.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 +860,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, List.of()), Map.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( @@ -937,21 +900,27 @@ 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; } + @Override + public Builder toBuilder() + { + throw new UnsupportedOperationException(); + } + @Override public String getType() { @@ -1454,6 +1423,97 @@ 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(oldSpec.requireRestart(newSpec)); + } + + @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(oldSpec.requireRestart(newSpec)); + } + + @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(oldSpec.requireRestart(newSpec)); + } + + @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(Map.of("k", "v")).build(); + Assert.assertTrue(oldSpec.requireRestart(newSpec)); + } + + @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(oldSpec.requireRestart(newSpec)); + } + + @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(oldSpec.requireRestart(newSpec)); + } + + @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", List.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(oldSpec.requireRestart(newSpec)); + } + + @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(oldSpec.requireRestart(newSpec)); + } + private void assertMergeResult( @Nullable TestSeekableStreamSupervisorSpec existingSpec, TestSeekableStreamSupervisorSpec newSpec, @@ -1527,53 +1587,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, List.of()), Map.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 +1661,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, List.of()), Map.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..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 @@ -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, List.of()), Map.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, List.of()), Map.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, List.of()), Map.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, List.of()), Map.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, List.of()), Map.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, List.of()), Map.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, List.of()), Map.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..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; @@ -446,6 +445,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, + null, + null, + null, + null, + null, + null, + null, + null, + null, + supervisor, + id + ); + } + } } protected static SeekableStreamSupervisorTuningConfig getTuningConfig() @@ -559,28 +606,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, List.of()), Map.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..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 @@ -139,4 +139,13 @@ default void merge(@Nullable SupervisorSpec existingSpec) { // No-op by default } + + /** + * 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 proposedSpec) + { + return true; + } }