From 920a03d4e81e7c290a30441614b36258d8b5ca6a Mon Sep 17 00:00:00 2001 From: Chris Egerton Date: Wed, 16 Feb 2022 00:01:26 -0500 Subject: [PATCH] KAFKA-10000: Zombie fencing logic --- checkstyle/suppressions.xml | 2 +- .../auth/extension/JaasBasicAuthFilter.java | 33 +- .../extension/JaasBasicAuthFilterTest.java | 36 +- .../apache/kafka/connect/runtime/Herder.java | 12 + .../apache/kafka/connect/runtime/Worker.java | 115 ++- .../distributed/DistributedHerder.java | 445 +++++++++- .../connect/runtime/isolation/LoaderSwap.java | 36 + .../connect/runtime/isolation/Plugins.java | 10 + .../connect/runtime/rest/RestClient.java | 17 +- .../rest/resources/ConnectorsResource.java | 11 + .../runtime/standalone/StandaloneHerder.java | 5 + .../connect/storage/ClusterConfigState.java | 46 + .../connect/storage/ConfigBackingStore.java | 7 + .../storage/KafkaConfigBackingStore.java | 493 ++++++----- .../storage/MemoryConfigBackingStore.java | 11 +- .../kafka/connect/util/ConnectUtils.java | 10 + .../connect/runtime/AbstractHerderTest.java | 4 +- .../kafka/connect/runtime/WorkerTest.java | 63 +- .../connect/runtime/WorkerTestUtils.java | 3 + .../distributed/DistributedHerderTest.java | 798 ++++++++++++++++-- .../IncrementalCooperativeAssignorTest.java | 3 + .../distributed/WorkerCoordinatorTest.java | 9 + .../resources/ConnectorsResourceTest.java | 64 ++ .../standalone/StandaloneHerderTest.java | 16 +- .../storage/KafkaConfigBackingStoreTest.java | 148 +++- gradle/spotbugs-exclude.xml | 10 + 26 files changed, 2022 insertions(+), 385 deletions(-) create mode 100644 connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/LoaderSwap.java diff --git a/checkstyle/suppressions.xml b/checkstyle/suppressions.xml index 773af1bc1b8b2..f2c24541a1114 100644 --- a/checkstyle/suppressions.xml +++ b/checkstyle/suppressions.xml @@ -137,7 +137,7 @@ files="ConfigKeyInfo.java"/> + files="(RestServer|AbstractHerder|DistributedHerder|Worker).java"/> diff --git a/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java b/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java index 0299cbba0b546..ff12a384b7628 100644 --- a/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java +++ b/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java @@ -18,7 +18,11 @@ package org.apache.kafka.connect.rest.basic.auth.extension; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.function.Predicate; import java.util.regex.Pattern; import javax.security.auth.login.Configuration; import javax.ws.rs.HttpMethod; @@ -45,7 +49,10 @@ public class JaasBasicAuthFilter implements ContainerRequestFilter { private static final Logger log = LoggerFactory.getLogger(JaasBasicAuthFilter.class); - private static final Pattern TASK_REQUEST_PATTERN = Pattern.compile("/?connectors/([^/]+)/tasks/?"); + private static final Set INTERNAL_REQUEST_MATCHERS = new HashSet<>(Arrays.asList( + new RequestMatcher(HttpMethod.POST, "/?connectors/([^/]+)/tasks/?"), + new RequestMatcher(HttpMethod.PUT, "/?connectors/[^/]+/fence/?") + )); private static final String CONNECT_LOGIN_MODULE = "KafkaConnect"; static final String AUTHORIZATION = "Authorization"; @@ -53,13 +60,29 @@ public class JaasBasicAuthFilter implements ContainerRequestFilter { // Package-private for testing final Configuration configuration; + private static class RequestMatcher implements Predicate { + private final String method; + private final Pattern path; + + public RequestMatcher(String method, String path) { + this.method = method; + this.path = Pattern.compile(path); + } + + @Override + public boolean test(ContainerRequestContext requestContext) { + return requestContext.getMethod().equals(method) + && path.matcher(requestContext.getUriInfo().getPath()).matches(); + } + } + public JaasBasicAuthFilter(Configuration configuration) { this.configuration = configuration; } @Override public void filter(ContainerRequestContext requestContext) throws IOException { - if (isInternalTaskConfigRequest(requestContext)) { + if (isInternalRequest(requestContext)) { log.trace("Skipping authentication for internal request"); return; } @@ -82,12 +105,10 @@ public void filter(ContainerRequestContext requestContext) throws IOException { } } - private static boolean isInternalTaskConfigRequest(ContainerRequestContext requestContext) { - return requestContext.getMethod().equals(HttpMethod.POST) - && TASK_REQUEST_PATTERN.matcher(requestContext.getUriInfo().getPath()).matches(); + private boolean isInternalRequest(ContainerRequestContext requestContext) { + return INTERNAL_REQUEST_MATCHERS.stream().anyMatch(m -> m.test(requestContext)); } - public static class BasicAuthCallBackHandler implements CallbackHandler { private static final String BASIC = "basic"; diff --git a/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java b/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java index 561095f68218a..2513c308a7e01 100644 --- a/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java +++ b/connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java @@ -42,8 +42,10 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; public class JaasBasicAuthFilterTest { @@ -58,7 +60,7 @@ public void testSuccess() throws IOException { ContainerRequestContext requestContext = setMock("Basic", "user", "password"); jaasBasicAuthFilter.filter(requestContext); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @@ -69,7 +71,7 @@ public void testEmptyCredentialsFile() throws IOException { ContainerRequestContext requestContext = setMock("Basic", "user", "password"); jaasBasicAuthFilter.filter(requestContext); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @@ -81,7 +83,7 @@ public void testBadCredential() throws IOException { jaasBasicAuthFilter.filter(requestContext); verify(requestContext).abortWith(any(Response.class)); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @@ -93,7 +95,7 @@ public void testBadPassword() throws IOException { jaasBasicAuthFilter.filter(requestContext); verify(requestContext).abortWith(any(Response.class)); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @@ -105,7 +107,7 @@ public void testUnknownBearer() throws IOException { jaasBasicAuthFilter.filter(requestContext); verify(requestContext).abortWith(any(Response.class)); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @@ -117,7 +119,7 @@ public void testUnknownLoginModule() throws IOException { jaasBasicAuthFilter.filter(requestContext); verify(requestContext).abortWith(any(Response.class)); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @@ -128,7 +130,7 @@ public void testUnknownCredentialsFile() throws IOException { jaasBasicAuthFilter.filter(requestContext); verify(requestContext).abortWith(any(Response.class)); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @@ -139,17 +141,26 @@ public void testNoFileOption() throws IOException { jaasBasicAuthFilter.filter(requestContext); verify(requestContext).abortWith(any(Response.class)); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getHeaderString(JaasBasicAuthFilter.AUTHORIZATION); } @Test - public void testPostWithoutAppropriateCredential() throws IOException { + public void testInternalTaskConfigEndpointSkipped() throws IOException { + testInternalEndpointSkipped(HttpMethod.POST, "connectors/connName/tasks"); + } + + @Test + public void testInternalZombieFencingEndpointSkipped() throws IOException { + testInternalEndpointSkipped(HttpMethod.PUT, "connectors/connName/fence"); + } + + private void testInternalEndpointSkipped(String method, String endpoint) throws IOException { UriInfo uriInfo = mock(UriInfo.class); - when(uriInfo.getPath()).thenReturn("connectors/connName/tasks"); + when(uriInfo.getPath()).thenReturn(endpoint); ContainerRequestContext requestContext = mock(ContainerRequestContext.class); - when(requestContext.getMethod()).thenReturn(HttpMethod.POST); + when(requestContext.getMethod()).thenReturn(method); when(requestContext.getUriInfo()).thenReturn(uriInfo); File credentialFile = setupPropertyLoginFile(true); @@ -158,8 +169,9 @@ public void testPostWithoutAppropriateCredential() throws IOException { jaasBasicAuthFilter.filter(requestContext); verify(uriInfo).getPath(); - verify(requestContext).getMethod(); + verify(requestContext, atLeastOnce()).getMethod(); verify(requestContext).getUriInfo(); + verifyNoMoreInteractions(requestContext); } @Test diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Herder.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Herder.java index 0f20c0bb3557f..32ab697735417 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Herder.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Herder.java @@ -138,6 +138,18 @@ public interface Herder { */ void putTaskConfigs(String connName, List> configs, Callback callback, InternalRequestSignature requestSignature); + /** + * Fence out any older task generations for a source connector, and then write a record to the config topic + * indicating that it is safe to bring up a new generation of tasks. If that record is already present, do nothing + * and invoke the callback successfully. + * @param connName the name of the connector to fence out, which must refer to a source connector; if the + * connector does not exist or is not a source connector, the callback will be invoked with an error + * @param callback callback to invoke upon completion + * @param requestSignature the signature of the request made for this connector; + * may be null if no signature was provided + */ + void fenceZombieSourceTasks(String connName, Callback callback, InternalRequestSignature requestSignature); + /** * Get a list of connectors currently running in this cluster. * @return A list of connector names diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java index d309ac93d2bc0..34258f82fb1c4 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java @@ -16,11 +16,14 @@ */ package org.apache.kafka.connect.runtime; +import org.apache.kafka.clients.admin.Admin; import org.apache.kafka.clients.admin.AdminClientConfig; +import org.apache.kafka.clients.admin.FenceProducersOptions; import org.apache.kafka.clients.consumer.ConsumerConfig; import org.apache.kafka.clients.consumer.KafkaConsumer; import org.apache.kafka.clients.producer.KafkaProducer; import org.apache.kafka.clients.producer.ProducerConfig; +import org.apache.kafka.common.KafkaFuture; import org.apache.kafka.common.MetricNameTemplate; import org.apache.kafka.common.config.ConfigValue; import org.apache.kafka.common.config.provider.ConfigProvider; @@ -35,6 +38,8 @@ import org.apache.kafka.connect.json.JsonConverter; import org.apache.kafka.connect.json.JsonConverterConfig; import org.apache.kafka.connect.runtime.ConnectMetrics.MetricGroup; +import org.apache.kafka.connect.runtime.isolation.LoaderSwap; +import org.apache.kafka.connect.runtime.rest.resources.ConnectorsResource; import org.apache.kafka.connect.storage.ClusterConfigState; import org.apache.kafka.connect.runtime.errors.DeadLetterQueueReporter; import org.apache.kafka.connect.runtime.errors.ErrorHandlingMetrics; @@ -77,7 +82,9 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.IntStream; /** *

@@ -582,6 +589,60 @@ public boolean startTask( } } + /** + * Using the admin principal for this connector, perform a round of zombie fencing that disables transactional producers + * for the specified number of source tasks from sending any more records. + * @param connName the name of the connector + * @param numTasks the number of tasks to fence out + * @param connProps the configuration of the connector; may not be null + * @return a {@link KafkaFuture} that will complete when the producers have all been fenced out, or the attempt has failed + */ + public KafkaFuture fenceZombies(String connName, int numTasks, Map connProps) { + return fenceZombies(connName, numTasks, connProps, Admin::create); + } + + // Allows us to mock out the Admin client for testing + KafkaFuture fenceZombies(String connName, int numTasks, Map connProps, Function, Admin> adminFactory) { + log.debug("Fencing out {} task producers for source connector {}", numTasks, connName); + try (LoggingContext loggingContext = LoggingContext.forConnector(connName)) { + String connType = connProps.get(ConnectorConfig.CONNECTOR_CLASS_CONFIG); + ClassLoader connectorLoader = plugins.delegatingLoader().connectorLoader(connType); + try (LoaderSwap loaderSwap = plugins.withClassLoader(connectorLoader)) { + final SourceConnectorConfig connConfig = new SourceConnectorConfig(plugins, connProps, config.topicCreationEnable()); + final Class connClass = plugins.connectorClass( + connConfig.getString(ConnectorConfig.CONNECTOR_CLASS_CONFIG)); + + Map adminConfig = adminConfigs( + connName, + "connector-worker-adminclient-" + connName, + config, + connConfig, + connClass, + connectorClientConfigOverridePolicy, + kafkaClusterId, + ConnectorType.SOURCE); + final Admin admin = adminFactory.apply(adminConfig); + + try { + Collection transactionalIds = IntStream.range(0, numTasks) + .mapToObj(i -> new ConnectorTaskId(connName, i)) + .map(this::taskTransactionalId) + .collect(Collectors.toList()); + FenceProducersOptions fencingOptions = new FenceProducersOptions() + .timeoutMs((int) ConnectorsResource.REQUEST_TIMEOUT_MS); + return admin.fenceProducers(transactionalIds, fencingOptions).all().whenComplete((ignored, error) -> { + if (error != null) + log.debug("Finished fencing out {} task producers for source connector {}", numTasks, connName); + Utils.closeQuietly(admin, "Zombie fencing admin for connector " + connName); + }); + } catch (Exception e) { + Utils.closeQuietly(admin, "Zombie fencing admin for connector " + connName); + throw e; + } + } + } + } + private WorkerTask buildWorkerTask(ClusterConfigState configState, ConnectorConfig connConfig, ConnectorTaskId id, @@ -610,14 +671,14 @@ private WorkerTask buildWorkerTask(ClusterConfigState configState, internalKeyConverter, internalValueConverter); OffsetStorageWriter offsetWriter = new OffsetStorageWriter(offsetBackingStore, id.connector(), internalKeyConverter, internalValueConverter); - Map producerProps = producerConfigs(id, "connector-producer-" + id, config, sourceConfig, connectorClass, + Map producerProps = producerConfigs(id.connector(), "connector-producer-" + id, config, sourceConfig, connectorClass, connectorClientConfigOverridePolicy, kafkaClusterId); KafkaProducer producer = new KafkaProducer<>(producerProps); TopicAdmin admin; Map topicCreationGroups; if (config.topicCreationEnable() && sourceConfig.usesTopicCreation()) { - Map adminProps = adminConfigs(id, "connector-adminclient-" + id, config, - sourceConfig, connectorClass, connectorClientConfigOverridePolicy, kafkaClusterId); + Map adminProps = adminConfigs(id.connector(), "connector-adminclient-" + id, config, + sourceConfig, connectorClass, connectorClientConfigOverridePolicy, kafkaClusterId, ConnectorType.SOURCE); admin = new TopicAdmin(adminProps); topicCreationGroups = TopicCreationGroup.configuredGroups(sourceConfig); } else { @@ -649,7 +710,7 @@ private WorkerTask buildWorkerTask(ClusterConfigState configState, } } - static Map producerConfigs(ConnectorTaskId id, + static Map producerConfigs(String connName, String defaultClientId, WorkerConfig config, ConnectorConfig connConfig, @@ -657,7 +718,7 @@ static Map producerConfigs(ConnectorTaskId id, ConnectorClientConfigOverridePolicy connectorClientConfigOverridePolicy, String clusterId) { Map producerProps = new HashMap<>(); - producerProps.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, Utils.join(config.getList(WorkerConfig.BOOTSTRAP_SERVERS_CONFIG), ",")); + producerProps.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, config.bootstrapServers()); producerProps.put(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArraySerializer"); producerProps.put(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.ByteArraySerializer"); // These settings will execute infinite retries on retriable exceptions. They *may* be overridden via configs passed to the worker, @@ -680,7 +741,7 @@ static Map producerConfigs(ConnectorTaskId id, // Connector-specified overrides Map producerOverrides = - connectorClientConfigOverrides(id, connConfig, connectorClass, ConnectorConfig.CONNECTOR_CLIENT_PRODUCER_OVERRIDES_PREFIX, + connectorClientConfigOverrides(connName, connConfig, connectorClass, ConnectorConfig.CONNECTOR_CLIENT_PRODUCER_OVERRIDES_PREFIX, ConnectorType.SOURCE, ConnectorClientConfigRequest.ClientType.PRODUCER, connectorClientConfigOverridePolicy); producerProps.putAll(producerOverrides); @@ -712,7 +773,7 @@ static Map consumerConfigs(ConnectorTaskId id, ConnectUtils.addMetricsContextProperties(consumerProps, config, clusterId); // Connector-specified overrides Map consumerOverrides = - connectorClientConfigOverrides(id, connConfig, connectorClass, ConnectorConfig.CONNECTOR_CLIENT_CONSUMER_OVERRIDES_PREFIX, + connectorClientConfigOverrides(id.connector(), connConfig, connectorClass, ConnectorConfig.CONNECTOR_CLIENT_CONSUMER_OVERRIDES_PREFIX, ConnectorType.SINK, ConnectorClientConfigRequest.ClientType.CONSUMER, connectorClientConfigOverridePolicy); consumerProps.putAll(consumerOverrides); @@ -720,13 +781,14 @@ static Map consumerConfigs(ConnectorTaskId id, return consumerProps; } - static Map adminConfigs(ConnectorTaskId id, + static Map adminConfigs(String connName, String defaultClientId, WorkerConfig config, ConnectorConfig connConfig, Class connectorClass, ConnectorClientConfigOverridePolicy connectorClientConfigOverridePolicy, - String clusterId) { + String clusterId, + ConnectorType connectorType) { Map adminProps = new HashMap<>(); // Use the top-level worker configs to retain backwards compatibility with older releases which // did not require a prefix for connector admin client configs in the worker configuration file @@ -734,12 +796,11 @@ static Map adminConfigs(ConnectorTaskId id, // and those that begin with "producer." and "consumer.", since we know they aren't intended for // the admin client Map nonPrefixedWorkerConfigs = config.originals().entrySet().stream() - .filter(e -> !e.getKey().startsWith("admin.") - && !e.getKey().startsWith("producer.") - && !e.getKey().startsWith("consumer.")) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); - adminProps.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, - Utils.join(config.getList(WorkerConfig.BOOTSTRAP_SERVERS_CONFIG), ",")); + .filter(e -> !e.getKey().startsWith("admin.") + && !e.getKey().startsWith("producer.") + && !e.getKey().startsWith("consumer.")) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + adminProps.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, config.bootstrapServers()); adminProps.put(AdminClientConfig.CLIENT_ID_CONFIG, defaultClientId); adminProps.putAll(nonPrefixedWorkerConfigs); @@ -748,9 +809,9 @@ static Map adminConfigs(ConnectorTaskId id, // Connector-specified overrides Map adminOverrides = - connectorClientConfigOverrides(id, connConfig, connectorClass, ConnectorConfig.CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX, - ConnectorType.SINK, ConnectorClientConfigRequest.ClientType.ADMIN, - connectorClientConfigOverridePolicy); + connectorClientConfigOverrides(connName, connConfig, connectorClass, ConnectorConfig.CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX, + connectorType, ConnectorClientConfigRequest.ClientType.ADMIN, + connectorClientConfigOverridePolicy); adminProps.putAll(adminOverrides); //add client metrics.context properties @@ -759,7 +820,7 @@ static Map adminConfigs(ConnectorTaskId id, return adminProps; } - private static Map connectorClientConfigOverrides(ConnectorTaskId id, + private static Map connectorClientConfigOverrides(String connName, ConnectorConfig connConfig, Class connectorClass, String clientConfigPrefix, @@ -768,7 +829,7 @@ private static Map connectorClientConfigOverrides(ConnectorTaskI ConnectorClientConfigOverridePolicy connectorClientConfigOverridePolicy) { Map clientOverrides = connConfig.originalsWithPrefix(clientConfigPrefix); ConnectorClientConfigRequest connectorClientConfigRequest = new ConnectorClientConfigRequest( - id.connector(), + connName, connectorType, connectorClass, clientOverrides, @@ -784,6 +845,14 @@ private static Map connectorClientConfigOverrides(ConnectorTaskI return clientOverrides; } + private String taskTransactionalId(ConnectorTaskId id) { + return taskTransactionalId(config.groupId(), id.connector(), id.task()); + } + + public static String taskTransactionalId(String groupId, String connector, int taskId) { + return String.format("%s-%s-%d", groupId, connector, taskId); + } + ErrorHandlingMetrics errorHandlingMetrics(ConnectorTaskId id) { return new ErrorHandlingMetrics(id, metrics); } @@ -798,9 +867,9 @@ private List sinkTaskReporters(ConnectorTaskId id, SinkConnectorC // check if topic for dead letter queue exists String topic = connConfig.dlqTopicName(); if (topic != null && !topic.isEmpty()) { - Map producerProps = producerConfigs(id, "connector-dlq-producer-" + id, config, connConfig, connectorClass, - connectorClientConfigOverridePolicy, kafkaClusterId); - Map adminProps = adminConfigs(id, "connector-dlq-adminclient-", config, connConfig, connectorClass, connectorClientConfigOverridePolicy, kafkaClusterId); + Map producerProps = producerConfigs(id.connector(), "connector-dlq-producer-" + id, config, connConfig, connectorClass, + connectorClientConfigOverridePolicy, kafkaClusterId); + Map adminProps = adminConfigs(id.connector(), "connector-dlq-adminclient-", config, connConfig, connectorClass, connectorClientConfigOverridePolicy, kafkaClusterId, ConnectorType.SINK); DeadLetterQueueReporter reporter = DeadLetterQueueReporter.createAndSetup(adminProps, id, connConfig, producerProps, errorHandlingMetrics); reporters.add(reporter); diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java index b189d2e642176..8d269a3649eb9 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java @@ -17,6 +17,7 @@ package org.apache.kafka.connect.runtime.distributed; import org.apache.kafka.clients.CommonClientConfigs; +import org.apache.kafka.common.KafkaFuture; import org.apache.kafka.common.config.ConfigDef; import org.apache.kafka.common.config.ConfigValue; import org.apache.kafka.common.errors.WakeupException; @@ -54,6 +55,7 @@ import org.apache.kafka.connect.runtime.rest.RestClient; import org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo; import org.apache.kafka.connect.runtime.rest.entities.ConnectorStateInfo; +import org.apache.kafka.connect.runtime.rest.entities.ConnectorType; import org.apache.kafka.connect.runtime.rest.entities.TaskInfo; import org.apache.kafka.connect.runtime.rest.errors.BadRequestException; import org.apache.kafka.connect.runtime.rest.errors.ConnectRestException; @@ -66,7 +68,9 @@ import org.apache.kafka.connect.storage.ConfigBackingStore; import org.apache.kafka.connect.storage.StatusBackingStore; import org.apache.kafka.connect.util.Callback; +import org.apache.kafka.connect.util.ConnectUtils; import org.apache.kafka.connect.util.ConnectorTaskId; +import org.apache.kafka.connect.util.FutureCallback; import org.apache.kafka.connect.util.SinkUtils; import org.slf4j.Logger; @@ -89,6 +93,7 @@ import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentSkipListSet; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingDeque; @@ -162,8 +167,9 @@ public class DistributedHerder extends AbstractHerder implements Runnable { private final List keySignatureVerificationAlgorithms; private final KeyGenerator keyGenerator; + // Visible for testing + ExecutorService forwardRequestExecutor; private final ExecutorService herderExecutor; - private final ExecutorService forwardRequestExecutor; private final ExecutorService startAndStopExecutor; private final WorkerGroupMember member; private final AtomicBoolean stopping; @@ -190,6 +196,8 @@ public class DistributedHerder extends AbstractHerder implements Runnable { // Similarly collect target state changes (when observed by the config storage listener) for handling in the // herder's main thread. private Set connectorTargetStateChanges = new HashSet<>(); + // Access to this map is protected by the herder's monitor + private final Map activeZombieFencings = new HashMap<>(); private boolean needsReconfigRebalance; private volatile boolean fencedFromConfigTopic; private volatile int generation; @@ -203,6 +211,10 @@ public class DistributedHerder extends AbstractHerder implements Runnable { // The latest pending restart request for each named connector final Map pendingRestartRequests = new HashMap<>(); + // The thread that the herder's tick loop runs on. Would be final, but cannot be set in the constructor, + // and it's also useful to be able to modify it for testing + Thread herderThread; + private final DistributedConfig config; /** @@ -324,6 +336,7 @@ public void start() { public void run() { try { log.info("Herder starting"); + herderThread = Thread.currentThread(); startServices(); @@ -430,12 +443,7 @@ public void tick() { break; } - try { - next.action().call(); - next.callback().onCompletion(null, null); - } catch (Throwable t) { - next.callback().onCompletion(t, null); - } + runRequest(next.action(), next.callback()); } // Process all pending connector restart requests @@ -708,11 +716,25 @@ private void processTaskConfigUpdatesWithIncrementalCooperative(Set connectorsWhoseTasksToStop = taskConfigUpdates.stream() .map(ConnectorTaskId::connector).collect(Collectors.toSet()); + stopReconfiguredTasks(connectorsWhoseTasksToStop); + } + + private void stopReconfiguredTasks(Set connectors) { + Set localTasks = assignment == null + ? Collections.emptySet() + : new HashSet<>(assignment.tasks()); List tasksToStop = localTasks.stream() - .filter(taskId -> connectorsWhoseTasksToStop.contains(taskId.connector())) + .filter(taskId -> connectors.contains(taskId.connector())) .collect(Collectors.toList()); - log.info("Handling task config update by restarting tasks {}", tasksToStop); + + if (tasksToStop.isEmpty()) { + // The rest of the method would essentially be a no-op so this isn't strictly necessary, + // but it prevents an unnecessary log message from being emitted + return; + } + + log.info("Handling task config update by stopping tasks {}, which will be restarted after rebalance if still assigned to this worker", tasksToStop); worker.stopAndAwaitTasks(tasksToStop); tasksToRestart.addAll(tasksToStop); } @@ -1097,31 +1119,8 @@ public void taskConfigs(final String connName, final Callback> ca @Override public void putTaskConfigs(final String connName, final List> configs, final Callback callback, InternalRequestSignature requestSignature) { log.trace("Submitting put task configuration request {}", connName); - if (internalRequestValidationEnabled()) { - ConnectRestException requestValidationError = null; - if (requestSignature == null) { - requestValidationError = new BadRequestException("Internal request missing required signature"); - } else if (!keySignatureVerificationAlgorithms.contains(requestSignature.keyAlgorithm())) { - requestValidationError = new BadRequestException(String.format( - "This worker does not support the '%s' key signing algorithm used by other workers. " - + "This worker is currently configured to use: %s. " - + "Check that all workers' configuration files permit the same set of signature algorithms, " - + "and correct any misconfigured worker and restart it.", - requestSignature.keyAlgorithm(), - keySignatureVerificationAlgorithms - )); - } else { - if (!requestSignature.isValid(sessionKey)) { - requestValidationError = new ConnectRestException( - Response.Status.FORBIDDEN, - "Internal request contained invalid signature." - ); - } - } - if (requestValidationError != null) { - callback.onCompletion(requestValidationError, null); - return; - } + if (requestNotSignedProperly(requestSignature, callback)) { + return; } addRequest( @@ -1140,6 +1139,113 @@ else if (!configState.contains(connName)) ); } + // Another worker has forwarded a request to this worker (which it believes is the leader) to perform a round of zombie fencing + @Override + public void fenceZombieSourceTasks(final String connName, final Callback callback, InternalRequestSignature requestSignature) { + log.trace("Submitting zombie fencing request {}", connName); + if (requestNotSignedProperly(requestSignature, callback)) { + return; + } + + fenceZombieSourceTasks(connName, callback); + } + + // A task on this worker requires a round of zombie fencing + void fenceZombieSourceTasks(final ConnectorTaskId id, Callback callback) { + log.trace("Performing preflight zombie check for task {}", id); + fenceZombieSourceTasks(id.connector(), (error, ignored) -> { + if (error == null) { + callback.onCompletion(null, null); + } else if (error instanceof NotLeaderException) { + String forwardedUrl = ((NotLeaderException) error).forwardUrl() + "connectors/" + id.connector() + "/fence"; + log.trace("Forwarding zombie fencing request for connector {} to leader at {}", id.connector(), forwardedUrl); + forwardRequestExecutor.execute(() -> { + try { + RestClient.httpRequest(forwardedUrl, "PUT", null, null, null, config, sessionKey, requestSignatureAlgorithm); + callback.onCompletion(null, null); + } catch (Throwable t) { + callback.onCompletion(t, null); + } + }); + } else { + error = ConnectUtils.maybeWrap(error, "Failed to perform zombie fencing"); + callback.onCompletion(error, null); + } + }); + } + + // Visible for testing + void fenceZombieSourceTasks(final String connName, final Callback callback) { + addRequest( + () -> { + log.trace("Performing zombie fencing request for connector {}", connName); + if (!isLeader()) + callback.onCompletion(new NotLeaderException("Only the leader may perform zombie fencing.", leaderUrl()), null); + else if (!configState.contains(connName)) + callback.onCompletion(new NotFoundException("Connector " + connName + " not found"), null); + else if (!isSourceConnector(connName)) + callback.onCompletion(new BadRequestException("Connector " + connName + " is not a source connector"), null); + else { + if (!refreshConfigSnapshot(workerSyncTimeoutMs)) { + throw new ConnectException("Failed to read to end of config topic before performing zombie fencing"); + } + + int taskCount = configState.taskCount(connName); + Integer taskCountRecord = configState.taskCountRecord(connName); + + ZombieFencing zombieFencing = null; + boolean newFencing = false; + synchronized (DistributedHerder.this) { + // Check first to see if we have to do a fencing. The control flow is a little awkward here (why not stick this in + // an else block lower down?) but we can't synchronize around the body below since that may contain a synchronous + // write to the config topic. + if (configState.pendingFencing(connName) && taskCountRecord != null + && (taskCountRecord != 1 || taskCount != 1)) { + int taskGen = configState.taskConfigGeneration(connName); + zombieFencing = activeZombieFencings.get(connName); + if (zombieFencing == null) { + zombieFencing = new ZombieFencing(connName, taskCountRecord, taskCount, taskGen); + activeZombieFencings.put(connName, zombieFencing); + newFencing = true; + } + } + } + if (zombieFencing != null) { + if (newFencing) { + zombieFencing.start(); + } + zombieFencing.addCallback(callback); + return null; + } + + if (!configState.pendingFencing(connName)) { + // If the latest task count record for the connector is present after the latest set of task configs, there's no need to + // do any zombie fencing or write a new task count record to the config topic + log.debug("Skipping zombie fencing round for connector {} as all old task generations have already been fenced out", connName); + } else { + if (taskCountRecord == null) { + // If there is no task count record present for the connector, no transactional producers should have been brought up for it, + // so there's nothing to fence--but we do need to write a task count record now so that we know to fence those tasks if/when + // the connector is reconfigured + log.debug("Skipping zombie fencing round but writing task count record for connector {} " + + "as it is being brought up for the first time with exactly-once source support", connName); + } else { + // If the last generation of tasks only had one task, and the next generation only has one, then the new task will automatically + // fence out the older task if it's still running; no need to fence here, but again, we still need to write a task count record + log.debug("Skipping zombie fencing round but writing task count record for connector {} " + + "as both the most recent and the current generation of task configs only contain one task", connName); + } + writeToConfigTopicAsLeader(() -> configBackingStore.putTaskCountRecord(connName, taskCount)); + } + callback.onCompletion(null, null); + return null; + } + return null; + }, + forwardErrorCallback(callback) + ); + } + @Override public void restartConnector(final String connName, final Callback callback) { restartConnector(0, connName, callback); @@ -1468,7 +1574,8 @@ private boolean handleRebalanceCompleted() { } /** - * Try to read to the end of the config log within the given timeout + * Try to read to the end of the config log within the given timeout. If unsuccessful, leave the group + * and wait for a brief backoff period before returning * @param timeoutMs maximum time to wait to sync to the end of the log * @return true if successful, false if timed out */ @@ -1478,18 +1585,32 @@ private boolean readConfigToEnd(long timeoutMs) { } else { log.info("Reading to end of config log; current config state offset: {}", configState.offset()); } + if (refreshConfigSnapshot(timeoutMs)) { + backoffRetries = BACKOFF_RETRIES; + return true; + } else { + // in case reading the log takes too long, leave the group to ensure a quick rebalance (although by default we should be out of the group already) + // and back off to avoid a tight loop of rejoin-attempt-to-catch-up-leave + member.maybeLeaveGroup("taking too long to read the log"); + backoff(workerUnsyncBackoffMs); + return false; + } + } + + /** + * Try to read to the end of the config log within the given timeout + * @param timeoutMs maximum time to wait to sync to the end of the log + * @return true if successful; false if timed out + */ + private boolean refreshConfigSnapshot(long timeoutMs) { try { configBackingStore.refresh(timeoutMs, TimeUnit.MILLISECONDS); configState = configBackingStore.snapshot(); log.info("Finished reading to end of log and updated config snapshot, new config log offset: {}", configState.offset()); - backoffRetries = BACKOFF_RETRIES; return true; } catch (TimeoutException e) { - // in case reading the log takes too long, leave the group to ensure a quick rebalance (although by default we should be out of the group already) - // and back off to avoid a tight loop of rejoin-attempt-to-catch-up-leave log.warn("Didn't reach end of config log quickly enough", e); - member.maybeLeaveGroup("taking too long to read the log"); - backoff(workerUnsyncBackoffMs); + canReadConfigs = false; return false; } } @@ -1778,6 +1899,48 @@ private void reconfigureConnector(final String connName, final Callback cb } } + // Currently unused, but will be invoked by exactly-once source tasks after they have successfully + // initialized their transactional producer + private void verifyTaskGenerationAndOwnership(ConnectorTaskId id, int initialTaskGen) { + log.debug("Reading to end of config topic to ensure it is still safe to bring up source task {} with exactly-once support", id); + if (!refreshConfigSnapshot(Long.MAX_VALUE)) { + throw new ConnectException("Failed to read to end of config topic"); + } + + FutureCallback verifyCallback = new FutureCallback<>(); + + addRequest( + () -> verifyTaskGenerationAndOwnership(id, initialTaskGen, verifyCallback), + forwardErrorCallback(verifyCallback) + ); + + try { + verifyCallback.get(); + } catch (InterruptedException e) { + throw new ConnectException("Interrupted while performing preflight check for task " + id, e); + } catch (ExecutionException e) { + Throwable cause = e.getCause(); + throw ConnectUtils.maybeWrap(cause, "Failed to perform preflight check for task " + id); + } + } + + // Visible for testing + Void verifyTaskGenerationAndOwnership(ConnectorTaskId id, int initialTaskGen, Callback callback) { + Integer currentTaskGen = configState.taskConfigGeneration(id.connector()); + if (!Objects.equals(initialTaskGen, currentTaskGen)) { + throw new ConnectException("Cannot start source task " + + id + " with exactly-once support as the connector has already generated a new set of task configs"); + } + + if (!assignment.tasks().contains(id)) { + throw new ConnectException("Cannot start source task " + + id + " as it has already been revoked from this worker"); + } + + callback.onCompletion(null, null); + return null; + } + private boolean checkRebalanceNeeded(Callback callback) { // Raise an error if we are expecting a rebalance to begin. This prevents us from forwarding requests // based on stale leadership or assignment information @@ -1788,6 +1951,23 @@ private boolean checkRebalanceNeeded(Callback callback) { return false; } + /** + * Execute the given action and subsequent callback immediately if the current thread is the herder's tick thread, + * or use them to create and store a {@link DistributedHerderRequest} on the request queue and return the resulting request + * if not. + * @param action the action that should be run on the herder's tick thread + * @param callback the callback that should be invoked once the action is complete + * @return a new {@link DistributedHerderRequest} if one has been created and added to the request queue, and {@code null} otherwise + */ + DistributedHerderRequest runOnTickThread(Callable action, Callback callback) { + if (Thread.currentThread().equals(herderThread)) { + runRequest(action, callback); + return null; + } else { + return addRequest(action, callback); + } + } + DistributedHerderRequest addRequest(Callable action, Callback callback) { return addRequest(0, action, callback); } @@ -1800,6 +1980,15 @@ DistributedHerderRequest addRequest(long delayMs, Callable action, Callbac return req; } + private void runRequest(Callable action, Callback callback) { + try { + action.call(); + callback.onCompletion(null, null); + } catch (Throwable t) { + callback.onCompletion(t, null); + } + } + private boolean internalRequestValidationEnabled() { return internalRequestValidationEnabled(member.currentProtocolVersion()); } @@ -1852,7 +2041,7 @@ public void onTaskConfigUpdate(Collection tasks) { log.info("Tasks {} configs updated", tasks); // Stage the update and wake up the work thread. - // The set of tasks is recorder for incremental cooperative rebalancing, in which + // The set of tasks is recorded for incremental cooperative rebalancing, in which // tasks don't get restarted unless they are balanced between workers. // With eager rebalancing there's no need to record the set of tasks because task reconfigs // always need a rebalance to ensure offsets get committed. In eager rebalancing the @@ -1863,6 +2052,20 @@ public void onTaskConfigUpdate(Collection tasks) { needsReconfigRebalance = true; taskConfigUpdates.addAll(tasks); } + tasks.stream() + .map(ConnectorTaskId::connector) + .distinct() + .forEach(connName -> { + synchronized (this) { + ZombieFencing activeFencing = activeZombieFencings.get(connName); + if (activeFencing != null) { + activeFencing.completeExceptionally(new ConnectRestException( + Response.Status.CONFLICT.getStatusCode(), + "Failed to complete zombie fencing because a new set of task configs was generated" + )); + } + } + }); member.wakeup(); } @@ -2133,6 +2336,166 @@ private void resetActiveTopics(Collection connectors, Collection callback) { + if (internalRequestValidationEnabled()) { + ConnectRestException requestValidationError = null; + if (requestSignature == null) { + requestValidationError = new BadRequestException("Internal request missing required signature"); + } else if (!keySignatureVerificationAlgorithms.contains(requestSignature.keyAlgorithm())) { + requestValidationError = new BadRequestException(String.format( + "This worker does not support the '%s' key signing algorithm used by other workers. " + + "This worker is currently configured to use: %s. " + + "Check that all workers' configuration files permit the same set of signature algorithms, " + + "and correct any misconfigured worker and restart it.", + requestSignature.keyAlgorithm(), + keySignatureVerificationAlgorithms + )); + } else { + if (!requestSignature.isValid(sessionKey)) { + requestValidationError = new ConnectRestException( + Response.Status.FORBIDDEN, + "Internal request contained invalid signature." + ); + } + } + if (requestValidationError != null) { + callback.onCompletion(requestValidationError, null); + return true; + } + } + + return false; + } + + /** + * Represents an active zombie fencing: that is, an in-progress attempt to invoke + * {@link Worker#fenceZombies(String, int, Map)} and then, if successful, write a new task count + * record to the config topic. + */ + class ZombieFencing { + private final String connName; + private final int tasksToFence; + private final int tasksToRecord; + private final int taskGen; + private final FutureCallback fencingFollowup; + private KafkaFuture fencingFuture; + + public ZombieFencing(String connName, int tasksToFence, int tasksToRecord, int taskGen) { + this.connName = connName; + this.tasksToFence = tasksToFence; + this.tasksToRecord = tasksToRecord; + this.taskGen = taskGen; + this.fencingFollowup = new FutureCallback<>(); + } + + /** + * Start sending requests to the Kafka cluster to fence zombies. In rare cases, may cause blocking calls to + * take place before returning, so care should be taken to ensure that this method is not invoked while holding + * any important locks (e.g., while synchronized on the surrounding DistributedHerder instance). + * This method must be invoked before any {@link #addCallback(Callback) callbacks can be added}, + * and may only be invoked once. + * @throws IllegalStateException if invoked multiple times + */ + public void start() { + if (fencingFuture != null) { + throw new IllegalStateException("Cannot invoke start() multiple times"); + } + fencingFuture = worker.fenceZombies(connName, tasksToFence, configState.connectorConfig(connName)).thenApply(ignored -> { + // This callback will be called on the same thread that invokes KafkaFuture::thenApply if + // the future is already completed. Since that thread is the herder tick thread, we don't need + // to perform follow-up logic through an additional herder request (and if we tried, it would lead + // to deadlock) + runOnTickThread( + this::onZombieFencingSuccess, + fencingFollowup + ); + awaitFollowup(); + return null; + }); + // Immediately after the fencing and necessary followup work (i.e., writing the task count record to the config topic) + // is complete, remove this from the list of active fencings + addCallback((ignored, error) -> { + synchronized (DistributedHerder.this) { + activeZombieFencings.remove(connName); + } + }); + + } + + // Invoked after the worker has successfully fenced out the producers of old task generations using an admin client + // Note that work here will be performed on the herder's tick thread, so it should not block for very long + private Void onZombieFencingSuccess() { + if (!refreshConfigSnapshot(workerSyncTimeoutMs)) { + throw new ConnectException("Failed to read to end of config topic"); + } + if (taskGen < configState.taskConfigGeneration(connName)) { + throw new ConnectRestException( + Response.Status.CONFLICT.getStatusCode(), + "Fencing failed because new task configurations were generated for the connector"); + } + // If we've already been cancelled, skip the write to the config topic + if (fencingFollowup.isDone()) { + return null; + } + writeToConfigTopicAsLeader(() -> configBackingStore.putTaskCountRecord(connName, tasksToRecord)); + return null; + } + + private void awaitFollowup() { + try { + fencingFollowup.get(); + } catch (InterruptedException e) { + throw new ConnectException("Interrupted while performing zombie fencing", e); + } catch (ExecutionException e) { + Throwable cause = e.getCause(); + throw ConnectUtils.maybeWrap(cause, "Failed to perform round of zombie fencing"); + } + } + + /** + * Fail the fencing if it is still active, reporting the given exception as the cause of failure + * @param t the cause of failure to report for the failed fencing; may not be null + */ + public void completeExceptionally(Throwable t) { + Objects.requireNonNull(t); + fencingFollowup.onCompletion(t, null); + } + + /** + * Add a callback to invoke after the fencing has succeeded and a record of it has been written to the config topic + * Note that this fencing must be {@link #start() started} before this method is invoked + * @param callback the callback to report the success or failure of the fencing to + * @throws IllegalStateException if this method is invoked before {@link #start()} + */ + public void addCallback(Callback callback) { + if (fencingFuture == null) { + throw new IllegalStateException("The start() method must be invoked before adding callbacks for this zombie fencing"); + } + fencingFuture.whenComplete((ignored, error) -> { + if (error != null) { + callback.onCompletion( + ConnectUtils.maybeWrap(error, "Failed to perform zombie fencing"), + null + ); + } else { + callback.onCompletion(null, null); + } + }); + } + } + class HerderMetrics { private final MetricGroup metricGroup; private final Sensor rebalanceCompletedCounts; diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/LoaderSwap.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/LoaderSwap.java new file mode 100644 index 0000000000000..47e8c12d54b25 --- /dev/null +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/LoaderSwap.java @@ -0,0 +1,36 @@ +/* + * 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.kafka.connect.runtime.isolation; + +/** + * Helper for having {@code Plugins} use a given classloader within a try-with-resources statement. + * See {@link Plugins#withClassLoader(ClassLoader)}. + */ +public class LoaderSwap implements AutoCloseable { + + private final ClassLoader savedLoader; + + public LoaderSwap(ClassLoader savedLoader) { + this.savedLoader = savedLoader; + } + + @Override + public void close() { + Plugins.compareAndSwapLoaders(savedLoader); + } + +} diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java index 7ec73ba78b83d..6d961272399e8 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java @@ -145,6 +145,16 @@ public ClassLoader compareAndSwapLoaders(Connector connector) { return compareAndSwapLoaders(connectorLoader); } + public LoaderSwap withClassLoader(ClassLoader loader) { + ClassLoader savedLoader = compareAndSwapLoaders(loader); + try { + return new LoaderSwap(savedLoader); + } catch (Throwable t) { + compareAndSwapLoaders(savedLoader); + throw t; + } + } + public DelegatingClassLoader delegatingLoader() { return delegatingLoader; } diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java index 03325526cd0cb..9cc580b9b5398 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestClient.java @@ -112,14 +112,15 @@ public static HttpResponse httpRequest(String url, String method, HttpHea if (serializedBody != null) { req.content(new StringContentProvider(serializedBody, StandardCharsets.UTF_8), "application/json"); - if (sessionKey != null && requestSignatureAlgorithm != null) { - InternalRequestSignature.addToRequest( - sessionKey, - serializedBody.getBytes(StandardCharsets.UTF_8), - requestSignatureAlgorithm, - req - ); - } + } + + if (sessionKey != null && requestSignatureAlgorithm != null) { + InternalRequestSignature.addToRequest( + sessionKey, + serializedBody != null ? serializedBody.getBytes(StandardCharsets.UTF_8) : null, + requestSignatureAlgorithm, + req + ); } ContentResponse res = req.send(); diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java index dbf246f00ef1c..ce8ae9baa2e27 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResource.java @@ -320,6 +320,17 @@ public void putTaskConfigs(final @PathParam("connector") String connector, completeOrForwardRequest(cb, "/connectors/" + connector + "/tasks", "POST", headers, taskConfigs, forward); } + @PUT + @Path("/{connector}/fence") + public void fenceZombies(final @PathParam("connector") String connector, + final @Context HttpHeaders headers, + final @QueryParam("forward") Boolean forward, + final byte[] requestBody) throws Throwable { + FutureCallback cb = new FutureCallback<>(); + herder.fenceZombieSourceTasks(connector, cb, InternalRequestSignature.fromHeaders(requestBody, headers)); + completeOrForwardRequest(cb, "/connectors/" + connector + "/fence", "PUT", headers, requestBody, forward); + } + @GET @Path("/{connector}/tasks/{task}/status") public ConnectorStateInfo.TaskState getTaskStatus(final @PathParam("connector") String connector, diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java index eb3c3393cebe0..f9e812610c3ad 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerder.java @@ -265,6 +265,11 @@ public void putTaskConfigs(String connName, List> configs, C throw new UnsupportedOperationException("Kafka Connect in standalone mode does not support externally setting task configurations."); } + @Override + public void fenceZombieSourceTasks(String connName, Callback callback, InternalRequestSignature requestSignature) { + throw new UnsupportedOperationException("Kafka Connect in standalone mode does not support exactly-once source connectors."); + } + @Override public synchronized void restartTask(ConnectorTaskId taskId, Callback cb) { if (!configState.contains(taskId.connector())) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ClusterConfigState.java b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ClusterConfigState.java index 7a766905b81d6..99000f7a8f48f 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ClusterConfigState.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ClusterConfigState.java @@ -42,6 +42,9 @@ public class ClusterConfigState { Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap(), + Collections.emptyMap(), + Collections.emptyMap(), + Collections.emptySet(), Collections.emptySet()); private final long offset; @@ -51,6 +54,9 @@ public class ClusterConfigState { final Map> connectorConfigs; final Map connectorTargetStates; final Map> taskConfigs; + final Map connectorTaskCountRecords; + final Map connectorTaskConfigGenerations; + final Set connectorsPendingFencing; final Set inconsistentConnectors; public ClusterConfigState(long offset, @@ -59,6 +65,9 @@ public ClusterConfigState(long offset, Map> connectorConfigs, Map connectorTargetStates, Map> taskConfigs, + Map connectorTaskCountRecords, + Map connectorTaskConfigGenerations, + Set connectorsPendingFencing, Set inconsistentConnectors) { this(offset, sessionKey, @@ -66,6 +75,9 @@ public ClusterConfigState(long offset, connectorConfigs, connectorTargetStates, taskConfigs, + connectorTaskCountRecords, + connectorTaskConfigGenerations, + connectorsPendingFencing, inconsistentConnectors, null); } @@ -76,6 +88,9 @@ public ClusterConfigState(long offset, Map> connectorConfigs, Map connectorTargetStates, Map> taskConfigs, + Map connectorTaskCountRecords, + Map connectorTaskConfigGenerations, + Set connectorsPendingFencing, Set inconsistentConnectors, WorkerConfigTransformer configTransformer) { this.offset = offset; @@ -84,6 +99,9 @@ public ClusterConfigState(long offset, this.connectorConfigs = connectorConfigs; this.connectorTargetStates = connectorTargetStates; this.taskConfigs = taskConfigs; + this.connectorTaskCountRecords = connectorTaskCountRecords; + this.connectorTaskConfigGenerations = connectorTaskConfigGenerations; + this.connectorsPendingFencing = connectorsPendingFencing; this.inconsistentConnectors = inconsistentConnectors; this.configTransformer = configTransformer; } @@ -202,6 +220,15 @@ public int taskCount(String connectorName) { return count == null ? 0 : count; } + /** + * Get whether the connector requires a round of zombie fencing before + * a new generation of tasks can be brought up for it. + * @param connectorName name of the connector + */ + public boolean pendingFencing(String connectorName) { + return connectorsPendingFencing.contains(connectorName); + } + /** * Get the current set of task IDs for the specified connector. * @param connectorName the name of the connector to look up task configs for @@ -225,6 +252,25 @@ public List tasks(String connectorName) { return Collections.unmodifiableList(taskIds); } + /** + * Get the task count record for the connector, if one exists + * @param connector name of the connector + * @return the latest task count record for the connector, or {@code null} if none exists + */ + public Integer taskCountRecord(String connector) { + return connectorTaskCountRecords.get(connector); + } + + /** + * Get the generation number for the connector's task configurations, if one exists. + * Generation numbers increase monotonically each time a new set of task configurations is detected for the connector + * @param connector name of the connector + * @return the latest task config generation number for the connector, or {@code null} if none exists + */ + public Integer taskConfigGeneration(String connector) { + return connectorTaskConfigGenerations.get(connector); + } + /** * Get the set of connectors which have inconsistent data in this snapshot. These inconsistencies can occur due to * partially completed writes combined with log compaction. diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ConfigBackingStore.java b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ConfigBackingStore.java index 24539076adc0f..490cfdafa3f35 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ConfigBackingStore.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/ConfigBackingStore.java @@ -101,6 +101,13 @@ public interface ConfigBackingStore { */ void putRestartRequest(RestartRequest restartRequest); + /** + * Record the number of tasks for the connector after a successful round of zombie fencing. + * @param connector name of the connector + * @param taskCount number of tasks used by the connector + */ + void putTaskCountRecord(String connector, int taskCount); + /** * Prepare to write to the backing config store. May be required by some implementations (such as those that only permit a single * writer at a time across a cluster of workers) before performing mutating operations like writing configurations, target states, etc. diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java index d17864458bf69..f8467e85b5164 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java @@ -191,6 +191,12 @@ public static String COMMIT_TASKS_KEY(String connectorName) { return COMMIT_TASKS_PREFIX + connectorName; } + public static final String TASK_COUNT_RECORD_PREFIX = "tasks-fencing-"; + + public static String TASK_COUNT_RECORD_KEY(String connectorName) { + return TASK_COUNT_RECORD_PREFIX + connectorName; + } + public static final String SESSION_KEY_KEY = "session-key"; // Note that while using real serialization for values as we have here, but ad hoc string serialization for keys, @@ -207,6 +213,9 @@ public static String COMMIT_TASKS_KEY(String connectorName) { public static final Schema TARGET_STATE_V0 = SchemaBuilder.struct() .field("state", Schema.STRING_SCHEMA) .build(); + public static final Schema TASK_COUNT_RECORD_V0 = SchemaBuilder.struct() + .field("task-count", Schema.INT32_SCHEMA) + .build(); // The key is logically a byte array, but we can't use the JSON converter to (de-)serialize that without a schema. // So instead, we base 64-encode it before serializing and decode it after deserializing. public static final Schema SESSION_KEY_V0 = SchemaBuilder.struct() @@ -237,7 +246,7 @@ public static String RESTART_KEY(String connectorName) { private volatile boolean started; // Although updateListener is not final, it's guaranteed to be visible to any thread after its // initialization as long as we always read the volatile variable "started" before we access the listener. - private UpdateListener updateListener; + private ConfigBackingStore.UpdateListener updateListener; private final Map baseProducerProps; @@ -267,6 +276,10 @@ public static String RESTART_KEY(String connectorName) { final Map connectorTargetStates = new HashMap<>(); + final Map connectorTaskCountRecords = new HashMap<>(); + final Map connectorTaskConfigGenerations = new HashMap<>(); + final Set connectorsPendingFencing = new HashSet<>(); + private final WorkerConfigTransformer configTransformer; private final boolean usesFencableWriter; @@ -305,7 +318,7 @@ public KafkaConfigBackingStore(Converter converter, DistributedConfig config, Wo } @Override - public void setUpdateListener(UpdateListener listener) { + public void setUpdateListener(ConfigBackingStore.UpdateListener listener) { this.updateListener = listener; } @@ -319,8 +332,8 @@ public void start() { } catch (UnsupportedVersionException e) { throw new ConnectException( "Enabling exactly-once support for source connectors requires a Kafka broker version that allows " - + "admin clients to read consumer offsets. Please either disable the worker's exactly-once " + - "support for source connectors, or use a newer Kafka broker version.", + + "admin clients to read consumer offsets. Please either disable the worker's exactly-once " + + "support for source connectors, or use a newer Kafka broker version.", e ); } @@ -417,6 +430,9 @@ public ClusterConfigState snapshot() { new HashMap<>(connectorConfigs), new HashMap<>(connectorTargetStates), new HashMap<>(taskConfigs), + new HashMap<>(connectorTaskCountRecords), + new HashMap<>(connectorTaskConfigGenerations), + new HashSet<>(connectorsPendingFencing), new HashSet<>(inconsistent), configTransformer ); @@ -449,7 +465,7 @@ public void putConnectorConfig(String connector, Map properties) connectConfig.put("properties", properties); byte[] serializedConfig = converter.fromConnectData(topic, CONNECTOR_CONFIGURATION_V0, connectConfig); try { - maybeSendFencably(CONNECTOR_KEY(connector), serializedConfig); + sendPrivileged(CONNECTOR_KEY(connector), serializedConfig); configLog.readToEnd().get(READ_TO_END_TIMEOUT_MS, TimeUnit.MILLISECONDS); } catch (InterruptedException | ExecutionException | TimeoutException e) { log.error("Failed to write connector configuration to Kafka: ", e); @@ -470,8 +486,8 @@ public void putConnectorConfig(String connector, Map properties) public void removeConnectorConfig(String connector) { log.debug("Removing connector configuration for connector '{}'", connector); try { - maybeSendFencably(CONNECTOR_KEY(connector), null); - maybeSendFencably(TARGET_STATE_KEY(connector), null); + sendPrivileged(CONNECTOR_KEY(connector), null); + sendPrivileged(TARGET_STATE_KEY(connector), null); configLog.readToEnd().get(READ_TO_END_TIMEOUT_MS, TimeUnit.MILLISECONDS); } catch (InterruptedException | ExecutionException | TimeoutException e) { log.error("Failed to remove connector configuration from Kafka: ", e); @@ -520,7 +536,7 @@ public void putTaskConfigs(String connector, List> configs) byte[] serializedConfig = converter.fromConnectData(topic, TASK_CONFIGURATION_V0, connectConfig); log.debug("Writing configuration for connector '{}' task {}", connector, index); ConnectorTaskId connectorTaskId = new ConnectorTaskId(connector, index); - maybeSendFencably(TASK_KEY(connectorTaskId), serializedConfig); + sendPrivileged(TASK_KEY(connectorTaskId), serializedConfig); index++; } @@ -536,7 +552,7 @@ public void putTaskConfigs(String connector, List> configs) connectConfig.put("tasks", taskCount); byte[] serializedConfig = converter.fromConnectData(topic, CONNECTOR_TASKS_COMMIT_V0, connectConfig); log.debug("Writing commit for connector '{}' with {} tasks.", connector, taskCount); - maybeSendFencably(COMMIT_TASKS_KEY(connector), serializedConfig); + sendPrivileged(COMMIT_TASKS_KEY(connector), serializedConfig); // Read to end to ensure all the commit messages have been written configLog.readToEnd().get(READ_TO_END_TIMEOUT_MS, TimeUnit.MILLISECONDS); @@ -570,6 +586,32 @@ public void putTargetState(String connector, TargetState state) { configLog.send(TARGET_STATE_KEY(connector), serializedTargetState); } + /** + * Write a task count record for a connector to persistent storage and wait until it has been acknowledged and read back by + * tailing the Kafka log with a consumer. {@link #claimWritePrivileges()} must be successfully invoked before calling this method + * if the worker is configured to use a fencable producer for writes to the config topic. + * @param connector name of the connector + * @param taskCount number of tasks used by the connector + * @throws IllegalStateException if {@link #claimWritePrivileges()} is required, but was not successfully invoked before + * this method was called + * @throws PrivilegedWriteException if the worker is configured to use a fencable producer for writes to the config topic + * and the write fails + */ + @Override + public void putTaskCountRecord(String connector, int taskCount) { + Struct taskCountRecord = new Struct(TASK_COUNT_RECORD_V0); + taskCountRecord.put("task-count", taskCount); + byte[] serializedTaskCountRecord = converter.fromConnectData(topic, TASK_COUNT_RECORD_V0, taskCountRecord); + log.debug("Writing task count record {} for connector {}", taskCount, connector); + try { + sendPrivileged(TASK_COUNT_RECORD_KEY(connector), serializedTaskCountRecord); + configLog.readToEnd().get(READ_TO_END_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException | ExecutionException | TimeoutException e) { + log.error("Failed to write task count record with {} tasks for connector {} to Kafka: ", taskCount, connector, e); + throw new ConnectException("Error writing task count record to Kafka", e); + } + } + /** * Write a session key to persistent storage and wait until it has been acknowledged and read back by tailing the Kafka log * with a consumer. {@link #claimWritePrivileges()} must be successfully invoked before calling this method if the worker @@ -589,7 +631,7 @@ public void putSessionKey(SessionKey sessionKey) { sessionKeyStruct.put("creation-timestamp", sessionKey.creationTimestamp()); byte[] serializedSessionKey = converter.fromConnectData(topic, SESSION_KEY_V0, sessionKeyStruct); try { - maybeSendFencably(SESSION_KEY_KEY, serializedSessionKey); + sendPrivileged(SESSION_KEY_KEY, serializedSessionKey); configLog.readToEnd().get(READ_TO_END_TIMEOUT_MS, TimeUnit.MILLISECONDS); } catch (InterruptedException | ExecutionException | TimeoutException e) { log.error("Failed to write session key to Kafka: ", e); @@ -612,7 +654,7 @@ public void putRestartRequest(RestartRequest restartRequest) { value.put(ONLY_FAILED_FIELD_NAME, restartRequest.onlyFailed()); byte[] serializedValue = converter.fromConnectData(topic, value.schema(), value); try { - maybeSendFencably(key, serializedValue); + sendPrivileged(key, serializedValue); configLog.readToEnd().get(READ_TO_END_TIMEOUT_MS, TimeUnit.MILLISECONDS); } catch (InterruptedException | ExecutionException | TimeoutException e) { log.error("Failed to write {} to Kafka: ", restartRequest, e); @@ -662,7 +704,7 @@ KafkaBasedLog setupAndCreateKafkaBasedLog(String topic, final Wo return createKafkaBasedLog(topic, producerProps, consumerProps, new ConsumeCallback(), topicDescription, adminSupplier); } - private void maybeSendFencably(String key, byte[] value) { + private void sendPrivileged(String key, byte[] value) { if (!usesFencableWriter) { configLog.send(key, value); return; @@ -708,7 +750,6 @@ private KafkaBasedLog createKafkaBasedLog(String topic, Map(topic, producerProps, consumerProps, adminSupplier, consumedCallback, Time.SYSTEM, createTopics); } - @SuppressWarnings("unchecked") private class ConsumeCallback implements Callback> { @Override public void onCompletion(Throwable error, ConsumerRecord record) { @@ -730,226 +771,221 @@ public void onCompletion(Throwable error, ConsumerRecord record) if (record.key().startsWith(TARGET_STATE_PREFIX)) { String connectorName = record.key().substring(TARGET_STATE_PREFIX.length()); - boolean removed = false; - synchronized (lock) { - if (value.value() == null) { - // When connector configs are removed, we also write tombstones for the target state. - log.debug("Removed target state for connector {} due to null value in topic.", connectorName); - connectorTargetStates.remove(connectorName); - removed = true; - - // If for some reason we still have configs for the connector, add back the default - // STARTED state to ensure each connector always has a valid target state. - if (connectorConfigs.containsKey(connectorName)) - connectorTargetStates.put(connectorName, TargetState.STARTED); - } else { - if (!(value.value() instanceof Map)) { - log.error("Found target state ({}) in wrong format: {}", record.key(), value.value().getClass()); - return; - } - Object targetState = ((Map) value.value()).get("state"); - if (!(targetState instanceof String)) { - log.error("Invalid data for target state for connector '{}': 'state' field should be a String but is {}", - connectorName, targetState == null ? null : targetState.getClass()); - return; - } - - try { - TargetState state = TargetState.valueOf((String) targetState); - log.debug("Setting target state for connector '{}' to {}", connectorName, targetState); - connectorTargetStates.put(connectorName, state); - } catch (IllegalArgumentException e) { - log.error("Invalid target state for connector '{}': {}", connectorName, targetState); - return; - } - } - } - - // Note that we do not notify the update listener if the target state has been removed. - // Instead we depend on the removal callback of the connector config itself to notify the worker. - if (started && !removed) - updateListener.onConnectorTargetStateChange(connectorName); - + processTargetStateRecord(connectorName, value); } else if (record.key().startsWith(CONNECTOR_PREFIX)) { String connectorName = record.key().substring(CONNECTOR_PREFIX.length()); - boolean removed = false; - synchronized (lock) { - if (value.value() == null) { - // Connector deletion will be written as a null value - log.info("Successfully processed removal of connector '{}'", connectorName); - connectorConfigs.remove(connectorName); - connectorTaskCounts.remove(connectorName); - taskConfigs.keySet().removeIf(taskId -> taskId.connector().equals(connectorName)); - removed = true; - } else { - // Connector configs can be applied and callbacks invoked immediately - if (!(value.value() instanceof Map)) { - log.error("Found configuration for connector '{}' in wrong format: {}", record.key(), value.value().getClass()); - return; - } - Object newConnectorConfig = ((Map) value.value()).get("properties"); - if (!(newConnectorConfig instanceof Map)) { - log.error("Invalid data for config for connector '{}': 'properties' field should be a Map but is {}", - connectorName, newConnectorConfig == null ? null : newConnectorConfig.getClass()); - return; - } - log.debug("Updating configuration for connector '{}'", connectorName); - connectorConfigs.put(connectorName, (Map) newConnectorConfig); - - // Set the initial state of the connector to STARTED, which ensures that any connectors - // which were created with 0.9 Connect will be initialized in the STARTED state. - if (!connectorTargetStates.containsKey(connectorName)) - connectorTargetStates.put(connectorName, TargetState.STARTED); - } - } - if (started) { - if (removed) - updateListener.onConnectorConfigRemove(connectorName); - else - updateListener.onConnectorConfigUpdate(connectorName); - } + processConnectorConfigRecord(connectorName, value); } else if (record.key().startsWith(TASK_PREFIX)) { - synchronized (lock) { - ConnectorTaskId taskId = parseTaskId(record.key()); - if (taskId == null) { - log.error("Ignoring task configuration because {} couldn't be parsed as a task config key", record.key()); - return; - } - if (value.value() == null) { - log.error("Ignoring task configuration for task {} because it is unexpectedly null", taskId); - return; - } - if (!(value.value() instanceof Map)) { - log.error("Ignoring task configuration for task {} because the value is not a Map but is {}", taskId, value.value().getClass()); - return; - } - - Object newTaskConfig = ((Map) value.value()).get("properties"); - if (!(newTaskConfig instanceof Map)) { - log.error("Invalid data for config of task {} 'properties' field should be a Map but is {}", taskId, newTaskConfig.getClass()); - return; - } - - Map> deferred = deferredTaskUpdates.computeIfAbsent(taskId.connector(), k -> new HashMap<>()); - log.debug("Storing new config for task {}; this will wait for a commit message before the new config will take effect.", taskId); - deferred.put(taskId, (Map) newTaskConfig); + ConnectorTaskId taskId = parseTaskId(record.key()); + if (taskId == null) { + log.error("Ignoring task configuration because {} couldn't be parsed as a task config key", record.key()); + return; } + processTaskConfigRecord(taskId, value); } else if (record.key().startsWith(COMMIT_TASKS_PREFIX)) { String connectorName = record.key().substring(COMMIT_TASKS_PREFIX.length()); - List updatedTasks = new ArrayList<>(); - synchronized (lock) { - // Apply any outstanding deferred task updates for the given connector. Note that just because we - // encounter a commit message does not mean it will result in consistent output. In particular due to - // compaction, there may be cases where . For example if we have the following sequence of writes: - // - // 1. Write connector "foo"'s config - // 2. Write connector "foo", task 1's config <-- compacted - // 3. Write connector "foo", task 2's config - // 4. Write connector "foo" task commit message - // 5. Write connector "foo", task 1's config - // 6. Write connector "foo", task 2's config - // 7. Write connector "foo" task commit message - // - // then when a new worker starts up, if message 2 had been compacted, then when message 4 is applied - // "foo" will not have a complete set of configs. Only when message 7 is applied will the complete - // configuration be available. Worse, if the leader died while writing messages 5, 6, and 7 such that - // only 5 was written, then there may be nothing that will finish writing the configs and get the - // log back into a consistent state. - // - // It is expected that the user of this class (i.e., the Herder) will take the necessary action to - // resolve this (i.e., get the connector to recommit its configuration). This inconsistent state is - // exposed in the snapshots provided via ClusterConfigState so they are easy to handle. - if (!(value.value() instanceof Map)) { // Schema-less, so we get maps instead of structs - log.error("Ignoring connector tasks configuration commit for connector '{}' because it is in the wrong format: {}", connectorName, value.value()); - return; - } - Map> deferred = deferredTaskUpdates.get(connectorName); - - int newTaskCount = intValue(((Map) value.value()).get("tasks")); - - // Validate the configs we're supposed to update to ensure we're getting a complete configuration - // update of all tasks that are expected based on the number of tasks in the commit message. - Set taskIdSet = taskIds(connectorName, deferred); - if (!completeTaskIdSet(taskIdSet, newTaskCount)) { - // Given the logic for writing commit messages, we should only hit this condition due to compacted - // historical data, in which case we would not have applied any updates yet and there will be no - // task config data already committed for the connector, so we shouldn't have to clear any data - // out. All we need to do is add the flag marking it inconsistent. - log.debug("We have an incomplete set of task configs for connector '{}' probably due to compaction. So we are not doing anything with the new configuration.", connectorName); - inconsistent.add(connectorName); - } else { - if (deferred != null) { - taskConfigs.putAll(deferred); - updatedTasks.addAll(deferred.keySet()); - } - inconsistent.remove(connectorName); - } - // Always clear the deferred entries, even if we didn't apply them. If they represented an inconsistent - // update, then we need to see a completely fresh set of configs after this commit message, so we don't - // want any of these outdated configs - if (deferred != null) - deferred.clear(); - - connectorTaskCounts.put(connectorName, newTaskCount); - } - - if (started) - updateListener.onTaskConfigUpdate(updatedTasks); + processTasksCommitRecord(connectorName, value); } else if (record.key().startsWith(RESTART_PREFIX)) { RestartRequest request = recordToRestartRequest(record, value); // Only notify the listener if this backing store is already successfully started (having caught up the first time) if (request != null && started) { updateListener.onRestartRequest(request); } + } else if (record.key().startsWith(TASK_COUNT_RECORD_PREFIX)) { + String connectorName = record.key().substring(TASK_COUNT_RECORD_PREFIX.length()); + processTaskCountRecord(connectorName, value); } else if (record.key().equals(SESSION_KEY_KEY)) { - if (value.value() == null) { - log.error("Ignoring session key because it is unexpectedly null"); + processSessionKeyRecord(value); + } else { + log.error("Discarding config update record with invalid key: {}", record.key()); + } + } + + } + + private void processTargetStateRecord(String connectorName, SchemaAndValue value) { + boolean removed = false; + synchronized (lock) { + if (value.value() == null) { + // When connector configs are removed, we also write tombstones for the target state. + log.debug("Removed target state for connector {} due to null value in topic.", connectorName); + connectorTargetStates.remove(connectorName); + removed = true; + + // If for some reason we still have configs for the connector, add back the default + // STARTED state to ensure each connector always has a valid target state. + if (connectorConfigs.containsKey(connectorName)) + connectorTargetStates.put(connectorName, TargetState.STARTED); + } else { + if (!(value.value() instanceof Map)) { + log.error("Ignoring target state for connector '{}' because it is in the wrong format: {}", connectorName, className(value.value())); return; } - if (!(value.value() instanceof Map)) { - log.error("Ignoring session key because the value is not a Map but is {}", value.value().getClass()); + @SuppressWarnings("unchecked") + Object targetState = ((Map) value.value()).get("state"); + if (!(targetState instanceof String)) { + log.error("Invalid data for target state for connector '{}': 'state' field should be a String but is {}", + connectorName, className(targetState)); return; } - Map valueAsMap = (Map) value.value(); - - Object sessionKey = valueAsMap.get("key"); - if (!(sessionKey instanceof String)) { - log.error("Invalid data for session key 'key' field should be a String but is {}", sessionKey.getClass()); + try { + TargetState state = TargetState.valueOf((String) targetState); + log.debug("Setting target state for connector '{}' to {}", connectorName, targetState); + connectorTargetStates.put(connectorName, state); + } catch (IllegalArgumentException e) { + log.error("Invalid target state for connector '{}': {}", connectorName, targetState); return; } - byte[] key = Base64.getDecoder().decode((String) sessionKey); + } + } + + // Note that we do not notify the update listener if the target state has been removed. + // Instead we depend on the removal callback of the connector config itself to notify the worker. + if (started && !removed) + updateListener.onConnectorTargetStateChange(connectorName); + } - Object keyAlgorithm = valueAsMap.get("algorithm"); - if (!(keyAlgorithm instanceof String)) { - log.error("Invalid data for session key 'algorithm' field should be a String but it is {}", keyAlgorithm.getClass()); + private void processConnectorConfigRecord(String connectorName, SchemaAndValue value) { + boolean removed = false; + synchronized (lock) { + if (value.value() == null) { + // Connector deletion will be written as a null value + log.info("Successfully processed removal of connector '{}'", connectorName); + connectorConfigs.remove(connectorName); + connectorTaskCounts.remove(connectorName); + taskConfigs.keySet().removeIf(taskId -> taskId.connector().equals(connectorName)); + removed = true; + } else { + // Connector configs can be applied and callbacks invoked immediately + if (!(value.value() instanceof Map)) { + log.error("Ignoring configuration for connector '{}' because it is in the wrong format: {}", connectorName, className(value.value())); return; } - - Object creationTimestamp = valueAsMap.get("creation-timestamp"); - if (!(creationTimestamp instanceof Long)) { - log.error("Invalid data for session key 'creation-timestamp' field should be a long but it is {}", creationTimestamp.getClass()); + @SuppressWarnings("unchecked") + Object newConnectorConfig = ((Map) value.value()).get("properties"); + if (!(newConnectorConfig instanceof Map)) { + log.error("Invalid data for config for connector '{}': 'properties' field should be a Map but is {}", + connectorName, className(newConnectorConfig)); return; } - KafkaConfigBackingStore.this.sessionKey = new SessionKey( - new SecretKeySpec(key, (String) keyAlgorithm), - (long) creationTimestamp - ); + log.debug("Updating configuration for connector '{}'", connectorName); + @SuppressWarnings("unchecked") + Map stringsConnectorConfig = (Map) newConnectorConfig; + connectorConfigs.put(connectorName, stringsConnectorConfig); + + // Set the initial state of the connector to STARTED, which ensures that any connectors + // which were created with 0.9 Connect will be initialized in the STARTED state. + if (!connectorTargetStates.containsKey(connectorName)) + connectorTargetStates.put(connectorName, TargetState.STARTED); + } + } + if (started) { + if (removed) + updateListener.onConnectorConfigRemove(connectorName); + else + updateListener.onConnectorConfigUpdate(connectorName); + } + } + + private void processTaskConfigRecord(ConnectorTaskId taskId, SchemaAndValue value) { + synchronized (lock) { + if (value.value() == null) { + log.error("Ignoring task configuration for task {} because it is unexpectedly null", taskId); + return; + } + if (!(value.value() instanceof Map)) { + log.error("Ignoring task configuration for task {} because the value is not a Map but is {}", taskId, className(value.value())); + return; + } + + @SuppressWarnings("unchecked") + Object newTaskConfig = ((Map) value.value()).get("properties"); + if (!(newTaskConfig instanceof Map)) { + log.error("Invalid data for config of task {} 'properties' field should be a Map but is {}", taskId, className(newTaskConfig)); + return; + } - if (started) - updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey); + Map> deferred = deferredTaskUpdates.computeIfAbsent(taskId.connector(), k -> new HashMap<>()); + log.debug("Storing new config for task {}; this will wait for a commit message before the new config will take effect.", taskId); + @SuppressWarnings("unchecked") + Map stringsTaskConfig = (Map) newTaskConfig; + deferred.put(taskId, stringsTaskConfig); + } + } + + private void processTasksCommitRecord(String connectorName, SchemaAndValue value) { + List updatedTasks = new ArrayList<>(); + synchronized (lock) { + // Apply any outstanding deferred task updates for the given connector. Note that just because we + // encounter a commit message does not mean it will result in consistent output. In particular due to + // compaction, there may be cases where . For example if we have the following sequence of writes: + // + // 1. Write connector "foo"'s config + // 2. Write connector "foo", task 1's config <-- compacted + // 3. Write connector "foo", task 2's config + // 4. Write connector "foo" task commit message + // 5. Write connector "foo", task 1's config + // 6. Write connector "foo", task 2's config + // 7. Write connector "foo" task commit message + // + // then when a new worker starts up, if message 2 had been compacted, then when message 4 is applied + // "foo" will not have a complete set of configs. Only when message 7 is applied will the complete + // configuration be available. Worse, if the leader died while writing messages 5, 6, and 7 such that + // only 5 was written, then there may be nothing that will finish writing the configs and get the + // log back into a consistent state. + // + // It is expected that the user of this class (i.e., the Herder) will take the necessary action to + // resolve this (i.e., get the connector to recommit its configuration). This inconsistent state is + // exposed in the snapshots provided via ClusterConfigState so they are easy to handle. + if (!(value.value() instanceof Map)) { // Schema-less, so we get maps instead of structs + log.error("Ignoring connector tasks configuration commit for connector '{}' because it is in the wrong format: {}", connectorName, className(value.value())); + return; + } + Map> deferred = deferredTaskUpdates.get(connectorName); + + @SuppressWarnings("unchecked") + int newTaskCount = intValue(((Map) value.value()).get("tasks")); + + // Validate the configs we're supposed to update to ensure we're getting a complete configuration + // update of all tasks that are expected based on the number of tasks in the commit message. + Set taskIdSet = taskIds(connectorName, deferred); + if (!completeTaskIdSet(taskIdSet, newTaskCount)) { + // Given the logic for writing commit messages, we should only hit this condition due to compacted + // historical data, in which case we would not have applied any updates yet and there will be no + // task config data already committed for the connector, so we shouldn't have to clear any data + // out. All we need to do is add the flag marking it inconsistent. + log.debug("We have an incomplete set of task configs for connector '{}' probably due to compaction. So we are not doing anything with the new configuration.", connectorName); + inconsistent.add(connectorName); } else { - log.error("Discarding config update record with invalid key: {}", record.key()); + if (deferred != null) { + taskConfigs.putAll(deferred); + updatedTasks.addAll(deferred.keySet()); + connectorTaskConfigGenerations.compute(connectorName, (ignored, generation) -> generation != null ? generation + 1 : 0); + } + inconsistent.remove(connectorName); } + // Always clear the deferred entries, even if we didn't apply them. If they represented an inconsistent + // update, then we need to see a completely fresh set of configs after this commit message, so we don't + // want any of these outdated configs + if (deferred != null) + deferred.clear(); + + connectorTaskCounts.put(connectorName, newTaskCount); } + // If task configs appear after the latest task count record, the connector needs a new round of zombie fencing + // before it can start tasks with these configs + connectorsPendingFencing.add(connectorName); + if (started) + updateListener.onTaskConfigUpdate(updatedTasks); } @SuppressWarnings("unchecked") RestartRequest recordToRestartRequest(ConsumerRecord record, SchemaAndValue value) { String connectorName = record.key().substring(RESTART_PREFIX.length()); if (!(value.value() instanceof Map)) { - log.error("Ignoring restart request because the value is not a Map but is {}", value.value() == null ? "null" : value.value().getClass()); + log.error("Ignoring restart request because the value is not a Map but is {}", className(value.value())); return null; } @@ -958,7 +994,7 @@ RestartRequest recordToRestartRequest(ConsumerRecord record, Sch Object failed = valueAsMap.get(ONLY_FAILED_FIELD_NAME); boolean onlyFailed; if (!(failed instanceof Boolean)) { - log.warn("Invalid data for restart request '{}' field should be a Boolean but is {}, defaulting to {}", ONLY_FAILED_FIELD_NAME, failed == null ? "null" : failed.getClass(), ONLY_FAILED_DEFAULT); + log.warn("Invalid data for restart request '{}' field should be a Boolean but is {}, defaulting to {}", ONLY_FAILED_FIELD_NAME, className(failed), ONLY_FAILED_DEFAULT); onlyFailed = ONLY_FAILED_DEFAULT; } else { onlyFailed = (Boolean) failed; @@ -967,7 +1003,7 @@ RestartRequest recordToRestartRequest(ConsumerRecord record, Sch Object withTasks = valueAsMap.get(INCLUDE_TASKS_FIELD_NAME); boolean includeTasks; if (!(withTasks instanceof Boolean)) { - log.warn("Invalid data for restart request '{}' field should be a Boolean but is {}, defaulting to {}", INCLUDE_TASKS_FIELD_NAME, withTasks == null ? "null" : withTasks.getClass(), INCLUDE_TASKS_DEFAULT); + log.warn("Invalid data for restart request '{}' field should be a Boolean but is {}, defaulting to {}", INCLUDE_TASKS_FIELD_NAME, className(withTasks), INCLUDE_TASKS_DEFAULT); includeTasks = INCLUDE_TASKS_DEFAULT; } else { includeTasks = (Boolean) withTasks; @@ -975,6 +1011,61 @@ RestartRequest recordToRestartRequest(ConsumerRecord record, Sch return new RestartRequest(connectorName, onlyFailed, includeTasks); } + private void processTaskCountRecord(String connectorName, SchemaAndValue value) { + if (!(value.value() instanceof Map)) { + log.error("Ignoring task count record for connector '{}' because it is in the wrong format: {}", connectorName, className(value.value())); + return; + } + @SuppressWarnings("unchecked") + int taskCount = intValue(((Map) value.value()).get("task-count")); + + log.debug("Setting task count record for connector '{}' to {}", connectorName, taskCount); + connectorTaskCountRecords.put(connectorName, taskCount); + // If a task count record appears after the latest task configs, the connectors doesn't need a round of zombie + // fencing before it can start tasks with the latest configs + connectorsPendingFencing.remove(connectorName); + } + + private void processSessionKeyRecord(SchemaAndValue value) { + if (value.value() == null) { + log.error("Ignoring session key because it is unexpectedly null"); + return; + } + if (!(value.value() instanceof Map)) { + log.error("Ignoring session key because the value is not a Map but is {}", className(value.value())); + return; + } + + @SuppressWarnings("unchecked") + Map valueAsMap = (Map) value.value(); + + Object sessionKey = valueAsMap.get("key"); + if (!(sessionKey instanceof String)) { + log.error("Invalid data for session key 'key' field should be a String but is {}", className(sessionKey)); + return; + } + byte[] key = Base64.getDecoder().decode((String) sessionKey); + + Object keyAlgorithm = valueAsMap.get("algorithm"); + if (!(keyAlgorithm instanceof String)) { + log.error("Invalid data for session key 'algorithm' field should be a String but it is {}", className(keyAlgorithm)); + return; + } + + Object creationTimestamp = valueAsMap.get("creation-timestamp"); + if (!(creationTimestamp instanceof Long)) { + log.error("Invalid data for session key 'creation-timestamp' field should be a long but it is {}", className(creationTimestamp)); + return; + } + KafkaConfigBackingStore.this.sessionKey = new SessionKey( + new SecretKeySpec(key, (String) keyAlgorithm), + (long) creationTimestamp + ); + + if (started) + updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey); + } + private ConnectorTaskId parseTaskId(String key) { String[] parts = key.split("-"); if (parts.length < 3) return null; @@ -1045,5 +1136,9 @@ else if (value instanceof Long) else throw new ConnectException("Expected integer value to be either Integer or Long"); } + + private String className(Object o) { + return o != null ? o.getClass().getName() : "null"; + } } diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/MemoryConfigBackingStore.java b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/MemoryConfigBackingStore.java index b8f19329ee9b3..1e483ec731920 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/MemoryConfigBackingStore.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/MemoryConfigBackingStore.java @@ -74,8 +74,12 @@ public synchronized ClusterConfigState snapshot() { connectorConfigs, connectorTargetStates, taskConfigs, + Collections.emptyMap(), + Collections.emptyMap(), Collections.emptySet(), - configTransformer); + Collections.emptySet(), + configTransformer + ); } @Override @@ -155,6 +159,11 @@ public void putRestartRequest(RestartRequest restartRequest) { // no-op } + @Override + public void putTaskCountRecord(String connector, int taskCount) { + // no-op + } + @Override public synchronized void setUpdateListener(UpdateListener listener) { this.updateListener = listener; diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/util/ConnectUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/util/ConnectUtils.java index bbf24776035ec..0af14cc7f30ec 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/util/ConnectUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/util/ConnectUtils.java @@ -191,4 +191,14 @@ public static C combineCollections( .collect(collector); } + public static ConnectException maybeWrap(Throwable t, String message) { + if (t == null) { + return null; + } + if (t instanceof ConnectException) { + return (ConnectException) t; + } + return new ConnectException(message, t); + } + } diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java index 1f5acb412c52f..ada507f1eb35b 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/AbstractHerderTest.java @@ -130,10 +130,10 @@ public class AbstractHerderTest { } private static final ClusterConfigState SNAPSHOT = new ClusterConfigState(1, null, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet()); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); private static final ClusterConfigState SNAPSHOT_NO_TASKS = new ClusterConfigState(1, null, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - Collections.emptyMap(), Collections.emptySet()); + Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); private final String workerId = "workerId"; private final String kafkaClusterId = "I4ZmrWqfT2e-upky_4fdPA"; diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java index 36cec9205861f..b2d7e0a50074b 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java @@ -18,8 +18,11 @@ import java.util.Collection; import org.apache.kafka.clients.CommonClientConfigs; +import org.apache.kafka.clients.admin.Admin; +import org.apache.kafka.clients.admin.FenceProducersResult; import org.apache.kafka.clients.consumer.ConsumerConfig; import org.apache.kafka.clients.producer.ProducerConfig; +import org.apache.kafka.common.KafkaFuture; import org.apache.kafka.common.MetricName; import org.apache.kafka.common.config.AbstractConfig; import org.apache.kafka.common.config.ConfigException; @@ -33,6 +36,7 @@ import org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy; import org.apache.kafka.connect.connector.policy.NoneConnectorClientConfigOverridePolicy; import org.apache.kafka.connect.errors.ConnectException; +import org.apache.kafka.connect.health.ConnectorType; import org.apache.kafka.connect.json.JsonConverter; import org.apache.kafka.connect.runtime.ConnectMetrics.MetricGroup; import org.apache.kafka.connect.runtime.MockConnectMetrics.MockMetricsReporter; @@ -84,7 +88,11 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; +import static org.apache.kafka.clients.admin.AdminClientConfig.RETRY_BACKOFF_MS_CONFIG; +import static org.apache.kafka.connect.runtime.ConnectorConfig.CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX; import static org.apache.kafka.connect.runtime.TopicCreationConfig.DEFAULT_TOPIC_CREATION_PREFIX; import static org.apache.kafka.connect.runtime.TopicCreationConfig.PARTITIONS_CONFIG; import static org.apache.kafka.connect.runtime.TopicCreationConfig.REPLICATION_FACTOR_CONFIG; @@ -968,7 +976,7 @@ public void testProducerConfigsWithoutOverrides() { expectedConfigs.put("client.id", "connector-producer-job-0"); expectedConfigs.put("metrics.context.connect.kafka.cluster.id", CLUSTER_ID); assertEquals(expectedConfigs, - Worker.producerConfigs(TASK_ID, "connector-producer-" + TASK_ID, config, connectorConfig, null, noneConnectorClientConfigOverridePolicy, CLUSTER_ID)); + Worker.producerConfigs(TASK_ID.connector(), "connector-producer-" + TASK_ID, config, connectorConfig, null, noneConnectorClientConfigOverridePolicy, CLUSTER_ID)); verify(connectorConfig).originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_PRODUCER_OVERRIDES_PREFIX); } @@ -989,7 +997,7 @@ public void testProducerConfigsWithOverrides() { when(connectorConfig.originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_PRODUCER_OVERRIDES_PREFIX)).thenReturn(new HashMap<>()); assertEquals(expectedConfigs, - Worker.producerConfigs(TASK_ID, "connector-producer-" + TASK_ID, configWithOverrides, connectorConfig, null, allConnectorClientConfigOverridePolicy, CLUSTER_ID)); + Worker.producerConfigs(TASK_ID.connector(), "connector-producer-" + TASK_ID, configWithOverrides, connectorConfig, null, allConnectorClientConfigOverridePolicy, CLUSTER_ID)); verify(connectorConfig).originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_PRODUCER_OVERRIDES_PREFIX); } @@ -1015,7 +1023,7 @@ public void testProducerConfigsWithClientOverrides() { when(connectorConfig.originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_PRODUCER_OVERRIDES_PREFIX)).thenReturn(connConfig); assertEquals(expectedConfigs, - Worker.producerConfigs(TASK_ID, "connector-producer-" + TASK_ID, configWithOverrides, connectorConfig, null, allConnectorClientConfigOverridePolicy, CLUSTER_ID)); + Worker.producerConfigs(TASK_ID.connector(), "connector-producer-" + TASK_ID, configWithOverrides, connectorConfig, null, allConnectorClientConfigOverridePolicy, CLUSTER_ID)); verify(connectorConfig).originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_PRODUCER_OVERRIDES_PREFIX); } @@ -1121,8 +1129,8 @@ public void testAdminConfigsClientOverridesWithAllPolicy() { expectedConfigs.put("metrics.context.connect.kafka.cluster.id", CLUSTER_ID); when(connectorConfig.originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX)).thenReturn(connConfig); - assertEquals(expectedConfigs, Worker.adminConfigs(new ConnectorTaskId("test", 1), "", configWithOverrides, connectorConfig, - null, allConnectorClientConfigOverridePolicy, CLUSTER_ID)); + assertEquals(expectedConfigs, Worker.adminConfigs(CONNECTOR_ID, "", configWithOverrides, connectorConfig, + null, allConnectorClientConfigOverridePolicy, CLUSTER_ID, ConnectorType.SOURCE)); verify(connectorConfig).originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX); } @@ -1137,8 +1145,8 @@ public void testAdminConfigsClientOverridesWithNonePolicy() { Map connConfig = Collections.singletonMap("metadata.max.age.ms", "10000"); when(connectorConfig.originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX)).thenReturn(connConfig); - assertThrows(ConnectException.class, () -> Worker.adminConfigs(new ConnectorTaskId("test", 1), - "", configWithOverrides, connectorConfig, null, noneConnectorClientConfigOverridePolicy, CLUSTER_ID)); + assertThrows(ConnectException.class, () -> Worker.adminConfigs( + CONNECTOR_ID, "", configWithOverrides, connectorConfig, null, noneConnectorClientConfigOverridePolicy, CLUSTER_ID, ConnectorType.SOURCE)); verify(connectorConfig).originalsWithPrefix(ConnectorConfig.CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX); } @@ -1226,7 +1234,47 @@ public void testExecutorServiceShutdownWhenTerminationThrowsException() throws I verify(executorService, times(1)).shutdownNow(); verify(executorService, times(1)).awaitTermination(1000L, TimeUnit.MILLISECONDS); verifyNoMoreInteractions(executorService); + } + + @Test + @SuppressWarnings("unchecked") + public void testZombieFencing() { + Admin admin = mock(Admin.class); + FenceProducersResult fenceProducersResult = mock(FenceProducersResult.class); + KafkaFuture fenceProducersFuture = mock(KafkaFuture.class); + KafkaFuture expectedZombieFenceFuture = mock(KafkaFuture.class); + when(admin.fenceProducers(any(), any())).thenReturn(fenceProducersResult); + when(fenceProducersResult.all()).thenReturn(fenceProducersFuture); + when(fenceProducersFuture.whenComplete(any())).thenReturn(expectedZombieFenceFuture); + + when(plugins.delegatingLoader()).thenReturn(delegatingLoader); + when(delegatingLoader.connectorLoader(anyString())).thenReturn(pluginLoader); + pluginsMockedStatic.when(() -> Plugins.compareAndSwapLoaders(pluginLoader)).thenReturn(delegatingLoader); + pluginsMockedStatic.when(() -> Plugins.compareAndSwapLoaders(delegatingLoader)).thenReturn(pluginLoader); + + worker = new Worker(WORKER_ID, new MockTime(), plugins, config, offsetBackingStore, executorService, + allConnectorClientConfigOverridePolicy); + worker.herder = herder; + worker.start(); + + Map connectorConfig = anyConnectorConfigMap(); + connectorConfig.put(CONNECTOR_CLIENT_ADMIN_OVERRIDES_PREFIX + RETRY_BACKOFF_MS_CONFIG, "4761"); + AtomicReference> adminConfig = new AtomicReference<>(); + Function, Admin> mockAdminConstructor = actualAdminConfig -> { + adminConfig.set(actualAdminConfig); + return admin; + }; + + KafkaFuture actualZombieFenceFuture = + worker.fenceZombies(CONNECTOR_ID, 12, connectorConfig, mockAdminConstructor); + + assertEquals(expectedZombieFenceFuture, actualZombieFenceFuture); + assertNotNull(adminConfig.get()); + assertEquals("Admin should be configured with user-specified overrides", + "4761", + adminConfig.get().get(RETRY_BACKOFF_MS_CONFIG) + ); } private void assertStatusMetrics(long expected, String metricName) { @@ -1331,7 +1379,6 @@ private Map anyConnectorConfigMap() { return props; } - private static class TestSourceTask extends SourceTask { public TestSourceTask() { } diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTestUtils.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTestUtils.java index 544507ab64739..084d865cc5eca 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTestUtils.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTestUtils.java @@ -70,6 +70,9 @@ public static ClusterConfigState clusterConfigState(long offset, connectorConfigs(1, connectorNum), connectorTargetStates(1, connectorNum, TargetState.STARTED), taskConfigs(0, connectorNum, connectorNum * taskNum), + Collections.emptyMap(), + Collections.emptyMap(), + Collections.emptySet(), Collections.emptySet()); } diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java index 8f3a8cf3eb418..71cdcc2915546 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java @@ -17,7 +17,10 @@ package org.apache.kafka.connect.runtime.distributed; import org.apache.kafka.clients.CommonClientConfigs; +import org.apache.kafka.common.KafkaException; +import org.apache.kafka.common.KafkaFuture; import org.apache.kafka.common.config.ConfigValue; +import org.apache.kafka.common.errors.AuthorizationException; import org.apache.kafka.common.utils.MockTime; import org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy; import org.apache.kafka.connect.connector.policy.NoneConnectorClientConfigOverridePolicy; @@ -44,6 +47,7 @@ import org.apache.kafka.connect.runtime.isolation.PluginClassLoader; import org.apache.kafka.connect.runtime.isolation.Plugins; import org.apache.kafka.connect.runtime.rest.InternalRequestSignature; +import org.apache.kafka.connect.runtime.rest.RestClient; import org.apache.kafka.connect.runtime.rest.entities.ConfigInfos; import org.apache.kafka.connect.runtime.rest.entities.ConnectorInfo; import org.apache.kafka.connect.runtime.rest.entities.ConnectorStateInfo; @@ -62,7 +66,9 @@ import org.apache.kafka.connect.util.Callback; import org.apache.kafka.connect.util.ConnectorTaskId; import org.apache.kafka.connect.util.FutureCallback; +import org.apache.kafka.connect.util.ThreadedTest; import org.easymock.Capture; +import org.easymock.CaptureType; import org.easymock.EasyMock; import org.junit.After; import org.junit.Before; @@ -85,11 +91,17 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import static java.util.Collections.singletonList; import static javax.ws.rs.core.Response.Status.FORBIDDEN; @@ -100,6 +112,7 @@ import static org.apache.kafka.connect.runtime.distributed.IncrementalCooperativeConnectProtocol.CONNECT_PROTOCOL_V1; import static org.apache.kafka.connect.runtime.distributed.IncrementalCooperativeConnectProtocol.CONNECT_PROTOCOL_V2; import static org.apache.kafka.connect.source.SourceTask.TransactionBoundary.CONNECTOR; +import static org.easymock.EasyMock.anyLong; import static org.easymock.EasyMock.anyObject; import static org.easymock.EasyMock.capture; import static org.easymock.EasyMock.newCapture; @@ -110,9 +123,9 @@ import static org.junit.Assert.fail; @RunWith(PowerMockRunner.class) -@PrepareForTest({DistributedHerder.class, Plugins.class}) +@PrepareForTest({DistributedHerder.class, Plugins.class, RestClient.class}) @PowerMockIgnore({"javax.management.*", "javax.crypto.*"}) -public class DistributedHerderTest { +public class DistributedHerderTest extends ThreadedTest { private static final Map HERDER_CONFIG = new HashMap<>(); static { HERDER_CONFIG.put(DistributedConfig.STATUS_STORAGE_TOPIC_CONFIG, "status-topic"); @@ -177,13 +190,13 @@ public class DistributedHerderTest { } private static final ClusterConfigState SNAPSHOT = new ClusterConfigState(1, null, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet()); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); private static final ClusterConfigState SNAPSHOT_PAUSED_CONN1 = new ClusterConfigState(1, null, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.PAUSED), - TASK_CONFIGS_MAP, Collections.emptySet()); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); private static final ClusterConfigState SNAPSHOT_UPDATED_CONN1_CONFIG = new ClusterConfigState(1, null, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG_UPDATED), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet()); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); private static final String WORKER_ID = "localhost:8083"; private static final String KAFKA_CLUSTER_ID = "I4ZmrWqfT2e-upky_4fdPA"; @@ -206,6 +219,7 @@ public class DistributedHerderTest { private ConfigBackingStore.UpdateListener configUpdateListener; private WorkerRebalanceListener rebalanceListener; + private ExecutorService herderExecutor; private SinkConnectorConfig conn1SinkConfig; private SinkConnectorConfig conn1SinkConfigUpdated; @@ -247,6 +261,10 @@ public void setUp() throws Exception { @After public void tearDown() { if (metrics != null) metrics.stop(); + if (herderExecutor != null) { + herderExecutor.shutdownNow(); + herderExecutor = null; + } } @Test @@ -256,7 +274,7 @@ public void testJoinAssignment() throws Exception { EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); EasyMock.expect(worker.getPlugins()).andReturn(plugins); expectRebalance(1, Arrays.asList(CONN1), Arrays.asList(TASK1)); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); Capture> onStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED), capture(onStart)); @@ -291,7 +309,7 @@ public void testRebalance() throws Exception { EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); EasyMock.expect(worker.getPlugins()).andReturn(plugins); expectRebalance(1, Arrays.asList(CONN1), Arrays.asList(TASK1)); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); Capture> onFirstStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED), capture(onFirstStart)); @@ -352,7 +370,7 @@ public void testIncrementalCooperativeRebalanceForNewMember() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("member"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V1); expectRebalance(1, Collections.emptyList(), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -447,7 +465,7 @@ public void testIncrementalCooperativeRebalanceWithDelay() throws Exception { ConnectProtocol.Assignment.NO_ERROR, 1, Collections.emptyList(), Arrays.asList(TASK2), rebalanceDelay); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); worker.startTask(EasyMock.eq(TASK2), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED)); @@ -508,7 +526,7 @@ public void testRebalanceFailedConnector() throws Exception { EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); EasyMock.expect(worker.getPlugins()).andReturn(plugins); expectRebalance(1, Arrays.asList(CONN1), Arrays.asList(TASK1)); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); Capture> onFirstStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED), capture(onFirstStart)); @@ -579,7 +597,7 @@ public void revokeAndReassign(boolean incompleteRebalance) throws TimeoutExcepti EasyMock.expect(worker.getPlugins()).andReturn(plugins); // The lists need to be mutable because assignments might be removed expectRebalance(configOffset, new ArrayList<>(singletonList(CONN1)), new ArrayList<>(singletonList(TASK1))); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); Capture> onFirstStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED), capture(onFirstStart)); @@ -605,7 +623,7 @@ public void revokeAndReassign(boolean incompleteRebalance) throws TimeoutExcepti configOffset++; expectRebalance(configOffset, Arrays.asList(), Arrays.asList()); // give it the wrong snapshot, as if we're out of sync/can't reach the broker - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.requestRejoin(); PowerMock.expectLastCall(); // tick exits early because we failed, and doesn't do the poll at the end of the method @@ -623,8 +641,8 @@ public void revokeAndReassign(boolean incompleteRebalance) throws TimeoutExcepti ClusterConfigState secondSnapshot = new ClusterConfigState( configOffset, null, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet()); - expectPostRebalanceCatchup(secondSnapshot); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); + expectConfigRefreshAndSnapshot(secondSnapshot); } member.requestRejoin(); PowerMock.expectLastCall(); @@ -689,7 +707,7 @@ public void testCreateConnector() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.wakeup(); PowerMock.expectLastCall(); @@ -744,7 +762,7 @@ public void testCreateConnectorFailedValidation() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); HashMap config = new HashMap<>(CONN2_CONFIG); config.remove(ConnectorConfig.NAME_CONFIG); @@ -1067,7 +1085,7 @@ public void testCreateConnectorAlreadyExists() throws Exception { }); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.wakeup(); PowerMock.expectLastCall(); @@ -1106,7 +1124,7 @@ public void testDestroyConnector() throws Exception { // Start with one connector EasyMock.expect(worker.getPlugins()).andReturn(plugins); expectRebalance(1, Arrays.asList(CONN1), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); Capture> onStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED), capture(onStart)); @@ -1141,7 +1159,7 @@ public void testDestroyConnector() throws Exception { expectRebalance(Arrays.asList(CONN1), Arrays.asList(TASK1), ConnectProtocol.Assignment.NO_ERROR, 2, "leader", "leaderUrl", Collections.emptyList(), Collections.emptyList(), 0, true); - expectPostRebalanceCatchup(ClusterConfigState.EMPTY); + expectConfigRefreshAndSnapshot(ClusterConfigState.EMPTY); member.requestRejoin(); PowerMock.expectLastCall(); PowerMock.replayAll(); @@ -1170,7 +1188,7 @@ public void testRestartConnector() throws Exception { EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); EasyMock.expect(worker.getPlugins()).andReturn(plugins); expectRebalance(1, singletonList(CONN1), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); Capture> onFirstStart = newCapture(); @@ -1223,7 +1241,7 @@ public void testRestartUnknownConnector() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1257,7 +1275,7 @@ public void testRestartConnectorRedirectToLeader() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("member"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1292,7 +1310,7 @@ public void testRestartConnectorRedirectToOwner() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1341,7 +1359,7 @@ public void testRestartConnectorAndTasksUnknownConnector() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1374,7 +1392,7 @@ public void testRestartConnectorAndTasksNotLeader() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("member"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1410,7 +1428,7 @@ public void testRestartConnectorAndTasksUnknownStatus() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1450,7 +1468,7 @@ public void testRestartConnectorAndTasksSuccess() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1613,7 +1631,7 @@ public void testRestartTask() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), singletonList(TASK0), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); worker.startTask(EasyMock.eq(TASK0), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject(), @@ -1651,7 +1669,7 @@ public void testRestartUnknownTask() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("member"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1698,7 +1716,7 @@ public void testRestartTaskRedirectToLeader() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("member"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1733,7 +1751,7 @@ public void testRestartTaskRedirectToOwner() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -1825,7 +1843,7 @@ public void testConnectorConfigUpdate() throws Exception { // join expectRebalance(1, Arrays.asList(CONN1), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); Capture> onFirstStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), @@ -1892,7 +1910,7 @@ public void testConnectorPaused() throws Exception { // join expectRebalance(1, Arrays.asList(CONN1), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); Capture> onStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), @@ -1952,7 +1970,7 @@ public void testConnectorResumed() throws Exception { // start with the connector paused expectRebalance(1, Arrays.asList(CONN1), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT_PAUSED_CONN1); + expectConfigRefreshAndSnapshot(SNAPSHOT_PAUSED_CONN1); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); Capture> onStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), @@ -2015,7 +2033,7 @@ public void testUnknownConnectorPaused() throws Exception { // join expectRebalance(1, Collections.emptyList(), singletonList(TASK0)); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); worker.startTask(EasyMock.eq(TASK0), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED)); PowerMock.expectLastCall().andReturn(true); @@ -2054,7 +2072,7 @@ public void testConnectorPausedRunningTaskOnly() throws Exception { // join expectRebalance(1, Collections.emptyList(), singletonList(TASK0)); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); worker.startTask(EasyMock.eq(TASK0), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED)); PowerMock.expectLastCall().andReturn(true); @@ -2102,7 +2120,7 @@ public void testConnectorResumedRunningTaskOnly() throws Exception { // join expectRebalance(1, Collections.emptyList(), singletonList(TASK0)); - expectPostRebalanceCatchup(SNAPSHOT_PAUSED_CONN1); + expectConfigRefreshAndSnapshot(SNAPSHOT_PAUSED_CONN1); worker.startTask(EasyMock.eq(TASK0), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.anyObject(), EasyMock.eq(herder), EasyMock.eq(TargetState.PAUSED)); PowerMock.expectLastCall().andReturn(true); @@ -2197,7 +2215,7 @@ public void testJoinLeaderCatchUpFails() throws Exception { ConnectProtocol.Assignment.CONFIG_MISMATCH, 1, "leader", "leaderUrl", Collections.emptyList(), Collections.emptyList(), 0, true); // Reading to end of log times out - configBackingStore.refresh(EasyMock.anyLong(), EasyMock.anyObject(TimeUnit.class)); + configBackingStore.refresh(anyLong(), EasyMock.anyObject(TimeUnit.class)); EasyMock.expectLastCall().andThrow(new TimeoutException()); member.maybeLeaveGroup(EasyMock.eq("taking too long to read the log")); EasyMock.expectLastCall(); @@ -2205,7 +2223,7 @@ public void testJoinLeaderCatchUpFails() throws Exception { // After backoff, restart the process and this time succeed expectRebalance(1, Arrays.asList(CONN1), Arrays.asList(TASK1), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); Capture> onStart = newCapture(); @@ -2263,7 +2281,7 @@ public void testJoinLeaderCatchUpRetriesForIncrementalCooperative() throws Excep EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V1); expectRebalance(1, Arrays.asList(CONN1), Arrays.asList(TASK1), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -2303,7 +2321,7 @@ public void testJoinLeaderCatchUpRetriesForIncrementalCooperative() throws Excep member.requestRejoin(); for (int i = retries; i >= 0; --i) { // Reading to end of log times out - configBackingStore.refresh(EasyMock.anyLong(), EasyMock.anyObject(TimeUnit.class)); + configBackingStore.refresh(anyLong(), EasyMock.anyObject(TimeUnit.class)); EasyMock.expectLastCall().andThrow(new TimeoutException()); member.maybeLeaveGroup(EasyMock.eq("taking too long to read the log")); EasyMock.expectLastCall(); @@ -2313,7 +2331,7 @@ public void testJoinLeaderCatchUpRetriesForIncrementalCooperative() throws Excep expectRebalance(Collections.emptyList(), Collections.emptyList(), ConnectProtocol.Assignment.NO_ERROR, 1, "leader", "leaderUrl", Arrays.asList(CONN1), Arrays.asList(TASK1), 0, true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -2354,7 +2372,7 @@ public void testJoinLeaderCatchUpFailsForIncrementalCooperative() throws Excepti EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V1); expectRebalance(1, Arrays.asList(CONN1), Arrays.asList(TASK1), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -2395,7 +2413,7 @@ public void testJoinLeaderCatchUpFailsForIncrementalCooperative() throws Excepti member.requestRejoin(); for (int i = maxRetries; i >= 0; --i) { // Reading to end of log times out - configBackingStore.refresh(EasyMock.anyLong(), EasyMock.anyObject(TimeUnit.class)); + configBackingStore.refresh(anyLong(), EasyMock.anyObject(TimeUnit.class)); EasyMock.expectLastCall().andThrow(new TimeoutException()); member.maybeLeaveGroup(EasyMock.eq("taking too long to read the log")); EasyMock.expectLastCall(); @@ -2411,7 +2429,7 @@ public void testJoinLeaderCatchUpFailsForIncrementalCooperative() throws Excepti ConnectProtocol.Assignment.NO_ERROR, 1, "leader", "leaderUrl", Arrays.asList(CONN1), Arrays.asList(TASK1), 0, true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.poll(EasyMock.anyInt()); PowerMock.expectLastCall(); @@ -2459,9 +2477,9 @@ public void testAccessors() throws Exception { EasyMock.replay(configTransformer); ClusterConfigState snapshotWithTransform = new ClusterConfigState(1, null, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet(), configTransformer); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet(), configTransformer); - expectPostRebalanceCatchup(snapshotWithTransform); + expectConfigRefreshAndSnapshot(snapshotWithTransform); member.wakeup(); @@ -2506,7 +2524,7 @@ public void testAccessors() throws Exception { public void testPutConnectorConfig() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); expectRebalance(1, Arrays.asList(CONN1), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V0); Capture> onFirstStart = newCapture(); worker.startConnector(EasyMock.eq(CONN1), EasyMock.anyObject(), EasyMock.anyObject(), @@ -2602,7 +2620,7 @@ public void testKeyRotationWhenWorkerBecomesLeader() throws Exception { EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); expectRebalance(1, Collections.emptyList(), Collections.emptyList()); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); // First rebalance: poll indefinitely as no key has been read yet, so expiration doesn't come into play member.poll(Long.MAX_VALUE); EasyMock.expectLastCall(); @@ -2611,8 +2629,8 @@ public void testKeyRotationWhenWorkerBecomesLeader() throws Exception { SessionKey initialKey = new SessionKey(EasyMock.mock(SecretKey.class), 0); ClusterConfigState snapshotWithKey = new ClusterConfigState(2, initialKey, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet()); - expectPostRebalanceCatchup(snapshotWithKey); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); + expectConfigRefreshAndSnapshot(snapshotWithKey); // Second rebalance: poll indefinitely as worker is follower, so expiration still doesn't come into play member.poll(Long.MAX_VALUE); EasyMock.expectLastCall(); @@ -2653,8 +2671,8 @@ public void testKeyRotationDisabledWhenWorkerBecomesFollower() throws Exception SessionKey initialKey = new SessionKey(initialSecretKey, time.milliseconds()); ClusterConfigState snapshotWithKey = new ClusterConfigState(1, initialKey, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet()); - expectPostRebalanceCatchup(snapshotWithKey); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); + expectConfigRefreshAndSnapshot(snapshotWithKey); // First rebalance: poll for a limited time as worker is leader and must wake up for key expiration Capture firstPollTimeout = EasyMock.newCapture(); member.poll(EasyMock.captureLong(firstPollTimeout)); @@ -2786,7 +2804,7 @@ public void testFailedToWriteSessionKey() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); configBackingStore.putSessionKey(anyObject(SessionKey.class)); EasyMock.expectLastCall().andThrow(new ConnectException("Oh no!")); @@ -2794,7 +2812,7 @@ public void testFailedToWriteSessionKey() throws Exception { // then ensure we're still active in the group // then try a second time to write a new session key, // then finally begin polling for group activity - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); member.ensureActive(); PowerMock.expectLastCall(); configBackingStore.putSessionKey(anyObject(SessionKey.class)); @@ -2818,7 +2836,7 @@ public void testFailedToReadBackNewlyWrittenSessionKey() throws Exception { SessionKey sessionKey = new SessionKey(secretKey, time.milliseconds()); ClusterConfigState snapshotWithSessionKey = new ClusterConfigState(1, sessionKey, Collections.singletonMap(CONN1, 3), Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), - TASK_CONFIGS_MAP, Collections.emptySet()); + TASK_CONFIGS_MAP, Collections.emptyMap(), Collections.emptyMap(), Collections.emptySet(), Collections.emptySet()); // First tick -- after joining the group, we try to write a new session key to // the config topic, and fail (in this case, we're trying to simulate that we've @@ -2828,7 +2846,7 @@ public void testFailedToReadBackNewlyWrittenSessionKey() throws Exception { EasyMock.expect(member.memberId()).andStubReturn("leader"); EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); - expectPostRebalanceCatchup(SNAPSHOT); + expectConfigRefreshAndSnapshot(SNAPSHOT); configBackingStore.putSessionKey(anyObject(SessionKey.class)); EasyMock.expectLastCall().andThrow(new ConnectException("Oh no!")); @@ -2837,7 +2855,7 @@ public void testFailedToReadBackNewlyWrittenSessionKey() throws Exception { // then ensure we're still active in the group // then finally begin polling for group activity // Importantly, we do not try to write a new session key this time around - configBackingStore.refresh(EasyMock.anyLong(), EasyMock.anyObject(TimeUnit.class)); + configBackingStore.refresh(anyLong(), EasyMock.anyObject(TimeUnit.class)); EasyMock.expectLastCall().andAnswer(() -> { configUpdateListener.onSessionKeyUpdate(sessionKey); return null; @@ -2856,6 +2874,562 @@ public void testFailedToReadBackNewlyWrittenSessionKey() throws Exception { PowerMock.verifyAll(); } + @Test + public void testFenceZombiesInvalidSignature() { + // Don't have to run the whole gamut of scenarios (invalid signature, missing signature, earlier protocol that doesn't require signatures) + // since the task config tests cover that pretty well. One sanity check to ensure that this method is guarded should be sufficient. + Callback taskConfigCb = EasyMock.mock(Callback.class); + Capture errorCapture = Capture.newInstance(); + taskConfigCb.onCompletion(capture(errorCapture), EasyMock.eq(null)); + EasyMock.expectLastCall().once(); + + EasyMock.expect(member.currentProtocolVersion()).andReturn(CONNECT_PROTOCOL_V2).anyTimes(); + + InternalRequestSignature signature = EasyMock.mock(InternalRequestSignature.class); + EasyMock.expect(signature.keyAlgorithm()).andReturn("HmacSHA256").anyTimes(); + EasyMock.expect(signature.isValid(EasyMock.anyObject())).andReturn(false).anyTimes(); + + PowerMock.replayAll(taskConfigCb, signature); + + herder.fenceZombieSourceTasks(CONN1, taskConfigCb, signature); + + PowerMock.verifyAll(); + assertTrue(errorCapture.getValue() instanceof ConnectRestException); + assertEquals(FORBIDDEN.getStatusCode(), ((ConnectRestException) errorCapture.getValue()).statusCode()); + } + + @Test + public void testTaskRequestedZombieFencingForwardedToLeader() throws Exception { + testTaskRequestedZombieFencingForwardingToLeader(true); + } + + @Test + public void testTaskRequestedZombieFencingFailedForwardToLeader() throws Exception { + testTaskRequestedZombieFencingForwardingToLeader(false); + } + + private void testTaskRequestedZombieFencingForwardingToLeader(boolean succeed) throws Exception { + expectHerderStartup(); + ExecutorService forwardRequestExecutor = EasyMock.mock(ExecutorService.class); + herder.forwardRequestExecutor = forwardRequestExecutor; + + EasyMock.expect(member.memberId()).andStubReturn("member"); + EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); + expectConfigRefreshAndSnapshot(SNAPSHOT); + + expectRebalance(1, Collections.emptyList(), Collections.emptyList()); + + expectAnyTicks(); + + member.wakeup(); + EasyMock.expectLastCall(); + + PowerMock.mockStatic(RestClient.class); + + org.easymock.IExpectationSetters> expectRequest = EasyMock.expect( + RestClient.httpRequest( + anyObject(), EasyMock.eq("PUT"), EasyMock.isNull(), EasyMock.isNull(), EasyMock.isNull(), anyObject(), anyObject(), anyObject() + )); + if (succeed) { + expectRequest.andReturn(null); + } else { + expectRequest.andThrow(new ConnectRestException(409, "Rebalance :(")); + } + + Capture forwardRequest = EasyMock.newCapture(); + forwardRequestExecutor.execute(EasyMock.capture(forwardRequest)); + EasyMock.expectLastCall().andAnswer(() -> { + forwardRequest.getValue().run(); + return null; + }); + + expectHerderShutdown(true); + forwardRequestExecutor.shutdown(); + EasyMock.expectLastCall(); + EasyMock.expect(forwardRequestExecutor.awaitTermination(anyLong(), anyObject())).andReturn(true); + + PowerMock.replayAll(forwardRequestExecutor); + + + startBackgroundHerder(); + + FutureCallback fencing = new FutureCallback<>(); + herder.fenceZombieSourceTasks(TASK1, fencing); + + if (!succeed) { + ExecutionException fencingException = + assertThrows(ExecutionException.class, () -> fencing.get(10, TimeUnit.SECONDS)); + assertTrue(fencingException.getCause() instanceof ConnectException); + } else { + fencing.get(10, TimeUnit.SECONDS); + } + + stopBackgroundHerder(); + + PowerMock.verifyAll(); + } + + @Test + public void testExternalZombieFencingRequestForAlreadyFencedConnector() throws Exception { + ClusterConfigState configState = exactlyOnceSnapshot( + expectNewSessionKey(), + TASK_CONFIGS_MAP, + Collections.singletonMap(CONN1, 12), + Collections.singletonMap(CONN1, 5), + Collections.emptySet() + ); + testExternalZombieFencingRequestThatRequiresNoPhysicalFencing(configState, false); + } + + @Test + public void testExternalZombieFencingRequestForSingleTaskConnector() throws Exception { + ClusterConfigState configState = exactlyOnceSnapshot( + expectNewSessionKey(), + Collections.singletonMap(TASK1, TASK_CONFIG), + Collections.singletonMap(CONN1, 1), + Collections.singletonMap(CONN1, 5), + Collections.singleton(CONN1) + ); + testExternalZombieFencingRequestThatRequiresNoPhysicalFencing(configState, true); + } + + @Test + public void testExternalZombieFencingRequestForFreshConnector() throws Exception { + ClusterConfigState configState = exactlyOnceSnapshot( + expectNewSessionKey(), + TASK_CONFIGS_MAP, + Collections.emptyMap(), + Collections.singletonMap(CONN1, 5), + Collections.singleton(CONN1) + ); + testExternalZombieFencingRequestThatRequiresNoPhysicalFencing(configState, true); + } + + private void testExternalZombieFencingRequestThatRequiresNoPhysicalFencing( + ClusterConfigState configState, boolean expectTaskCountRecord + ) throws Exception { + expectHerderStartup(); + + EasyMock.expect(member.memberId()).andStubReturn("leader"); + EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); + expectConfigRefreshAndSnapshot(SNAPSHOT); + + expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); + + expectAnyTicks(); + + member.wakeup(); + EasyMock.expectLastCall().anyTimes(); + + expectConfigRefreshAndSnapshot(configState); + + if (expectTaskCountRecord) { + configBackingStore.putTaskCountRecord(CONN1, 1); + EasyMock.expectLastCall(); + } + + expectHerderShutdown(false); + + PowerMock.replayAll(); + + + startBackgroundHerder(); + + FutureCallback fencing = new FutureCallback<>(); + herder.fenceZombieSourceTasks(CONN1, fencing); + + fencing.get(10, TimeUnit.SECONDS); + + stopBackgroundHerder(); + + PowerMock.verifyAll(); + } + + /** + * Tests zombie fencing that completes extremely quickly, and causes all callback-related logic to be invoked + * effectively as soon as it's put into place. This is not likely to occur in practice, but the test is valuable all the + * same especially since it may shed light on potential deadlocks when the unlikely-but-not-impossible happens. + */ + @Test + public void testExternalZombieFencingRequestImmediateCompletion() throws Exception { + expectHerderStartup(); + EasyMock.expect(member.memberId()).andStubReturn("leader"); + EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); + expectConfigRefreshAndSnapshot(SNAPSHOT); + + expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); + SessionKey sessionKey = expectNewSessionKey(); + + expectAnyTicks(); + + member.wakeup(); + EasyMock.expectLastCall(); + + ClusterConfigState configState = exactlyOnceSnapshot( + sessionKey, + TASK_CONFIGS_MAP, + Collections.singletonMap(CONN1, 2), + Collections.singletonMap(CONN1, 5), + Collections.singleton(CONN1) + ); + expectConfigRefreshAndSnapshot(configState); + + // The future returned by Worker::fenceZombies + KafkaFuture workerFencingFuture = EasyMock.mock(KafkaFuture.class); + // The future tracked by the herder (which tracks the fencing performed by the worker and the possible followup write to the config topic) + KafkaFuture herderFencingFuture = EasyMock.mock(KafkaFuture.class); + + // Immediately invoke callbacks that the herder sets up for when the worker fencing and writes to the config topic have completed + for (int i = 0; i < 2; i++) { + Capture> herderFencingCallback = EasyMock.newCapture(); + EasyMock.expect(herderFencingFuture.whenComplete(EasyMock.capture(herderFencingCallback))).andAnswer(() -> { + herderFencingCallback.getValue().accept(null, null); + return null; + }); + } + + Capture> fencingFollowup = EasyMock.newCapture(); + EasyMock.expect(workerFencingFuture.thenApply(EasyMock.capture(fencingFollowup))).andAnswer(() -> { + fencingFollowup.getValue().apply(null); + return herderFencingFuture; + }); + EasyMock.expect(worker.fenceZombies(EasyMock.eq(CONN1), EasyMock.eq(2), EasyMock.eq(CONN1_CONFIG))) + .andReturn(workerFencingFuture); + + expectConfigRefreshAndSnapshot(configState); + + configBackingStore.putTaskCountRecord(CONN1, 1); + EasyMock.expectLastCall(); + + expectHerderShutdown(true); + + PowerMock.replayAll(workerFencingFuture, herderFencingFuture); + + + startBackgroundHerder(); + + FutureCallback fencing = new FutureCallback<>(); + herder.fenceZombieSourceTasks(CONN1, fencing); + + fencing.get(10, TimeUnit.SECONDS); + + stopBackgroundHerder(); + + PowerMock.verifyAll(); + } + + /** + * The herder tries to perform a round of fencing, but fails synchronously while invoking Worker::fenceZombies + */ + @Test + public void testExternalZombieFencingRequestSynchronousFailure() throws Exception { + expectHerderStartup(); + EasyMock.expect(member.memberId()).andStubReturn("leader"); + EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); + expectConfigRefreshAndSnapshot(SNAPSHOT); + + expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); + SessionKey sessionKey = expectNewSessionKey(); + + expectAnyTicks(); + + member.wakeup(); + EasyMock.expectLastCall(); + + ClusterConfigState configState = exactlyOnceSnapshot( + sessionKey, + TASK_CONFIGS_MAP, + Collections.singletonMap(CONN1, 2), + Collections.singletonMap(CONN1, 5), + Collections.singleton(CONN1) + ); + expectConfigRefreshAndSnapshot(configState); + + Exception fencingException = new KafkaException("whoops!"); + EasyMock.expect(worker.fenceZombies(EasyMock.eq(CONN1), EasyMock.eq(2), EasyMock.eq(CONN1_CONFIG))) + .andThrow(fencingException); + + expectHerderShutdown(true); + + PowerMock.replayAll(); + + + startBackgroundHerder(); + + FutureCallback fencing = new FutureCallback<>(); + herder.fenceZombieSourceTasks(CONN1, fencing); + + ExecutionException exception = assertThrows(ExecutionException.class, () -> fencing.get(10, TimeUnit.SECONDS)); + assertEquals(fencingException, exception.getCause()); + + stopBackgroundHerder(); + + PowerMock.verifyAll(); + } + + /** + * The herder tries to perform a round of fencing and is able to retrieve a future from worker::fenceZombies, but the attempt + * fails at a later point. + */ + @Test + public void testExternalZombieFencingRequestAsynchronousFailure() throws Exception { + expectHerderStartup(); + EasyMock.expect(member.memberId()).andStubReturn("leader"); + EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); + expectConfigRefreshAndSnapshot(SNAPSHOT); + + expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); + SessionKey sessionKey = expectNewSessionKey(); + + expectAnyTicks(); + + member.wakeup(); + EasyMock.expectLastCall(); + + ClusterConfigState configState = exactlyOnceSnapshot( + sessionKey, + TASK_CONFIGS_MAP, + Collections.singletonMap(CONN1, 2), + Collections.singletonMap(CONN1, 5), + Collections.singleton(CONN1) + ); + expectConfigRefreshAndSnapshot(configState); + + // The future returned by Worker::fenceZombies + KafkaFuture workerFencingFuture = EasyMock.mock(KafkaFuture.class); + // The future tracked by the herder (which tracks the fencing performed by the worker and the possible followup write to the config topic) + KafkaFuture herderFencingFuture = EasyMock.mock(KafkaFuture.class); + // The callbacks that the herder has accrued for outstanding fencing futures + Capture> herderFencingCallbacks = EasyMock.newCapture(CaptureType.ALL); + + EasyMock.expect(worker.fenceZombies(EasyMock.eq(CONN1), EasyMock.eq(2), EasyMock.eq(CONN1_CONFIG))) + .andReturn(workerFencingFuture); + + EasyMock.expect(workerFencingFuture.thenApply(EasyMock.>anyObject())) + .andReturn(herderFencingFuture); + + CountDownLatch callbacksInstalled = new CountDownLatch(2); + for (int i = 0; i < 2; i++) { + EasyMock.expect(herderFencingFuture.whenComplete(EasyMock.capture(herderFencingCallbacks))).andAnswer(() -> { + callbacksInstalled.countDown(); + return null; + }); + } + + expectHerderShutdown(true); + + PowerMock.replayAll(workerFencingFuture, herderFencingFuture); + + + startBackgroundHerder(); + + FutureCallback fencing = new FutureCallback<>(); + herder.fenceZombieSourceTasks(CONN1, fencing); + + assertTrue(callbacksInstalled.await(10, TimeUnit.SECONDS)); + + Exception fencingException = new AuthorizationException("you didn't say the magic word"); + herderFencingCallbacks.getValues().forEach(cb -> cb.accept(null, fencingException)); + + ExecutionException exception = assertThrows(ExecutionException.class, () -> fencing.get(10, TimeUnit.SECONDS)); + assertTrue(exception.getCause() instanceof ConnectException); + + stopBackgroundHerder(); + + PowerMock.verifyAll(); + } + + /** + * Issues multiple rapid fencing requests for a handful of connectors, each of which takes a little while to complete. + * This mimics what might happen when a few connectors are reconfigured in quick succession and each task for the + * connector needs to hit the leader with a fencing request during its preflight check. + */ + @Test + public void testExternalZombieFencingRequestDelayedCompletion() throws Exception { + final String conn3 = "SourceC"; + final Map tasksPerConnector = new HashMap<>(); + tasksPerConnector.put(CONN1, 5); + tasksPerConnector.put(CONN2, 3); + tasksPerConnector.put(conn3, 12); + + expectHerderStartup(); + EasyMock.expect(member.memberId()).andStubReturn("leader"); + EasyMock.expect(member.currentProtocolVersion()).andStubReturn(CONNECT_PROTOCOL_V2); + expectConfigRefreshAndSnapshot(SNAPSHOT); + + expectRebalance(1, Collections.emptyList(), Collections.emptyList(), true); + SessionKey sessionKey = expectNewSessionKey(); + + expectAnyTicks(); + + // We invoke the herder's fenceZombies method repeatedly, which adds a new request to the queue. + // If the queue is empty, the member is woken up; however, if two or more requests are issued in rapid + // succession, the member won't be woken up. We allow the member to be woken up any number of times + // here since it's not critical to the testing logic and it's difficult to mock things in order to lead to an + // exact number of wakeups. + member.wakeup(); + EasyMock.expectLastCall().anyTimes(); + + Map taskCountRecords = new HashMap<>(); + taskCountRecords.put(CONN1, 2); + taskCountRecords.put(CONN2, 3); + taskCountRecords.put(conn3, 5); + Map taskConfigGenerations = new HashMap<>(); + taskConfigGenerations.put(CONN1, 3); + taskConfigGenerations.put(CONN2, 4); + taskConfigGenerations.put(conn3, 2); + Set pendingFencing = new HashSet<>(Arrays.asList(CONN1, CONN2, conn3)); + ClusterConfigState configState = exactlyOnceSnapshot( + sessionKey, + TASK_CONFIGS_MAP, + taskCountRecords, + taskConfigGenerations, + pendingFencing, + tasksPerConnector + ); + tasksPerConnector.keySet().forEach(c -> expectConfigRefreshAndSnapshot(configState)); + + // The callbacks that the herder has accrued for outstanding fencing futures, which will be completed after + // a successful round of fencing and a task record write to the config topic + Map>> herderFencingCallbacks = new HashMap<>(); + // The callbacks that the herder has installed for after a successful round of zombie fencing, but before writing + // a task record to the config topic + Map>> workerFencingFollowups = new HashMap<>(); + + Map callbacksInstalled = new HashMap<>(); + tasksPerConnector.forEach((connector, numStackedRequests) -> { + // The future returned by Worker::fenceZombies + KafkaFuture workerFencingFuture = EasyMock.mock(KafkaFuture.class); + // The future tracked by the herder (which tracks the fencing performed by the worker and the possible followup write to the config topic) + KafkaFuture herderFencingFuture = EasyMock.mock(KafkaFuture.class); + + Capture> herderFencingCallback = EasyMock.newCapture(CaptureType.ALL); + herderFencingCallbacks.put(connector, herderFencingCallback); + + // Don't immediately invoke callbacks that the herder sets up for when the worker fencing and writes to the config topic have completed + // Instead, wait for them to be installed, then invoke them explicitly after the fact on a thread separate from the herder's tick thread + EasyMock.expect(herderFencingFuture.whenComplete(EasyMock.capture(herderFencingCallback))) + .andReturn(null) + .times(numStackedRequests + 1); + + Capture> fencingFollowup = EasyMock.newCapture(); + CountDownLatch callbackInstalled = new CountDownLatch(1); + workerFencingFollowups.put(connector, fencingFollowup); + callbacksInstalled.put(connector, callbackInstalled); + EasyMock.expect(workerFencingFuture.thenApply(EasyMock.capture(fencingFollowup))).andAnswer(() -> { + callbackInstalled.countDown(); + return herderFencingFuture; + }); + + // We should only perform a single physical zombie fencing; all the subsequent requests should be stacked onto the first one + EasyMock.expect(worker.fenceZombies( + EasyMock.eq(connector), EasyMock.eq(taskCountRecords.get(connector)), EasyMock.anyObject()) + ).andReturn(workerFencingFuture); + + for (int i = 0; i < numStackedRequests; i++) { + expectConfigRefreshAndSnapshot(configState); + } + + PowerMock.replay(workerFencingFuture, herderFencingFuture); + }); + + tasksPerConnector.forEach((connector, taskCount) -> { + configBackingStore.putTaskCountRecord(connector, taskCount); + EasyMock.expectLastCall(); + }); + + expectHerderShutdown(false); + + PowerMock.replayAll(); + + + startBackgroundHerder(); + + List> stackedFencingRequests = new ArrayList<>(); + tasksPerConnector.forEach((connector, numStackedRequests) -> { + List> connectorFencingRequests = IntStream.range(0, numStackedRequests) + .mapToObj(i -> new FutureCallback()) + .collect(Collectors.toList()); + + connectorFencingRequests.forEach(fencing -> + herder.fenceZombieSourceTasks(connector, fencing) + ); + + stackedFencingRequests.addAll(connectorFencingRequests); + }); + + callbacksInstalled.forEach((connector, latch) -> { + try { + assertTrue(latch.await(10, TimeUnit.SECONDS)); + workerFencingFollowups.get(connector).getValue().apply(null); + herderFencingCallbacks.get(connector).getValues().forEach(cb -> cb.accept(null, null)); + } catch (InterruptedException e) { + fail("Unexpectedly interrupted"); + } + }); + + for (FutureCallback fencing : stackedFencingRequests) { + fencing.get(10, TimeUnit.SECONDS); + } + + stopBackgroundHerder(); + + PowerMock.verifyAll(); + } + + @Test + public void testVerifyTaskGeneration() { + Map taskConfigGenerations = new HashMap<>(); + herder.configState = new ClusterConfigState(1, null, Collections.singletonMap(CONN1, 3), + Collections.singletonMap(CONN1, CONN1_CONFIG), Collections.singletonMap(CONN1, TargetState.STARTED), + TASK_CONFIGS_MAP, Collections.emptyMap(), taskConfigGenerations, Collections.emptySet(), Collections.emptySet()); + + Callback verifyCallback = EasyMock.mock(Callback.class); + for (int i = 0; i < 5; i++) { + verifyCallback.onCompletion(null, null); + EasyMock.expectLastCall(); + } + + PowerMock.replayAll(); + + herder.assignment = new ExtendedAssignment( + (short) 2, (short) 0, "leader", "leaderUrl", 0, + Collections.emptySet(), Collections.singleton(TASK1), + Collections.emptySet(), Collections.emptySet(), 0); + + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 0, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 1, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 2, verifyCallback)); + + taskConfigGenerations.put(CONN1, 0); + herder.verifyTaskGenerationAndOwnership(TASK1, 0, verifyCallback); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 1, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 2, verifyCallback)); + + taskConfigGenerations.put(CONN1, 1); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 0, verifyCallback)); + herder.verifyTaskGenerationAndOwnership(TASK1, 1, verifyCallback); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 2, verifyCallback)); + + taskConfigGenerations.put(CONN1, 2); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 0, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 1, verifyCallback)); + herder.verifyTaskGenerationAndOwnership(TASK1, 2, verifyCallback); + + taskConfigGenerations.put(CONN1, 3); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 0, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 1, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(TASK1, 2, verifyCallback)); + + ConnectorTaskId unassignedTask = new ConnectorTaskId(CONN2, 0); + taskConfigGenerations.put(unassignedTask.connector(), 1); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(unassignedTask, 0, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(unassignedTask, 1, verifyCallback)); + assertThrows(ConnectException.class, () -> herder.verifyTaskGenerationAndOwnership(unassignedTask, 2, verifyCallback)); + + PowerMock.verifyAll(); + } + @Test public void testKeyExceptionDetection() { assertFalse(herder.isPossibleExpiredKeyException( @@ -2912,6 +3486,7 @@ private void expectRebalance(final long offset, final List assignedConnectors, final List assignedTasks, final boolean isLeader) { + expectRebalance(Collections.emptyList(), Collections.emptyList(), ConnectProtocol.Assignment.NO_ERROR, offset, "leader", "leaderUrl", assignedConnectors, assignedTasks, 0, isLeader); } @@ -3003,10 +3578,111 @@ private void expectRebalance(final Collection revokedConnectors, PowerMock.expectLastCall(); } - private void expectPostRebalanceCatchup(final ClusterConfigState readToEndSnapshot) throws TimeoutException { - configBackingStore.refresh(EasyMock.anyLong(), EasyMock.anyObject(TimeUnit.class)); + private ClusterConfigState exactlyOnceSnapshot( + SessionKey sessionKey, + Map> taskConfigs, + Map taskCountRecords, + Map taskConfigGenerations, + Set pendingFencing) { + + Set connectors = new HashSet<>(); + connectors.addAll(taskCountRecords.keySet()); + connectors.addAll(taskConfigGenerations.keySet()); + connectors.addAll(pendingFencing); + Map taskCounts = connectors.stream() + .collect(Collectors.toMap(Function.identity(), c -> 1)); + + return exactlyOnceSnapshot(sessionKey, taskConfigs, taskCountRecords, taskConfigGenerations, pendingFencing, taskCounts); + } + + private ClusterConfigState exactlyOnceSnapshot( + SessionKey sessionKey, + Map> taskConfigs, + Map taskCountRecords, + Map taskConfigGenerations, + Set pendingFencing, + Map taskCounts) { + + Set connectors = new HashSet<>(); + connectors.addAll(taskCounts.keySet()); + connectors.addAll(taskCountRecords.keySet()); + connectors.addAll(taskConfigGenerations.keySet()); + connectors.addAll(pendingFencing); + + Map> connectorConfigs = connectors.stream() + .collect(Collectors.toMap(Function.identity(), c -> CONN1_CONFIG)); + + return new ClusterConfigState(1, sessionKey, taskCounts, + connectorConfigs, Collections.singletonMap(CONN1, TargetState.STARTED), + taskConfigs, taskCountRecords, taskConfigGenerations, pendingFencing, Collections.emptySet()); + } + + private void expectAnyTicks() { + member.ensureActive(); + EasyMock.expectLastCall().anyTimes(); + member.poll(EasyMock.anyInt()); + PowerMock.expectLastCall().anyTimes(); + } + + private SessionKey expectNewSessionKey() { + SecretKey secretKey = EasyMock.niceMock(SecretKey.class); + EasyMock.expect(secretKey.getAlgorithm()).andReturn(INTER_WORKER_KEY_GENERATION_ALGORITHM_DEFAULT).anyTimes(); + EasyMock.expect(secretKey.getEncoded()).andReturn(new byte[32]).anyTimes(); + SessionKey sessionKey = new SessionKey(secretKey, time.milliseconds() + TimeUnit.DAYS.toMillis(1)); + configBackingStore.putSessionKey(anyObject(SessionKey.class)); + EasyMock.expectLastCall().andAnswer(() -> { + configUpdateListener.onSessionKeyUpdate(sessionKey); + return null; + }); + EasyMock.replay(secretKey); + return sessionKey; + } + + private void expectConfigRefreshAndSnapshot(final ClusterConfigState readToEndSnapshot) { + try { + configBackingStore.refresh(anyLong(), EasyMock.anyObject(TimeUnit.class)); + EasyMock.expectLastCall(); + EasyMock.expect(configBackingStore.snapshot()).andReturn(readToEndSnapshot); + } catch (TimeoutException e) { + fail("Mocked method should not throw checked exception"); + } + } + + private void startBackgroundHerder() { + herderExecutor = Executors.newSingleThreadExecutor(); + herderExecutor.submit(herder); + } + + private void stopBackgroundHerder() throws Exception { + herder.stop(); + herderExecutor.shutdown(); + herderExecutor.awaitTermination(10, TimeUnit.SECONDS); + } + + private void expectHerderStartup() { + worker.start(); + EasyMock.expectLastCall(); + statusBackingStore.start(); + EasyMock.expectLastCall(); + configBackingStore.start(); + EasyMock.expectLastCall(); + } + + private void expectHerderShutdown(boolean wakeup) { + if (wakeup) { + member.wakeup(); + EasyMock.expectLastCall(); + } + EasyMock.expect(worker.connectorNames()).andReturn(Collections.emptySet()); + EasyMock.expect(worker.taskIds()).andReturn(Collections.emptySet()); + member.stop(); + EasyMock.expectLastCall(); + statusBackingStore.stop(); + EasyMock.expectLastCall(); + configBackingStore.stop(); + EasyMock.expectLastCall(); + worker.stop(); EasyMock.expectLastCall(); - EasyMock.expect(configBackingStore.snapshot()).andReturn(readToEndSnapshot); } private void assertStatistics(int expectedEpoch, int completedRebalances, double rebalanceTime, double millisSinceLastRebalance) { diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignorTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignorTest.java index bb767139bb910..ed825312096f7 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignorTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignorTest.java @@ -1111,6 +1111,9 @@ private ClusterConfigState configState() { connectorConfigs, targetStates, taskConfigs, + Collections.emptyMap(), + Collections.emptyMap(), + Collections.emptySet(), Collections.emptySet()); } diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/WorkerCoordinatorTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/WorkerCoordinatorTest.java index e17fb7daf7543..c3715aa3028ec 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/WorkerCoordinatorTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/WorkerCoordinatorTest.java @@ -157,6 +157,9 @@ public void setup() { Collections.singletonMap(connectorId1, new HashMap<>()), Collections.singletonMap(connectorId1, TargetState.STARTED), Collections.singletonMap(taskId1x0, new HashMap<>()), + Collections.emptyMap(), + Collections.emptyMap(), + Collections.emptySet(), Collections.emptySet() ); @@ -180,6 +183,9 @@ public void setup() { configState2ConnectorConfigs, configState2TargetStates, configState2TaskConfigs, + Collections.emptyMap(), + Collections.emptyMap(), + Collections.emptySet(), Collections.emptySet() ); @@ -206,6 +212,9 @@ public void setup() { configStateSingleTaskConnectorsConnectorConfigs, configStateSingleTaskConnectorsTargetStates, configStateSingleTaskConnectorsTaskConfigs, + Collections.emptyMap(), + Collections.emptyMap(), + Collections.emptySet(), Collections.emptySet() ); } diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java index 3c5fe92d8b561..ba89a21c89fef 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/ConnectorsResourceTest.java @@ -848,6 +848,70 @@ public void testRestartConnectorAndTasksRequestAccepted() throws Throwable { PowerMock.verifyAll(); } + @Test + public void testFenceZombiesNoInternalRequestSignature() throws Throwable { + final Capture> cb = Capture.newInstance(); + herder.fenceZombieSourceTasks(EasyMock.eq(CONNECTOR_NAME), EasyMock.capture(cb), EasyMock.anyObject(InternalRequestSignature.class)); + expectAndCallbackResult(cb, null); + + PowerMock.replayAll(); + + connectorsResource.fenceZombies(CONNECTOR_NAME, NULL_HEADERS, FORWARD, serializeAsBytes(null)); + + PowerMock.verifyAll(); + } + + @Test + public void testFenceZombiesWithInternalRequestSignature() throws Throwable { + final String signatureAlgorithm = "HmacSHA256"; + final String encodedSignature = "Kv1/OSsxzdVIwvZ4e30avyRIVrngDfhzVUm/kAZEKc4="; + + final Capture> cb = Capture.newInstance(); + final Capture signatureCapture = Capture.newInstance(); + herder.fenceZombieSourceTasks(EasyMock.eq(CONNECTOR_NAME), EasyMock.capture(cb), EasyMock.capture(signatureCapture)); + expectAndCallbackResult(cb, null); + + HttpHeaders headers = EasyMock.mock(HttpHeaders.class); + EasyMock.expect(headers.getHeaderString(InternalRequestSignature.SIGNATURE_ALGORITHM_HEADER)) + .andReturn(signatureAlgorithm) + .once(); + EasyMock.expect(headers.getHeaderString(InternalRequestSignature.SIGNATURE_HEADER)) + .andReturn(encodedSignature) + .once(); + + PowerMock.replayAll(headers); + + connectorsResource.fenceZombies(CONNECTOR_NAME, headers, FORWARD, serializeAsBytes(null)); + + PowerMock.verifyAll(); + InternalRequestSignature expectedSignature = new InternalRequestSignature( + serializeAsBytes(null), + Mac.getInstance(signatureAlgorithm), + Base64.getDecoder().decode(encodedSignature) + ); + assertEquals( + expectedSignature, + signatureCapture.getValue() + ); + + PowerMock.verifyAll(); + } + + @Test + public void testFenceZombiesConnectorNotFound() throws Throwable { + final Capture> cb = Capture.newInstance(); + herder.fenceZombieSourceTasks(EasyMock.eq(CONNECTOR_NAME), EasyMock.capture(cb), EasyMock.anyObject(InternalRequestSignature.class)); + + expectAndCallbackException(cb, new NotFoundException("not found")); + + PowerMock.replayAll(); + + assertThrows(NotFoundException.class, + () -> connectorsResource.fenceZombies(CONNECTOR_NAME, NULL_HEADERS, FORWARD, serializeAsBytes(null))); + + PowerMock.verifyAll(); + } + @Test public void testRestartConnectorNotFound() { final Capture> cb = Capture.newInstance(); diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java index 629187a6ef2b4..99848d8314744 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/standalone/StandaloneHerderTest.java @@ -364,6 +364,9 @@ public void testRestartTask() throws Exception { Collections.singletonMap(CONNECTOR_NAME, connectorConfig), Collections.singletonMap(CONNECTOR_NAME, TargetState.STARTED), Collections.singletonMap(taskId, taskConfig(SourceSink.SOURCE)), + Collections.emptyMap(), + Collections.emptyMap(), + new HashSet<>(), new HashSet<>(), transformer); worker.startTask(taskId, configState, connectorConfig, taskConfig(SourceSink.SOURCE), herder, TargetState.STARTED); @@ -402,6 +405,9 @@ public void testRestartTaskFailureOnStart() throws Exception { Collections.singletonMap(CONNECTOR_NAME, connectorConfig), Collections.singletonMap(CONNECTOR_NAME, TargetState.STARTED), Collections.singletonMap(new ConnectorTaskId(CONNECTOR_NAME, 0), taskConfig(SourceSink.SOURCE)), + Collections.emptyMap(), + Collections.emptyMap(), + new HashSet<>(), new HashSet<>(), transformer); worker.startTask(taskId, configState, connectorConfig, taskConfig(SourceSink.SOURCE), herder, TargetState.STARTED); @@ -572,6 +578,9 @@ public void testRestartConnectorAndTasksOnlyTasks() throws Exception { Collections.singletonMap(CONNECTOR_NAME, connectorConfig), Collections.singletonMap(CONNECTOR_NAME, TargetState.STARTED), Collections.singletonMap(taskId, taskConfig(SourceSink.SINK)), + Collections.emptyMap(), + Collections.emptyMap(), + new HashSet<>(), new HashSet<>(), transformer); worker.startTask(taskId, configState, connectorConfig, taskConfig(SourceSink.SINK), herder, TargetState.STARTED); @@ -635,6 +644,9 @@ public void testRestartConnectorAndTasksBoth() throws Exception { Collections.singletonMap(CONNECTOR_NAME, connectorConfig), Collections.singletonMap(CONNECTOR_NAME, TargetState.STARTED), Collections.singletonMap(taskId, taskConfig(SourceSink.SINK)), + Collections.emptyMap(), + Collections.emptyMap(), + new HashSet<>(), new HashSet<>(), transformer); worker.startTask(taskId, configState, connectorConfig, taskConfig(SourceSink.SINK), herder, TargetState.STARTED); @@ -878,7 +890,6 @@ private void expectAdd(SourceSink sourceSink) { Capture> onStart = EasyMock.newCapture(); worker.startConnector(EasyMock.eq(CONNECTOR_NAME), EasyMock.eq(connectorProps), EasyMock.anyObject(HerderConnectorContext.class), EasyMock.eq(herder), EasyMock.eq(TargetState.STARTED), EasyMock.capture(onStart)); - // EasyMock.expectLastCall().andReturn(true); EasyMock.expectLastCall().andAnswer(() -> { onStart.getValue().onCompletion(null, TargetState.STARTED); return true; @@ -902,6 +913,9 @@ private void expectAdd(SourceSink sourceSink) { Collections.singletonMap(CONNECTOR_NAME, connectorConfig(sourceSink)), Collections.singletonMap(CONNECTOR_NAME, TargetState.STARTED), Collections.singletonMap(new ConnectorTaskId(CONNECTOR_NAME, 0), generatedTaskProps), + Collections.emptyMap(), + Collections.emptyMap(), + new HashSet<>(), new HashSet<>(), transformer); worker.startTask(new ConnectorTaskId(CONNECTOR_NAME, 0), configState, connectorConfig(sourceSink), generatedTaskProps, herder, TargetState.STARTED); diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaConfigBackingStoreTest.java b/connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaConfigBackingStoreTest.java index 32719d816cde4..b374f8f5d2f79 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaConfigBackingStoreTest.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/storage/KafkaConfigBackingStoreTest.java @@ -105,6 +105,7 @@ public class KafkaConfigBackingStoreTest { private static final List CONNECTOR_CONFIG_KEYS = Arrays.asList("connector-connector1", "connector-connector2"); private static final List COMMIT_TASKS_CONFIG_KEYS = Arrays.asList("commit-connector1", "commit-connector2"); private static final List TARGET_STATE_KEYS = Arrays.asList("target-state-connector1", "target-state-connector2"); + private static final List CONNECTOR_TASK_COUNT_RECORD_KEYS = Arrays.asList("tasks-fencing-connector1", "tasks-fencing-connector2"); private static final String CONNECTOR_1_NAME = "connector1"; private static final String CONNECTOR_2_NAME = "connector2"; @@ -133,6 +134,10 @@ public class KafkaConfigBackingStoreTest { new Struct(KafkaConfigBackingStore.TASK_CONFIGURATION_V0).put("properties", SAMPLE_CONFIGS.get(0)), new Struct(KafkaConfigBackingStore.TASK_CONFIGURATION_V0).put("properties", SAMPLE_CONFIGS.get(1)) ); + private static final List CONNECTOR_TASK_COUNT_RECORD_STRUCTS = Arrays.asList( + new Struct(KafkaConfigBackingStore.TASK_COUNT_RECORD_V0).put("task-count", 6), + new Struct(KafkaConfigBackingStore.TASK_COUNT_RECORD_V0).put("task-count", 9) + ); private static final Struct TARGET_STATE_PAUSED = new Struct(KafkaConfigBackingStore.TARGET_STATE_V0).put("state", "PAUSED"); private static final Struct TASKS_COMMIT_STRUCT_TWO_TASK_CONNECTOR @@ -240,6 +245,9 @@ public void testSnapshotCannotMutateInternalState() throws Exception { assertNotSame(snapshot.connectorConfigs, configStorage.connectorConfigs); assertNotSame(snapshot.connectorTargetStates, configStorage.connectorTargetStates); assertNotSame(snapshot.taskConfigs, configStorage.taskConfigs); + assertNotSame(snapshot.connectorTaskCountRecords, configStorage.connectorTaskCountRecords); + assertNotSame(snapshot.connectorTaskConfigGenerations, configStorage.connectorTaskConfigGenerations); + assertNotSame(snapshot.connectorsPendingFencing, configStorage.connectorsPendingFencing); assertNotSame(snapshot.inconsistentConnectors, configStorage.inconsistent); PowerMock.verifyAll(); @@ -320,12 +328,20 @@ public void testWritePrivileges() throws Exception { expectConfigure(); expectStart(Collections.emptyList(), Collections.emptyMap()); - // Try and fail to write a connector config to the config topic without write privileges - expectConvert(KafkaConfigBackingStore.CONNECTOR_CONFIGURATION_V0, CONNECTOR_CONFIG_STRUCTS.get(0), CONFIGS_SERIALIZED.get(1)); + // Try and fail to write a task count record to the config topic without write privileges + expectConvert(KafkaConfigBackingStore.TASK_COUNT_RECORD_V0, CONNECTOR_TASK_COUNT_RECORD_STRUCTS.get(0), CONFIGS_SERIALIZED.get(0)); // Claim write privileges expectFencableProducer(); + // And write the task count record successfully + expectConvert(KafkaConfigBackingStore.TASK_COUNT_RECORD_V0, CONNECTOR_TASK_COUNT_RECORD_STRUCTS.get(0), CONFIGS_SERIALIZED.get(0)); + fencableProducer.beginTransaction(); + EasyMock.expectLastCall(); + EasyMock.expect(fencableProducer.send(EasyMock.anyObject())).andReturn(null); + fencableProducer.commitTransaction(); + EasyMock.expectLastCall(); + expectRead(CONNECTOR_TASK_COUNT_RECORD_KEYS.get(0), CONFIGS_SERIALIZED.get(0), CONNECTOR_TASK_COUNT_RECORD_STRUCTS.get(0)); - // Try again to write a connector config + // Try to write a connector config expectConvert(KafkaConfigBackingStore.CONNECTOR_CONFIGURATION_V0, CONNECTOR_CONFIG_STRUCTS.get(0), CONFIGS_SERIALIZED.get(1)); fencableProducer.beginTransaction(); EasyMock.expectLastCall(); @@ -368,9 +384,11 @@ public void testWritePrivileges() throws Exception { configStorage.start(); // Should fail the first time since we haven't claimed write privileges - assertThrows(IllegalStateException.class, () -> configStorage.putConnectorConfig(CONNECTOR_IDS.get(1), SAMPLE_CONFIGS.get(0))); - // Try to recover + assertThrows(IllegalStateException.class, () -> configStorage.putTaskCountRecord(CONNECTOR_IDS.get(0), 6)); + // Should succeed now configStorage.claimWritePrivileges(); + configStorage.putTaskCountRecord(CONNECTOR_IDS.get(0), 6); + // Should fail again when we get fenced out assertThrows(PrivilegedWriteException.class, () -> configStorage.putConnectorConfig(CONNECTOR_IDS.get(1), SAMPLE_CONFIGS.get(0))); // Should fail if we retry without reclaiming write privileges @@ -388,6 +406,84 @@ public void testWritePrivileges() throws Exception { PowerMock.verifyAll(); } + @Test + public void testTaskCountRecordsAndGenerations() throws Exception { + expectConfigure(); + expectStart(Collections.emptyList(), Collections.emptyMap()); + + // Task configs should read to end, write to the log, read to end, write root, then read to end again + expectReadToEnd(new LinkedHashMap<>()); + expectConvertWriteRead( + TASK_CONFIG_KEYS.get(0), KafkaConfigBackingStore.TASK_CONFIGURATION_V0, CONFIGS_SERIALIZED.get(0), + "properties", SAMPLE_CONFIGS.get(0)); + expectConvertWriteRead( + TASK_CONFIG_KEYS.get(1), KafkaConfigBackingStore.TASK_CONFIGURATION_V0, CONFIGS_SERIALIZED.get(1), + "properties", SAMPLE_CONFIGS.get(1)); + expectReadToEnd(new LinkedHashMap<>()); + expectConvertWriteRead( + COMMIT_TASKS_CONFIG_KEYS.get(0), KafkaConfigBackingStore.CONNECTOR_TASKS_COMMIT_V0, CONFIGS_SERIALIZED.get(2), + "tasks", 2); // Starts with 0 tasks, after update has 2 + // As soon as root is rewritten, we should see a callback notifying us that we reconfigured some tasks + configUpdateListener.onTaskConfigUpdate(Arrays.asList(TASK_IDS.get(0), TASK_IDS.get(1))); + EasyMock.expectLastCall(); + + // Records to be read by consumer as it reads to the end of the log + LinkedHashMap serializedConfigs = new LinkedHashMap<>(); + serializedConfigs.put(TASK_CONFIG_KEYS.get(0), CONFIGS_SERIALIZED.get(0)); + serializedConfigs.put(TASK_CONFIG_KEYS.get(1), CONFIGS_SERIALIZED.get(1)); + serializedConfigs.put(COMMIT_TASKS_CONFIG_KEYS.get(0), CONFIGS_SERIALIZED.get(2)); + expectReadToEnd(serializedConfigs); + + // Task count records are read back after writing as well + expectConvertWriteRead( + CONNECTOR_TASK_COUNT_RECORD_KEYS.get(0), KafkaConfigBackingStore.TASK_COUNT_RECORD_V0, CONFIGS_SERIALIZED.get(3), + "task-count", 4); + serializedConfigs = new LinkedHashMap<>(); + serializedConfigs.put(CONNECTOR_TASK_COUNT_RECORD_KEYS.get(0), CONFIGS_SERIALIZED.get(3)); + expectReadToEnd(serializedConfigs); + + expectPartitionCount(1); + expectStop(); + + PowerMock.replayAll(); + + configStorage.setupAndCreateKafkaBasedLog(TOPIC, DEFAULT_DISTRIBUTED_CONFIG); + configStorage.start(); + + // Bootstrap as if we had already added the connector, but no tasks had been added yet + whiteboxAddConnector(CONNECTOR_IDS.get(0), SAMPLE_CONFIGS.get(0), Collections.emptyList()); + + // Before anything is written + String connectorName = CONNECTOR_IDS.get(0); + ClusterConfigState configState = configStorage.snapshot(); + assertFalse(configState.pendingFencing(connectorName)); + assertNull(configState.taskCountRecord(connectorName)); + assertNull(configState.taskConfigGeneration(connectorName)); + + // Writing task configs should block until all the writes have been performed and the root record update + // has completed + List> taskConfigs = Arrays.asList(SAMPLE_CONFIGS.get(0), SAMPLE_CONFIGS.get(1)); + configStorage.putTaskConfigs("connector1", taskConfigs); + + configState = configStorage.snapshot(); + assertEquals(3, configState.offset()); + assertTrue(configState.pendingFencing(connectorName)); + assertNull(configState.taskCountRecord(connectorName)); + assertEquals(0, (long) configState.taskConfigGeneration(connectorName)); + + configStorage.putTaskCountRecord(connectorName, 4); + + configState = configStorage.snapshot(); + assertEquals(4, configState.offset()); + assertFalse(configState.pendingFencing(connectorName)); + assertEquals(4, (long) configState.taskCountRecord(connectorName)); + assertEquals(0, (long) configState.taskConfigGeneration(connectorName)); + + configStorage.stop(); + + PowerMock.verifyAll(); + } + @Test public void testPutTaskConfigs() throws Exception { expectConfigure(); @@ -805,30 +901,36 @@ public void testRestore() throws Exception { expectConfigure(); // Overwrite each type at least once to ensure we see the latest data after loading List> existingRecords = Arrays.asList( - new ConsumerRecord<>(TOPIC, 0, 0, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_CONFIG_KEYS.get(0), + new ConsumerRecord<>(TOPIC, 0, 0, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_TASK_COUNT_RECORD_KEYS.get(0), CONFIGS_SERIALIZED.get(0), new RecordHeaders(), Optional.empty()), - new ConsumerRecord<>(TOPIC, 0, 1, 0L, TimestampType.CREATE_TIME, 0, 0, TASK_CONFIG_KEYS.get(0), + new ConsumerRecord<>(TOPIC, 0, 1, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_CONFIG_KEYS.get(0), CONFIGS_SERIALIZED.get(1), new RecordHeaders(), Optional.empty()), - new ConsumerRecord<>(TOPIC, 0, 2, 0L, TimestampType.CREATE_TIME, 0, 0, TASK_CONFIG_KEYS.get(1), + new ConsumerRecord<>(TOPIC, 0, 2, 0L, TimestampType.CREATE_TIME, 0, 0, TASK_CONFIG_KEYS.get(0), CONFIGS_SERIALIZED.get(2), new RecordHeaders(), Optional.empty()), - new ConsumerRecord<>(TOPIC, 0, 3, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_CONFIG_KEYS.get(0), + new ConsumerRecord<>(TOPIC, 0, 3, 0L, TimestampType.CREATE_TIME, 0, 0, TASK_CONFIG_KEYS.get(1), CONFIGS_SERIALIZED.get(3), new RecordHeaders(), Optional.empty()), - new ConsumerRecord<>(TOPIC, 0, 4, 0L, TimestampType.CREATE_TIME, 0, 0, COMMIT_TASKS_CONFIG_KEYS.get(0), + new ConsumerRecord<>(TOPIC, 0, 4, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_CONFIG_KEYS.get(0), CONFIGS_SERIALIZED.get(4), new RecordHeaders(), Optional.empty()), - // Connector after root update should make it through, task update shouldn't - new ConsumerRecord<>(TOPIC, 0, 5, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_CONFIG_KEYS.get(0), + new ConsumerRecord<>(TOPIC, 0, 5, 0L, TimestampType.CREATE_TIME, 0, 0, COMMIT_TASKS_CONFIG_KEYS.get(0), CONFIGS_SERIALIZED.get(5), new RecordHeaders(), Optional.empty()), - new ConsumerRecord<>(TOPIC, 0, 6, 0L, TimestampType.CREATE_TIME, 0, 0, TASK_CONFIG_KEYS.get(0), - CONFIGS_SERIALIZED.get(6), new RecordHeaders(), Optional.empty())); + new ConsumerRecord<>(TOPIC, 0, 6, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_TASK_COUNT_RECORD_KEYS.get(1), + CONFIGS_SERIALIZED.get(6), new RecordHeaders(), Optional.empty()), + // Connector after root update should make it through, task update shouldn't + new ConsumerRecord<>(TOPIC, 0, 7, 0L, TimestampType.CREATE_TIME, 0, 0, CONNECTOR_CONFIG_KEYS.get(0), + CONFIGS_SERIALIZED.get(7), new RecordHeaders(), Optional.empty()), + new ConsumerRecord<>(TOPIC, 0, 8, 0L, TimestampType.CREATE_TIME, 0, 0, TASK_CONFIG_KEYS.get(0), + CONFIGS_SERIALIZED.get(8), new RecordHeaders(), Optional.empty())); LinkedHashMap deserialized = new LinkedHashMap<>(); - deserialized.put(CONFIGS_SERIALIZED.get(0), CONNECTOR_CONFIG_STRUCTS.get(0)); - deserialized.put(CONFIGS_SERIALIZED.get(1), TASK_CONFIG_STRUCTS.get(0)); + deserialized.put(CONFIGS_SERIALIZED.get(0), CONNECTOR_TASK_COUNT_RECORD_STRUCTS.get(0)); + deserialized.put(CONFIGS_SERIALIZED.get(1), CONNECTOR_CONFIG_STRUCTS.get(0)); deserialized.put(CONFIGS_SERIALIZED.get(2), TASK_CONFIG_STRUCTS.get(0)); - deserialized.put(CONFIGS_SERIALIZED.get(3), CONNECTOR_CONFIG_STRUCTS.get(1)); - deserialized.put(CONFIGS_SERIALIZED.get(4), TASKS_COMMIT_STRUCT_TWO_TASK_CONNECTOR); - deserialized.put(CONFIGS_SERIALIZED.get(5), CONNECTOR_CONFIG_STRUCTS.get(2)); - deserialized.put(CONFIGS_SERIALIZED.get(6), TASK_CONFIG_STRUCTS.get(1)); - logOffset = 7; + deserialized.put(CONFIGS_SERIALIZED.get(3), TASK_CONFIG_STRUCTS.get(0)); + deserialized.put(CONFIGS_SERIALIZED.get(4), CONNECTOR_CONFIG_STRUCTS.get(1)); + deserialized.put(CONFIGS_SERIALIZED.get(5), TASKS_COMMIT_STRUCT_TWO_TASK_CONNECTOR); + deserialized.put(CONFIGS_SERIALIZED.get(6), CONNECTOR_TASK_COUNT_RECORD_STRUCTS.get(1)); + deserialized.put(CONFIGS_SERIALIZED.get(7), CONNECTOR_CONFIG_STRUCTS.get(2)); + deserialized.put(CONFIGS_SERIALIZED.get(8), TASK_CONFIG_STRUCTS.get(1)); + logOffset = 9; expectStart(existingRecords, deserialized); expectPartitionCount(1); @@ -843,7 +945,7 @@ public void testRestore() throws Exception { // Should see a single connector and its config should be the last one seen anywhere in the log ClusterConfigState configState = configStorage.snapshot(); - assertEquals(7, configState.offset()); // Should always be next to be read, even if uncommitted + assertEquals(logOffset, configState.offset()); // Should always be next to be read, even if uncommitted assertEquals(Arrays.asList(CONNECTOR_IDS.get(0)), new ArrayList<>(configState.connectors())); assertEquals(TargetState.STARTED, configState.targetState(CONNECTOR_IDS.get(0))); // CONNECTOR_CONFIG_STRUCTS[2] -> SAMPLE_CONFIGS[2] @@ -853,7 +955,9 @@ public void testRestore() throws Exception { // Both TASK_CONFIG_STRUCTS[0] -> SAMPLE_CONFIGS[0] assertEquals(SAMPLE_CONFIGS.get(0), configState.taskConfig(TASK_IDS.get(0))); assertEquals(SAMPLE_CONFIGS.get(0), configState.taskConfig(TASK_IDS.get(1))); + assertEquals(9, (int) configState.taskCountRecord(CONNECTOR_IDS.get(1))); assertEquals(Collections.EMPTY_SET, configState.inconsistentConnectors()); + assertEquals(Collections.singleton("connector1"), configState.connectorsPendingFencing); configStorage.stop(); diff --git a/gradle/spotbugs-exclude.xml b/gradle/spotbugs-exclude.xml index 8e09cf926791b..38116642fb5ab 100644 --- a/gradle/spotbugs-exclude.xml +++ b/gradle/spotbugs-exclude.xml @@ -311,6 +311,16 @@ For a detailed description of spotbugs bug categories, see https://spotbugs.read + + + + + + +