From 45aae641a5a6632f7d1261b1880dbb8e73be1ff6 Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Thu, 24 Aug 2023 19:29:33 +0200 Subject: [PATCH 01/11] MINOR: Upgrade version of Gradle entreprise to 3.14.1 & custom user data Gradle plugin to 1.11.1 (#14131) Reviewers: Divij Vaidya --- settings.gradle | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/settings.gradle b/settings.gradle index 09cb1b44d821f..79af7b84c3084 100644 --- a/settings.gradle +++ b/settings.gradle @@ -14,8 +14,8 @@ // limitations under the License. plugins { - id 'com.gradle.enterprise' version '3.13.4' - id 'com.gradle.common-custom-user-data-gradle-plugin' version '1.11' + id 'com.gradle.enterprise' version '3.14.1' + id 'com.gradle.common-custom-user-data-gradle-plugin' version '1.11.1' } def isGithubActions = System.getenv('GITHUB_ACTIONS') != null From 8d12c1175cca5af27f79aa746c24bd998ff62345 Mon Sep 17 00:00:00 2001 From: Phuc-Hong-Tran <44060007+Phuc-Hong-Tran@users.noreply.github.com> Date: Fri, 25 Aug 2023 03:38:45 +1000 Subject: [PATCH 02/11] KAFKA-15152: Fix incorrect format specifiers when formatting string (#14026) Reviewers: Divij Vaidya Co-authored-by: phuchong.tran --- .../java/org/apache/kafka/clients/admin/KafkaAdminClient.java | 2 +- .../org/apache/kafka/common/record/MemoryRecordsBuilder.java | 2 +- .../internals/secured/HttpAccessTokenRetriever.java | 4 ++-- .../oauthbearer/internals/secured/RefreshingHttpsJwks.java | 4 ++-- .../security/oauthbearer/internals/secured/SerializedJwt.java | 2 +- .../apache/kafka/controller/ControllerResultAndOffset.java | 2 +- raft/src/main/java/org/apache/kafka/raft/Batch.java | 4 ++-- raft/src/main/java/org/apache/kafka/raft/FollowerState.java | 4 ++-- raft/src/main/java/org/apache/kafka/raft/ReplicatedLog.java | 2 +- .../java/org/apache/kafka/raft/internals/BatchBuilder.java | 2 +- .../internals/assignment/CopartitionedTopicsEnforcer.java | 4 ++-- 11 files changed, 16 insertions(+), 16 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java b/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java index 1f82499cc8189..fb1b6268c57d4 100644 --- a/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java +++ b/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java @@ -1228,7 +1228,7 @@ private void handleResponses(long now, List responses) { call.fail(now, authException); } else { call.fail(now, new DisconnectException(String.format( - "Cancelled %s request with correlation id %s due to node %s being disconnected", + "Cancelled %s request with correlation id %d due to node %s being disconnected", call.callName, correlationId, response.destination()))); } } else { diff --git a/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java b/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java index 3e9360f04ca69..479b306a1d652 100644 --- a/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java +++ b/clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java @@ -438,7 +438,7 @@ private void appendWithOffset(long offset, boolean isControlRecord, long timesta throw new IllegalArgumentException("Control records can only be appended to control batches"); if (lastOffset != null && offset <= lastOffset) - throw new IllegalArgumentException(String.format("Illegal offset %s following previous offset %s " + + throw new IllegalArgumentException(String.format("Illegal offset %d following previous offset %d " + "(Offsets must increase monotonically).", offset, lastOffset)); if (timestamp < 0 && timestamp != RecordBatch.NO_TIMESTAMP) diff --git a/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java b/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java index f0362f00f297f..6544005835b07 100644 --- a/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java +++ b/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java @@ -272,7 +272,7 @@ static String handleOutput(final HttpURLConnection con) throws IOException { errorResponseBody); if (responseBody == null || responseBody.isEmpty()) - throw new IOException(String.format("The token endpoint response was unexpectedly empty despite response code %s from %s and error message %s", + throw new IOException(String.format("The token endpoint response was unexpectedly empty despite response code %d from %s and error message %s", responseCode, con.getURL(), formatErrorMessage(errorResponseBody))); return responseBody; @@ -337,7 +337,7 @@ static String parseAccessToken(String responseBody) throws IOException { if (snippet.length() > MAX_RESPONSE_BODY_LENGTH) { int actualLength = responseBody.length(); String s = responseBody.substring(0, MAX_RESPONSE_BODY_LENGTH); - snippet = String.format("%s (trimmed to first %s characters out of %s total)", s, MAX_RESPONSE_BODY_LENGTH, actualLength); + snippet = String.format("%s (trimmed to first %d characters out of %d total)", s, MAX_RESPONSE_BODY_LENGTH, actualLength); } throw new IOException(String.format("The token endpoint response did not contain an access_token value. Response: (%s)", snippet)); diff --git a/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/RefreshingHttpsJwks.java b/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/RefreshingHttpsJwks.java index 5dc57dead3a93..590c5d8b7499e 100644 --- a/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/RefreshingHttpsJwks.java +++ b/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/RefreshingHttpsJwks.java @@ -348,8 +348,8 @@ public boolean maybeExpediteRefresh(String keyId) { // 1. Don't try to resolve the key as the large ID will sit in our cache // 2. Report the issue in the logs but include only the first N characters int actualLength = keyId.length(); - String s = keyId.substring(0, MISSING_KEY_ID_MAX_KEY_LENGTH); - String snippet = String.format("%s (trimmed to first %s characters out of %s total)", s, MISSING_KEY_ID_MAX_KEY_LENGTH, actualLength); + String trimmedKeyId = keyId.substring(0, MISSING_KEY_ID_MAX_KEY_LENGTH); + String snippet = String.format("%s (trimmed to first %d characters out of %d total)", trimmedKeyId, MISSING_KEY_ID_MAX_KEY_LENGTH, actualLength); log.warn("Key ID {} was too long to cache", snippet); return false; } else { diff --git a/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/SerializedJwt.java b/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/SerializedJwt.java index 6456e8b06c3f6..82c63111d1f2c 100644 --- a/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/SerializedJwt.java +++ b/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/SerializedJwt.java @@ -44,7 +44,7 @@ public SerializedJwt(String token) { String[] splits = token.split("\\."); if (splits.length != 3) - throw new ValidateException(String.format("Malformed JWT provided (%s); expected three sections (header, payload, and signature), but %s sections provided", + throw new ValidateException(String.format("Malformed JWT provided (%s); expected three sections (header, payload, and signature), but %d sections provided", token, splits.length)); this.token = token.trim(); diff --git a/metadata/src/main/java/org/apache/kafka/controller/ControllerResultAndOffset.java b/metadata/src/main/java/org/apache/kafka/controller/ControllerResultAndOffset.java index 1b725653d3aff..8759dff5fa54d 100644 --- a/metadata/src/main/java/org/apache/kafka/controller/ControllerResultAndOffset.java +++ b/metadata/src/main/java/org/apache/kafka/controller/ControllerResultAndOffset.java @@ -55,7 +55,7 @@ public int hashCode() { @Override public String toString() { return String.format( - "ControllerResultAndOffset(records=%s, response=%s, isAtomic=%s, offset=%s)", + "ControllerResultAndOffset(records=%s, response=%s, isAtomic=%s, offset=%d)", String.join(",", records().stream().map(ApiMessageAndVersion::toString).collect(Collectors.toList())), response(), isAtomic(), diff --git a/raft/src/main/java/org/apache/kafka/raft/Batch.java b/raft/src/main/java/org/apache/kafka/raft/Batch.java index 94c2b75c22b93..a2283e850131d 100644 --- a/raft/src/main/java/org/apache/kafka/raft/Batch.java +++ b/raft/src/main/java/org/apache/kafka/raft/Batch.java @@ -168,7 +168,7 @@ public static Batch control( if (records.isEmpty()) { throw new IllegalArgumentException( String.format( - "Control batch must contain at least one record; baseOffset = %s; epoch = %s", + "Control batch must contain at least one record; baseOffset = %d; epoch = %d", baseOffset, epoch ) @@ -205,7 +205,7 @@ public static Batch data( if (records.isEmpty()) { throw new IllegalArgumentException( String.format( - "Batch must contain at least one record; baseOffset = %s; epoch = %s", + "Batch must contain at least one record; baseOffset = %d; epoch = %d", baseOffset, epoch ) diff --git a/raft/src/main/java/org/apache/kafka/raft/FollowerState.java b/raft/src/main/java/org/apache/kafka/raft/FollowerState.java index 1eb9eb685d9d6..2dffb76d30e71 100644 --- a/raft/src/main/java/org/apache/kafka/raft/FollowerState.java +++ b/raft/src/main/java/org/apache/kafka/raft/FollowerState.java @@ -118,12 +118,12 @@ public boolean updateHighWatermark(OptionalLong newHighWatermark) { if (updatedHighWatermark < 0) { throw new IllegalArgumentException( - String.format("Illegal negative (%s) high watermark update", updatedHighWatermark) + String.format("Illegal negative (%d) high watermark update", updatedHighWatermark) ); } else if (previousHighWatermark > updatedHighWatermark) { throw new IllegalArgumentException( String.format( - "Non-monotonic update of high watermark from %s to %s", + "Non-monotonic update of high watermark from %d to %d", previousHighWatermark, updatedHighWatermark ) diff --git a/raft/src/main/java/org/apache/kafka/raft/ReplicatedLog.java b/raft/src/main/java/org/apache/kafka/raft/ReplicatedLog.java index ba21f63b73e98..b8ce9bc02c6ab 100644 --- a/raft/src/main/java/org/apache/kafka/raft/ReplicatedLog.java +++ b/raft/src/main/java/org/apache/kafka/raft/ReplicatedLog.java @@ -99,7 +99,7 @@ default ValidOffsetAndEpoch validateOffsetAndEpoch(long offset, int epoch) { */ OffsetAndEpoch latestSnapshotId = latestSnapshotId().orElseThrow(() -> new IllegalStateException( String.format( - "Log start offset (%s) is greater than zero but latest snapshot was not found", + "Log start offset (%d) is greater than zero but latest snapshot was not found", startOffset() ) )); diff --git a/raft/src/main/java/org/apache/kafka/raft/internals/BatchBuilder.java b/raft/src/main/java/org/apache/kafka/raft/internals/BatchBuilder.java index 92b63ec5526f1..9951b4387e97c 100644 --- a/raft/src/main/java/org/apache/kafka/raft/internals/BatchBuilder.java +++ b/raft/src/main/java/org/apache/kafka/raft/internals/BatchBuilder.java @@ -328,7 +328,7 @@ private int bytesNeededForRecords( if (expectedNextOffset - baseOffset >= Integer.MAX_VALUE) { throw new IllegalArgumentException( String.format( - "Adding %s records to a batch with base offset of %s and next offset of %s", + "Adding %d records to a batch with base offset of %d and next offset of %d", records.size(), baseOffset, expectedNextOffset diff --git a/streams/src/main/java/org/apache/kafka/streams/processor/internals/assignment/CopartitionedTopicsEnforcer.java b/streams/src/main/java/org/apache/kafka/streams/processor/internals/assignment/CopartitionedTopicsEnforcer.java index 8324532032cda..b3156facdbfcf 100644 --- a/streams/src/main/java/org/apache/kafka/streams/processor/internals/assignment/CopartitionedTopicsEnforcer.java +++ b/streams/src/main/java/org/apache/kafka/streams/processor/internals/assignment/CopartitionedTopicsEnforcer.java @@ -101,8 +101,8 @@ public void enforce(final Set copartitionGroup, .orElseThrow(emptyNumberOfPartitionsExceptionSupplier(config.name())); if (numberOfPartitionsOfInternalTopic != numPartitionsToUseForRepartitionTopics) { - final String msg = String.format("%sNumber of partitions [%s] of repartition topic [%s] " + - "doesn't match number of partitions [%s] of the source topic.", + final String msg = String.format("%sNumber of partitions [%d] of repartition topic [%s] " + + "doesn't match number of partitions [%d] of the source topic.", logPrefix, numberOfPartitionsOfInternalTopic, config.name(), From a3303b6112f658fd4fcd3b8ae1cdfd7c1e43db5b Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Thu, 24 Aug 2023 12:24:34 -0700 Subject: [PATCH 03/11] KAFKA-15393: Improve shutdown behavior in MM2 integration tests (#14278) Reviewers: Yash Mayya , Chris Egerton --- .../MirrorConnectorsIntegrationBaseTest.java | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java b/connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java index 4beeaaf876a74..6ea080a59a349 100644 --- a/connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java +++ b/connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java @@ -262,8 +262,6 @@ public void shutdownClusters() throws Exception { for (String x : backup.connectors()) { backup.deleteConnector(x); } - deleteAllTopics(primary.kafka()); - deleteAllTopics(backup.kafka()); } finally { shuttingDown = true; try { @@ -1049,17 +1047,6 @@ protected static void waitForTopicCreated(EmbeddedConnectCluster cluster, String } } - /* - * delete all topics of the input kafka cluster - */ - private static void deleteAllTopics(EmbeddedKafkaCluster cluster) throws Exception { - try (final Admin adminClient = cluster.createAdminClient()) { - Set topicsToBeDeleted = adminClient.listTopics().names().get(); - log.debug("Deleting topics: {} ", topicsToBeDeleted); - adminClient.deleteTopics(topicsToBeDeleted).all().get(); - } - } - /* * retrieve the config value based on the input cluster, topic and config name */ From 880288879e8200cd22951e67a50df166edcc1b33 Mon Sep 17 00:00:00 2001 From: Yash Mayya Date: Thu, 24 Aug 2023 23:44:53 +0100 Subject: [PATCH 04/11] KAFKA-15377: Don't expose externalized secret values in tasks-config API endpoint (#14244) Reviewers: Greg Harris --- .../org/apache/kafka/connect/runtime/AbstractHerder.java | 2 +- .../connect/runtime/standalone/StandaloneHerder.java | 2 +- .../runtime/distributed/DistributedHerderTest.java | 7 +++++++ .../connect/runtime/standalone/StandaloneHerderTest.java | 9 +++++++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java index 7f04914c8d12f..7bbcf0f686161 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java @@ -311,7 +311,7 @@ protected Map> buildTasksConfig(String conn Map> configs = new HashMap<>(); for (ConnectorTaskId cti : configState.tasks(connector)) { - configs.put(cti, configState.taskConfig(cti)); + configs.put(cti, configState.rawTaskConfig(cti)); } return configs; 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 c7fee9e671536..babb157f772c2 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 @@ -579,7 +579,7 @@ public int hashCode() { public void tasksConfig(String connName, Callback>> callback) { Map> tasksConfig = buildTasksConfig(connName); if (tasksConfig.isEmpty()) { - callback.onCompletion(new NotFoundException("Connector " + connName + " not found"), tasksConfig); + callback.onCompletion(new NotFoundException("Connector " + connName + " not found"), null); return; } callback.onCompletion(null, tasksConfig); 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 bec8369a3b09d..2b774133d364a 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 @@ -2091,6 +2091,8 @@ public void testAccessors() throws Exception { herder.connectorConfig(CONN1, connectorConfigCb); FutureCallback> taskConfigsCb = new FutureCallback<>(); herder.taskConfigs(CONN1, taskConfigsCb); + FutureCallback>> tasksConfigCb = new FutureCallback<>(); + herder.tasksConfig(CONN1, tasksConfigCb); herder.tick(); assertTrue(listConnectorsCb.isDone()); @@ -2107,6 +2109,11 @@ public void testAccessors() throws Exception { new TaskInfo(TASK1, TASK_CONFIG), new TaskInfo(TASK2, TASK_CONFIG)), taskConfigsCb.get()); + Map> tasksConfig = new HashMap<>(); + tasksConfig.put(TASK0, TASK_CONFIG); + tasksConfig.put(TASK1, TASK_CONFIG); + tasksConfig.put(TASK2, TASK_CONFIG); + assertEquals(tasksConfig, tasksConfigCb.get()); // Config transformation should not occur when requesting connector or task info verify(configTransformer, never()).transform(eq(CONN1), any()); 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 32c54f58035be..500ce58b8c651 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 @@ -765,6 +765,7 @@ public void testAccessors() throws Exception { Callback connectorInfoCb = PowerMock.createMock(Callback.class); Callback> connectorConfigCb = PowerMock.createMock(Callback.class); Callback> taskConfigsCb = PowerMock.createMock(Callback.class); + Callback>> tasksConfigCb = PowerMock.createMock(Callback.class); // Check accessors with empty worker listConnectorsCb.onCompletion(null, Collections.EMPTY_SET); @@ -775,6 +776,8 @@ public void testAccessors() throws Exception { EasyMock.expectLastCall(); taskConfigsCb.onCompletion(EasyMock.anyObject(), EasyMock.isNull()); EasyMock.expectLastCall(); + tasksConfigCb.onCompletion(EasyMock.anyObject(), EasyMock.isNull()); + EasyMock.expectLastCall(); // Create connector connector = PowerMock.createMock(BogusSourceConnector.class); @@ -795,6 +798,10 @@ public void testAccessors() throws Exception { taskConfigsCb.onCompletion(null, Arrays.asList(taskInfo)); EasyMock.expectLastCall(); + Map> tasksConfig = Collections.singletonMap(new ConnectorTaskId(CONNECTOR_NAME, 0), + taskConfig(SourceSink.SOURCE)); + tasksConfigCb.onCompletion(null, tasksConfig); + EasyMock.expectLastCall(); PowerMock.replayAll(); @@ -803,6 +810,7 @@ public void testAccessors() throws Exception { herder.connectorInfo(CONNECTOR_NAME, connectorInfoCb); herder.connectorConfig(CONNECTOR_NAME, connectorConfigCb); herder.taskConfigs(CONNECTOR_NAME, taskConfigsCb); + herder.tasksConfig(CONNECTOR_NAME, tasksConfigCb); herder.putConnectorConfig(CONNECTOR_NAME, connConfig, false, createCallback); Herder.Created connectorInfo = createCallback.get(1000L, TimeUnit.SECONDS); @@ -818,6 +826,7 @@ public void testAccessors() throws Exception { herder.connectorInfo(CONNECTOR_NAME, connectorInfoCb); herder.connectorConfig(CONNECTOR_NAME, connectorConfigCb); herder.taskConfigs(CONNECTOR_NAME, taskConfigsCb); + herder.tasksConfig(CONNECTOR_NAME, tasksConfigCb); PowerMock.verifyAll(); } From 9e3b1f9b9bf48603acde7f71c704af812a6dab4b Mon Sep 17 00:00:00 2001 From: Satish Duggana Date: Fri, 25 Aug 2023 05:03:38 +0530 Subject: [PATCH 05/11] MINOR Bump trunk to 3.7.0-SNAPSHOT (#14286) Reviewers: Divij Vaidya --- docs/js/templateData.js | 6 +++--- gradle.properties | 2 +- kafka-merge-pr.py | 2 +- streams/quickstart/java/pom.xml | 2 +- .../java/src/main/resources/archetype-resources/pom.xml | 2 +- streams/quickstart/pom.xml | 2 +- tests/kafkatest/__init__.py | 2 +- tests/kafkatest/version.py | 4 ++++ 8 files changed, 13 insertions(+), 9 deletions(-) diff --git a/docs/js/templateData.js b/docs/js/templateData.js index 1bb959e34602d..bfe5598ba547c 100644 --- a/docs/js/templateData.js +++ b/docs/js/templateData.js @@ -17,8 +17,8 @@ limitations under the License. // Define variables for doc templates var context={ - "version": "36", - "dotVersion": "3.6", - "fullDotVersion": "3.6.0", + "version": "37", + "dotVersion": "3.7", + "fullDotVersion": "3.7.0", "scalaVersion": "2.13" }; diff --git a/gradle.properties b/gradle.properties index 7cefc403c8c7b..a303a31419917 100644 --- a/gradle.properties +++ b/gradle.properties @@ -23,7 +23,7 @@ group=org.apache.kafka # - streams/quickstart/pom.xml # - streams/quickstart/java/src/main/resources/archetype-resources/pom.xml # - streams/quickstart/java/pom.xml -version=3.6.0-SNAPSHOT +version=3.7.0-SNAPSHOT scalaVersion=2.13.11 task=build org.gradle.jvmargs=-Xmx2g -Xss4m -XX:+UseParallelGC diff --git a/kafka-merge-pr.py b/kafka-merge-pr.py index 5368172485bbb..f868ba521abd1 100755 --- a/kafka-merge-pr.py +++ b/kafka-merge-pr.py @@ -70,7 +70,7 @@ DEV_BRANCH_NAME = "trunk" -DEFAULT_FIX_VERSION = os.environ.get("DEFAULT_FIX_VERSION", "3.6.0") +DEFAULT_FIX_VERSION = os.environ.get("DEFAULT_FIX_VERSION", "3.7.0") ORIGINAL_HEAD = "" diff --git a/streams/quickstart/java/pom.xml b/streams/quickstart/java/pom.xml index fc7436f1b3550..d4dbf26827d0c 100644 --- a/streams/quickstart/java/pom.xml +++ b/streams/quickstart/java/pom.xml @@ -26,7 +26,7 @@ org.apache.kafka streams-quickstart - 3.6.0-SNAPSHOT + 3.7.0-SNAPSHOT .. diff --git a/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml b/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml index 2a31d66c013af..74351ac786515 100644 --- a/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml +++ b/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml @@ -29,7 +29,7 @@ UTF-8 - 3.6.0-SNAPSHOT + 3.7.0-SNAPSHOT 1.7.36 diff --git a/streams/quickstart/pom.xml b/streams/quickstart/pom.xml index 9a8b617ac39e0..69b5f5fb4210c 100644 --- a/streams/quickstart/pom.xml +++ b/streams/quickstart/pom.xml @@ -22,7 +22,7 @@ org.apache.kafka streams-quickstart pom - 3.6.0-SNAPSHOT + 3.7.0-SNAPSHOT Kafka Streams :: Quickstart diff --git a/tests/kafkatest/__init__.py b/tests/kafkatest/__init__.py index a52cc8ebbd97b..9ab162964c1bb 100644 --- a/tests/kafkatest/__init__.py +++ b/tests/kafkatest/__init__.py @@ -22,4 +22,4 @@ # Instead, in development branches, the version should have a suffix of the form ".devN" # # For example, when Kafka is at version 1.0.0-SNAPSHOT, this should be something like "1.0.0.dev0" -__version__ = '3.6.0.dev0' +__version__ = '3.7.0.dev0' diff --git a/tests/kafkatest/version.py b/tests/kafkatest/version.py index 0656d767fe2bf..13ca608c787dd 100644 --- a/tests/kafkatest/version.py +++ b/tests/kafkatest/version.py @@ -256,3 +256,7 @@ def get_version(node=None): # 3.6.x versions V_3_6_0 = KafkaVersion("3.6.0") LATEST_3_6 = V_3_6_0 + +# 3.7.x versions +V_3_7_0 = KafkaVersion("3.7.0") +LATEST_3_7 = V_3_7_0 \ No newline at end of file From d4ab3ae85a35483c15f0ceb0002eb2d81ad79ad2 Mon Sep 17 00:00:00 2001 From: Satish Duggana Date: Fri, 25 Aug 2023 05:27:59 +0530 Subject: [PATCH 06/11] KAFKA-14888: Added remote log segments retention mechanism based on time and size. (#13561) This change introduces a remote log segment segment retention cleanup mechanism. RemoteLogManager runs retention cleanup activity tasks on each leader replica. It assesses factors such as overall size and retention duration, subsequently removing qualified segments from remote storage. This process also involves adjusting the log-start-offset within the UnifiedLog accordingly. It also cleans up the segments which have epochs earlier than the earliest leader epoch in the current leader. Co-authored-by: Satish Duggana Co-authored-by: Kamal Chandraprakash Reviewers: Jun Rao , Divij Vaidya , Kamal Chandraprakash , Christo Lolov , Jorge Esteban Quilcate Otoya , Alexandre Dupriez , Nikhil Ramakrishnan --- checkstyle/suppressions.xml | 4 +- .../kafka/log/remote/RemoteLogManager.java | 455 +++++++++++++++++- .../src/main/scala/kafka/log/LogManager.scala | 3 +- .../src/main/scala/kafka/log/UnifiedLog.scala | 153 ++++-- .../scala/kafka/server/BrokerServer.scala | 8 +- .../main/scala/kafka/server/KafkaServer.scala | 8 +- .../log/remote/RemoteLogManagerTest.java | 405 ++++++++++++---- .../scala/unit/kafka/log/UnifiedLogTest.scala | 18 +- .../kafka/server/ReplicaManagerTest.scala | 1 + .../internals/epoch/LeaderEpochFileCache.java | 15 +- .../storage/internals/log/LogConfig.java | 6 +- 11 files changed, 935 insertions(+), 141 deletions(-) diff --git a/checkstyle/suppressions.xml b/checkstyle/suppressions.xml index 124b028eb41c3..2ded53387107a 100644 --- a/checkstyle/suppressions.xml +++ b/checkstyle/suppressions.xml @@ -336,9 +336,9 @@ + files="(LogValidator|RemoteLogManagerConfig|RemoteLogManager).java"/> + files="(LogValidator|RemoteLogManager|RemoteIndexCache).java"/> diff --git a/core/src/main/java/kafka/log/remote/RemoteLogManager.java b/core/src/main/java/kafka/log/remote/RemoteLogManager.java index a9372a80fa0b3..f84fd2f80f7f7 100644 --- a/core/src/main/java/kafka/log/remote/RemoteLogManager.java +++ b/core/src/main/java/kafka/log/remote/RemoteLogManager.java @@ -92,10 +92,12 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.NavigableMap; import java.util.Optional; import java.util.OptionalInt; import java.util.OptionalLong; import java.util.Set; +import java.util.TreeMap; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -110,6 +112,7 @@ import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -121,7 +124,8 @@ * - initializing `RemoteStorageManager` and `RemoteLogMetadataManager` instances * - receives any leader and follower replica events and partition stop events and act on them * - also provides APIs to fetch indexes, metadata about remote log segments - * - copying log segments to remote storage + * - copying log segments to the remote storage + * - cleaning up segments that are expired based on retention size or retention time */ public class RemoteLogManager implements Closeable { @@ -132,6 +136,7 @@ public class RemoteLogManager implements Closeable { private final String logDir; private final Time time; private final Function> fetchLog; + private final BiConsumer updateRemoteLogStartOffset; private final BrokerTopicStats brokerTopicStats; private final RemoteStorageManager remoteLogStorageManager; @@ -165,6 +170,8 @@ public class RemoteLogManager implements Closeable { * @param time Time instance. * @param clusterId The cluster id. * @param fetchLog function to get UnifiedLog instance for a given topic. + * @param updateRemoteLogStartOffset function to update the log-start-offset for a given topic partition. + * @param brokerTopicStats BrokerTopicStats instance to update the respective metrics. */ public RemoteLogManager(RemoteLogManagerConfig rlmConfig, int brokerId, @@ -172,6 +179,7 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig, String clusterId, Time time, Function> fetchLog, + BiConsumer updateRemoteLogStartOffset, BrokerTopicStats brokerTopicStats) throws IOException { this.rlmConfig = rlmConfig; this.brokerId = brokerId; @@ -179,6 +187,7 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig, this.clusterId = clusterId; this.time = time; this.fetchLog = fetchLog; + this.updateRemoteLogStartOffset = updateRemoteLogStartOffset; this.brokerTopicStats = brokerTopicStats; remoteLogStorageManager = createRemoteStorageManager(); @@ -298,11 +307,6 @@ private void cacheTopicPartitionIds(TopicIdPartition topicIdPartition) { } } - // for testing - public RLMScheduledThreadPool rlmScheduledThreadPool() { - return rlmScheduledThreadPool; - } - /** * Callback to receive any leadership changes for the topic partitions assigned to this broker. If there are no * existing tasks for a given topic partition then it will assign new leader or follower task else it will convert the @@ -486,15 +490,25 @@ public Optional findOffsetByTimestamp(TopicParti throw new KafkaException("Topic id does not exist for topic partition: " + tp); } + Optional unifiedLogOptional = fetchLog.apply(tp); + if (!unifiedLogOptional.isPresent()) { + throw new KafkaException("UnifiedLog does not exist for topic partition: " + tp); + } + + UnifiedLog unifiedLog = unifiedLogOptional.get(); + // Get the respective epoch in which the starting-offset exists. OptionalInt maybeEpoch = leaderEpochCache.epochForOffset(startingOffset); + TopicIdPartition topicIdPartition = new TopicIdPartition(topicId, tp); + NavigableMap epochWithOffsets = buildFilteredLeaderEpochMap(leaderEpochCache.epochWithOffsets()); while (maybeEpoch.isPresent()) { int epoch = maybeEpoch.getAsInt(); - Iterator iterator = remoteLogMetadataManager.listRemoteLogSegments(new TopicIdPartition(topicId, tp), epoch); + Iterator iterator = remoteLogMetadataManager.listRemoteLogSegments(topicIdPartition, epoch); while (iterator.hasNext()) { RemoteLogSegmentMetadata rlsMetadata = iterator.next(); - if (rlsMetadata.maxTimestampMs() >= timestamp && rlsMetadata.endOffset() >= startingOffset) { + if (rlsMetadata.maxTimestampMs() >= timestamp && rlsMetadata.endOffset() >= startingOffset && + isRemoteSegmentWithinLeaderEpochs(rlsMetadata, unifiedLog.logEndOffset(), epochWithOffsets)) { return lookupTimestamp(rlsMetadata, timestamp, startingOffset); } } @@ -725,6 +739,8 @@ private void copyLogSegment(UnifiedLog log, LogSegment segment, long nextSegment .remoteCopyBytesRate().mark(copySegmentStartedRlsm.segmentSizeInBytes()); brokerTopicStats.allTopicsStats().remoteCopyBytesRate().mark(copySegmentStartedRlsm.segmentSizeInBytes()); copiedOffsetOption = OptionalLong.of(endOffset); + // Update the highest offset in remote storage for this partition's log so that the local log segments + // are not deleted before they are copied to remote storage. log.updateHighestOffsetInRemoteStorage(endOffset); logger.info("Copied {} to remote storage with segment-id: {}", logFileName, copySegmentFinishedRlsm.remoteLogSegmentId()); } @@ -744,14 +760,21 @@ public void run() { return; } + UnifiedLog log = unifiedLogOptional.get(); if (isLeader()) { // Copy log segments to remote storage - copyLogSegmentsToRemote(unifiedLogOptional.get()); + copyLogSegmentsToRemote(log); + // Cleanup/delete expired remote log segments + cleanupExpiredRemoteLogSegments(); + } else { + long offset = findHighestRemoteOffset(topicIdPartition, log); + // Update the highest offset in remote storage for this partition's log so that the local log segments + // are not deleted before they are copied to remote storage. + log.updateHighestOffsetInRemoteStorage(offset); } } catch (InterruptedException ex) { if (!isCancelled()) { - logger.warn("Current thread for topic-partition-id {} is interrupted, this task won't be rescheduled. " + - "Reason: {}", topicIdPartition, ex.getMessage()); + logger.warn("Current thread for topic-partition-id {} is interrupted. Reason: {}", topicIdPartition, ex.getMessage()); } } catch (Exception ex) { if (!isCancelled()) { @@ -761,11 +784,385 @@ public void run() { } } + public void handleLogStartOffsetUpdate(TopicPartition topicPartition, long remoteLogStartOffset) { + if (isLeader()) { + logger.debug("Updating {} with remoteLogStartOffset: {}", topicPartition, remoteLogStartOffset); + updateRemoteLogStartOffset.accept(topicPartition, remoteLogStartOffset); + } + } + + class RemoteLogRetentionHandler { + + private final Optional retentionSizeData; + private final Optional retentionTimeData; + + private long remainingBreachedSize; + + private OptionalLong logStartOffset = OptionalLong.empty(); + + public RemoteLogRetentionHandler(Optional retentionSizeData, Optional retentionTimeData) { + this.retentionSizeData = retentionSizeData; + this.retentionTimeData = retentionTimeData; + remainingBreachedSize = retentionSizeData.map(sizeData -> sizeData.remainingBreachedSize).orElse(0L); + } + + private boolean deleteRetentionSizeBreachedSegments(RemoteLogSegmentMetadata metadata) throws RemoteStorageException, ExecutionException, InterruptedException { + if (!retentionSizeData.isPresent()) { + return false; + } + + boolean isSegmentDeleted = deleteRemoteLogSegment(metadata, x -> { + // Assumption that segments contain size >= 0 + if (remainingBreachedSize > 0) { + long remainingBytes = remainingBreachedSize - x.segmentSizeInBytes(); + if (remainingBytes >= 0) { + remainingBreachedSize = remainingBytes; + return true; + } + } + + return false; + }); + if (isSegmentDeleted) { + logStartOffset = OptionalLong.of(metadata.endOffset() + 1); + logger.info("Deleted remote log segment {} due to retention size {} breach. Log size after deletion will be {}.", + metadata.remoteLogSegmentId(), retentionSizeData.get().retentionSize, remainingBreachedSize + retentionSizeData.get().retentionSize); + } + return isSegmentDeleted; + } + + public boolean deleteRetentionTimeBreachedSegments(RemoteLogSegmentMetadata metadata) + throws RemoteStorageException, ExecutionException, InterruptedException { + if (!retentionTimeData.isPresent()) { + return false; + } + + boolean isSegmentDeleted = deleteRemoteLogSegment(metadata, + x -> x.maxTimestampMs() <= retentionTimeData.get().cleanupUntilMs); + if (isSegmentDeleted) { + remainingBreachedSize = Math.max(0, remainingBreachedSize - metadata.segmentSizeInBytes()); + // It is fine to have logStartOffset as `metadata.endOffset() + 1` as the segment offset intervals + // are ascending with in an epoch. + logStartOffset = OptionalLong.of(metadata.endOffset() + 1); + logger.info("Deleted remote log segment {} due to retention time {}ms breach based on the largest record timestamp in the segment", + metadata.remoteLogSegmentId(), retentionTimeData.get().retentionMs); + } + return isSegmentDeleted; + } + + private boolean deleteLogStartOffsetBreachedSegments(RemoteLogSegmentMetadata metadata, long startOffset) + throws RemoteStorageException, ExecutionException, InterruptedException { + boolean isSegmentDeleted = deleteRemoteLogSegment(metadata, x -> startOffset > x.endOffset()); + if (isSegmentDeleted && retentionSizeData.isPresent()) { + remainingBreachedSize = Math.max(0, remainingBreachedSize - metadata.segmentSizeInBytes()); + logger.info("Deleted remote log segment {} due to log start offset {} breach", metadata.remoteLogSegmentId(), startOffset); + } + + return isSegmentDeleted; + } + + // It removes the segments beyond the current leader's earliest epoch. Those segments are considered as + // unreferenced because they are not part of the current leader epoch lineage. + private boolean deleteLogSegmentsDueToLeaderEpochCacheTruncation(EpochEntry earliestEpochEntry, RemoteLogSegmentMetadata metadata) throws RemoteStorageException, ExecutionException, InterruptedException { + boolean isSegmentDeleted = deleteRemoteLogSegment(metadata, x -> + x.segmentLeaderEpochs().keySet().stream().allMatch(epoch -> epoch < earliestEpochEntry.epoch)); + if (isSegmentDeleted) { + logger.info("Deleted remote log segment {} due to leader epoch cache truncation. Current earliest epoch: {}, segmentEndOffset: {} and segmentEpochs: {}", + metadata.remoteLogSegmentId(), earliestEpochEntry, metadata.endOffset(), metadata.segmentLeaderEpochs().keySet()); + } + + // No need to update the log-start-offset as these epochs/offsets are earlier to that value. + return isSegmentDeleted; + } + + private boolean deleteRemoteLogSegment(RemoteLogSegmentMetadata segmentMetadata, Predicate predicate) + throws RemoteStorageException, ExecutionException, InterruptedException { + if (predicate.test(segmentMetadata)) { + logger.info("Deleting remote log segment {}", segmentMetadata.remoteLogSegmentId()); + // Publish delete segment started event. + remoteLogMetadataManager.updateRemoteLogSegmentMetadata( + new RemoteLogSegmentMetadataUpdate(segmentMetadata.remoteLogSegmentId(), time.milliseconds(), + segmentMetadata.customMetadata(), RemoteLogSegmentState.DELETE_SEGMENT_STARTED, brokerId)).get(); + + // Delete the segment in remote storage. + remoteLogStorageManager.deleteLogSegmentData(segmentMetadata); + + // Publish delete segment finished event. + remoteLogMetadataManager.updateRemoteLogSegmentMetadata( + new RemoteLogSegmentMetadataUpdate(segmentMetadata.remoteLogSegmentId(), time.milliseconds(), + segmentMetadata.customMetadata(), RemoteLogSegmentState.DELETE_SEGMENT_FINISHED, brokerId)).get(); + logger.info("Deleted remote log segment {}", segmentMetadata.remoteLogSegmentId()); + return true; + } + + return false; + } + + } + + private void cleanupExpiredRemoteLogSegments() throws RemoteStorageException, ExecutionException, InterruptedException { + if (isCancelled() || !isLeader()) { + logger.info("Returning from remote log segments cleanup as the task state is changed"); + return; + } + + // Cleanup remote log segments and update the log start offset if applicable. + final Iterator segmentMetadataIter = remoteLogMetadataManager.listRemoteLogSegments(topicIdPartition); + if (!segmentMetadataIter.hasNext()) { + logger.debug("No remote log segments available on remote storage for partition: {}", topicIdPartition); + return; + } + + final Optional logOptional = fetchLog.apply(topicIdPartition.topicPartition()); + if (!logOptional.isPresent()) { + logger.debug("No UnifiedLog instance available for partition: {}", topicIdPartition); + return; + } + + final UnifiedLog log = logOptional.get(); + final Option leaderEpochCacheOption = log.leaderEpochCache(); + if (leaderEpochCacheOption.isEmpty()) { + logger.debug("No leader epoch cache available for partition: {}", topicIdPartition); + return; + } + + final Set epochsSet = new HashSet<>(); + // Good to have an API from RLMM to get all the remote leader epochs of all the segments of a partition + // instead of going through all the segments and building it here. + while (segmentMetadataIter.hasNext()) { + RemoteLogSegmentMetadata segmentMetadata = segmentMetadataIter.next(); + epochsSet.addAll(segmentMetadata.segmentLeaderEpochs().keySet()); + } + + // All the leader epochs in sorted order that exists in remote storage + final List remoteLeaderEpochs = new ArrayList<>(epochsSet); + Collections.sort(remoteLeaderEpochs); + + LeaderEpochFileCache leaderEpochCache = leaderEpochCacheOption.get(); + // Build the leader epoch map by filtering the epochs that do not have any records. + NavigableMap epochWithOffsets = buildFilteredLeaderEpochMap(leaderEpochCache.epochWithOffsets()); + Optional earliestEpochEntryOptional = leaderEpochCache.earliestEntry(); + + long logStartOffset = log.logStartOffset(); + long logEndOffset = log.logEndOffset(); + Optional retentionSizeData = buildRetentionSizeData(log.config().retentionSize, + log.onlyLocalLogSegmentsSize(), logEndOffset, epochWithOffsets); + Optional retentionTimeData = buildRetentionTimeData(log.config().retentionMs); + + RemoteLogRetentionHandler remoteLogRetentionHandler = new RemoteLogRetentionHandler(retentionSizeData, retentionTimeData); + Iterator epochIterator = epochWithOffsets.navigableKeySet().iterator(); + boolean isSegmentDeleted = true; + while (isSegmentDeleted && epochIterator.hasNext()) { + Integer epoch = epochIterator.next(); + Iterator segmentsIterator = remoteLogMetadataManager.listRemoteLogSegments(topicIdPartition, epoch); + while (isSegmentDeleted && segmentsIterator.hasNext()) { + if (isCancelled() || !isLeader()) { + logger.info("Returning from remote log segments cleanup for the remaining segments as the task state is changed."); + return; + } + RemoteLogSegmentMetadata metadata = segmentsIterator.next(); + + // check whether the segment contains the required epoch range with in the current leader epoch lineage. + if (isRemoteSegmentWithinLeaderEpochs(metadata, logEndOffset, epochWithOffsets)) { + isSegmentDeleted = + remoteLogRetentionHandler.deleteRetentionTimeBreachedSegments(metadata) || + remoteLogRetentionHandler.deleteRetentionSizeBreachedSegments(metadata) || + remoteLogRetentionHandler.deleteLogStartOffsetBreachedSegments(metadata, logStartOffset); + } + } + } + + // Remove the remote log segments whose segment-leader-epochs are less than the earliest-epoch known + // to the leader. This will remove the unreferenced segments in the remote storage. This is needed for + // unclean leader election scenarios as the remote storage can have epochs earlier to the current leader's + // earliest leader epoch. + if (earliestEpochEntryOptional.isPresent()) { + EpochEntry earliestEpochEntry = earliestEpochEntryOptional.get(); + Iterator epochsToClean = remoteLeaderEpochs.stream().filter(x -> x < earliestEpochEntry.epoch).iterator(); + while (epochsToClean.hasNext()) { + int epoch = epochsToClean.next(); + Iterator segmentsToBeCleaned = remoteLogMetadataManager.listRemoteLogSegments(topicIdPartition, epoch); + while (segmentsToBeCleaned.hasNext()) { + if (isCancelled() || !isLeader()) { + return; + } + // No need to update the log-start-offset even though the segment is deleted as these epochs/offsets are earlier to that value. + remoteLogRetentionHandler.deleteLogSegmentsDueToLeaderEpochCacheTruncation(earliestEpochEntry, segmentsToBeCleaned.next()); + } + } + } + + // Update log start offset with the computed value after retention cleanup is done + remoteLogRetentionHandler.logStartOffset.ifPresent(offset -> handleLogStartOffsetUpdate(topicIdPartition.topicPartition(), offset)); + } + + private Optional buildRetentionTimeData(long retentionMs) { + return retentionMs > -1 + ? Optional.of(new RetentionTimeData(retentionMs, time.milliseconds() - retentionMs)) + : Optional.empty(); + } + + private Optional buildRetentionSizeData(long retentionSize, + long onlyLocalLogSegmentsSize, + long logEndOffset, + NavigableMap epochEntries) throws RemoteStorageException { + if (retentionSize > -1) { + long remoteLogSizeBytes = 0L; + Set visitedSegmentIds = new HashSet<>(); + for (Integer epoch : epochEntries.navigableKeySet()) { + // remoteLogSize(topicIdPartition, epochEntry.epoch) may not be completely accurate as the remote + // log size may be computed for all the segments but not for segments with in the current + // partition's leader epoch lineage. Better to revisit this API. + // remoteLogSizeBytes += remoteLogMetadataManager.remoteLogSize(topicIdPartition, epochEntry.epoch); + Iterator segmentsIterator = remoteLogMetadataManager.listRemoteLogSegments(topicIdPartition, epoch); + while (segmentsIterator.hasNext()) { + RemoteLogSegmentMetadata segmentMetadata = segmentsIterator.next(); + RemoteLogSegmentId segmentId = segmentMetadata.remoteLogSegmentId(); + if (!visitedSegmentIds.contains(segmentId) && isRemoteSegmentWithinLeaderEpochs(segmentMetadata, logEndOffset, epochEntries)) { + remoteLogSizeBytes += segmentMetadata.segmentSizeInBytes(); + visitedSegmentIds.add(segmentId); + } + } + } + + // This is the total size of segments in local log that have their base-offset > local-log-start-offset + // and size of the segments in remote storage which have their end-offset < local-log-start-offset. + long totalSize = onlyLocalLogSegmentsSize + remoteLogSizeBytes; + if (totalSize > retentionSize) { + long remainingBreachedSize = totalSize - retentionSize; + RetentionSizeData retentionSizeData = new RetentionSizeData(retentionSize, remainingBreachedSize); + return Optional.of(retentionSizeData); + } + } + + return Optional.empty(); + } + public String toString() { return this.getClass().toString() + "[" + topicIdPartition + "]"; } } + /** + * Returns true if the remote segment's epoch/offsets are within the leader epoch lineage of the partition. + * The constraints here are as follows: + * - The segment's first epoch's offset should be more than or equal to the respective leader epoch's offset in the partition leader epoch lineage. + * - The segment's end offset should be less than or equal to the respective leader epoch's offset in the partition leader epoch lineage. + * - The segment's epoch lineage(epoch and offset) should be same as leader epoch lineage((epoch and offset)) except + * for the first and the last epochs in the segment. + * + * @param segmentMetadata The remote segment metadata to be validated. + * @param logEndOffset The log end offset of the partition. + * @param leaderEpochs The leader epoch lineage of the partition by filtering the epochs containing no data. + * @return true if the remote segment's epoch/offsets are within the leader epoch lineage of the partition. + */ + // Visible for testing + public static boolean isRemoteSegmentWithinLeaderEpochs(RemoteLogSegmentMetadata segmentMetadata, + long logEndOffset, + NavigableMap leaderEpochs) { + long segmentEndOffset = segmentMetadata.endOffset(); + // Filter epochs that does not have any messages/records associated with them. + NavigableMap segmentLeaderEpochs = buildFilteredLeaderEpochMap(segmentMetadata.segmentLeaderEpochs()); + // Check for out of bound epochs between segment epochs and current leader epochs. + Integer segmentFirstEpoch = segmentLeaderEpochs.firstKey(); + Integer segmentLastEpoch = segmentLeaderEpochs.lastKey(); + if (segmentFirstEpoch < leaderEpochs.firstKey() || segmentLastEpoch > leaderEpochs.lastKey()) { + LOGGER.debug("[{}] Remote segment {} is not within the partition leader epoch lineage. Remote segment epochs: {} and partition leader epochs: {}", + segmentMetadata.topicIdPartition(), segmentMetadata.remoteLogSegmentId(), segmentLeaderEpochs, leaderEpochs); + return false; + } + + for (Map.Entry entry : segmentLeaderEpochs.entrySet()) { + int epoch = entry.getKey(); + long offset = entry.getValue(); + + // If segment's epoch does not exist in the leader epoch lineage then it is not a valid segment. + if (!leaderEpochs.containsKey(epoch)) { + LOGGER.debug("[{}] Remote segment {}'s epoch {} is not within the leader epoch lineage. Remote segment epochs: {} and partition leader epochs: {}", + segmentMetadata.topicIdPartition(), segmentMetadata.remoteLogSegmentId(), epoch, segmentLeaderEpochs, leaderEpochs); + return false; + } + + // Segment's first epoch's offset should be more than or equal to the respective leader epoch's offset. + if (epoch == segmentFirstEpoch && offset < leaderEpochs.get(epoch)) { + LOGGER.debug("[{}] Remote segment {}'s first epoch {}'s offset is less than leader epoch's offset {}.", + segmentMetadata.topicIdPartition(), segmentMetadata.remoteLogSegmentId(), epoch, leaderEpochs.get(epoch)); + return false; + } + + // Segment's end offset should be less than or equal to the respective leader epoch's offset. + if (epoch == segmentLastEpoch) { + Map.Entry nextEntry = leaderEpochs.higherEntry(epoch); + if (nextEntry != null && segmentEndOffset > nextEntry.getValue() - 1) { + LOGGER.debug("[{}] Remote segment {}'s end offset {} is more than leader epoch's offset {}.", + segmentMetadata.topicIdPartition(), segmentMetadata.remoteLogSegmentId(), segmentEndOffset, nextEntry.getValue() - 1); + return false; + } + } + + // Next segment epoch entry and next leader epoch entry should be same to ensure that the segment's epoch + // is within the leader epoch lineage. + if (epoch != segmentLastEpoch && !leaderEpochs.higherEntry(epoch).equals(segmentLeaderEpochs.higherEntry(epoch))) { + LOGGER.debug("[{}] Remote segment {}'s epoch {} is not within the leader epoch lineage. Remote segment epochs: {} and partition leader epochs: {}", + segmentMetadata.topicIdPartition(), segmentMetadata.remoteLogSegmentId(), epoch, segmentLeaderEpochs, leaderEpochs); + return false; + } + + } + + // segment end offset should be with in the log end offset. + return segmentEndOffset < logEndOffset; + } + + /** + * Returns a map containing the epoch vs start-offset for the given leader epoch map by filtering the epochs that + * does not contain any messages/records associated with them. + * + * For ex: + * + * 0 - 0 + * 1 - 10 + * 2 - 20 + * 3 - 30 + * 4 - 40 + * 5 - 60 // epoch 5 does not have records or messages associated with it + * 6 - 60 + * 7 - 70 + * + * When the above leaderEpochMap is passed to this method, it returns the following map: + * + * 0 - 0 + * 1 - 10 + * 2 - 20 + * 3 - 30 + * 4 - 40 + * 6 - 60 + * 7 - 70 + * + * @param leaderEpochs The leader epoch map to be refined. + */ + // Visible for testing + public static NavigableMap buildFilteredLeaderEpochMap(NavigableMap leaderEpochs) { + List duplicatedEpochs = new ArrayList<>(); + Map.Entry previousEntry = null; + for (Map.Entry entry : leaderEpochs.entrySet()) { + if (previousEntry != null && previousEntry.getValue().equals(entry.getValue())) { + duplicatedEpochs.add(previousEntry.getKey()); + } + previousEntry = entry; + } + + if (duplicatedEpochs.isEmpty()) { + return leaderEpochs; + } + + TreeMap filteredLeaderEpochs = new TreeMap<>(leaderEpochs); + for (Integer duplicatedEpoch : duplicatedEpochs) { + filteredLeaderEpochs.remove(duplicatedEpoch); + } + return filteredLeaderEpochs; + } + public FetchDataInfo read(RemoteStorageFetchInfo remoteStorageFetchInfo) throws RemoteStorageException, IOException { int fetchMaxBytes = remoteStorageFetchInfo.fetchMaxBytes; TopicPartition tp = remoteStorageFetchInfo.topicPartition; @@ -1096,6 +1493,42 @@ public void close() { } } + // Visible for testing + public static class RetentionSizeData { + private final long retentionSize; + private final long remainingBreachedSize; + + public RetentionSizeData(long retentionSize, long remainingBreachedSize) { + if (retentionSize < 0) + throw new IllegalArgumentException("retentionSize should be non negative, but it is " + retentionSize); + + if (remainingBreachedSize <= 0) { + throw new IllegalArgumentException("remainingBreachedSize should be more than zero, but it is " + remainingBreachedSize); + } + + this.retentionSize = retentionSize; + this.remainingBreachedSize = remainingBreachedSize; + } + } + + // Visible for testing + public static class RetentionTimeData { + + private final long retentionMs; + private final long cleanupUntilMs; + + public RetentionTimeData(long retentionMs, long cleanupUntilMs) { + if (retentionMs < 0) + throw new IllegalArgumentException("retentionMs should be non negative, but it is " + retentionMs); + + if (cleanupUntilMs < 0) + throw new IllegalArgumentException("cleanupUntilMs should be non negative, but it is " + cleanupUntilMs); + + this.retentionMs = retentionMs; + this.cleanupUntilMs = cleanupUntilMs; + } + } + // Visible for testing static class EnrichedLogSegment { private final LogSegment logSegment; diff --git a/core/src/main/scala/kafka/log/LogManager.scala b/core/src/main/scala/kafka/log/LogManager.scala index bed9f0dfa03b0..78e48010e5240 100755 --- a/core/src/main/scala/kafka/log/LogManager.scala +++ b/core/src/main/scala/kafka/log/LogManager.scala @@ -430,7 +430,8 @@ class LogManager(logDirs: Seq[File], val remainingLogs = decNumRemainingLogs(numRemainingLogs, dir.getAbsolutePath) val currentNumLoaded = logsToLoad.length - remainingLogs log match { - case Some(loadedLog) => info(s"Completed load of $loadedLog with ${loadedLog.numberOfSegments} segments in ${logLoadDurationMs}ms " + + case Some(loadedLog) => info(s"Completed load of $loadedLog with ${loadedLog.numberOfSegments} segments, " + + s"local-log-start-offset ${loadedLog.localLogStartOffset()} and log-end-offset ${loadedLog.logEndOffset} in ${logLoadDurationMs}ms " + s"($currentNumLoaded/${logsToLoad.length} completed in $logDirAbsolutePath)") case None => info(s"Error while loading logs in $logDir in ${logLoadDurationMs}ms ($currentNumLoaded/${logsToLoad.length} completed in $logDirAbsolutePath)") } diff --git a/core/src/main/scala/kafka/log/UnifiedLog.scala b/core/src/main/scala/kafka/log/UnifiedLog.scala index 72e0784e37d2d..10c86183b3f06 100644 --- a/core/src/main/scala/kafka/log/UnifiedLog.scala +++ b/core/src/main/scala/kafka/log/UnifiedLog.scala @@ -147,11 +147,27 @@ class UnifiedLog(@volatile var logStartOffset: Long, def localLogStartOffset(): Long = _localLogStartOffset + // This is the offset(inclusive) until which segments are copied to the remote storage. @volatile private var highestOffsetInRemoteStorage: Long = -1L locally { + def updateLocalLogStartOffset(offset: Long): Unit = { + _localLogStartOffset = offset + + if (highWatermark < offset) { + updateHighWatermark(offset) + } + + if (this.recoveryPoint < offset) { + localLog.updateRecoveryPoint(offset) + } + } + initializePartitionMetadata() updateLogStartOffset(logStartOffset) + updateLocalLogStartOffset(math.max(logStartOffset, localLog.segments.firstSegmentBaseOffset.getOrElse(0L))) + if (!remoteLogEnabled()) + logStartOffset = localLogStartOffset() maybeIncrementFirstUnstableOffset() initializeTopicId() @@ -162,6 +178,14 @@ class UnifiedLog(@volatile var logStartOffset: Long, logOffsetsListener = listener } + def updateLogStartOffsetFromRemoteTier(remoteLogStartOffset: Long): Unit = { + if (!remoteLogEnabled()) { + error("Ignoring the call as the remote log storage is disabled") + return; + } + maybeIncrementLogStartOffset(remoteLogStartOffset, LogStartOffsetIncrementReason.SegmentDeletion) + } + def remoteLogEnabled(): Boolean = { // Remote log is enabled only for non-compact and non-internal topics remoteStorageSystemEnable && @@ -520,6 +544,7 @@ class UnifiedLog(@volatile var logStartOffset: Long, localLog.updateRecoveryPoint(offset) } } + def updateHighestOffsetInRemoteStorage(offset: Long): Unit = { if (!remoteLogEnabled()) warn(s"Unable to update the highest offset in remote storage with offset $offset since remote storage is not enabled. The existing highest offset is $highestOffsetInRemoteStorage.") @@ -957,6 +982,15 @@ class UnifiedLog(@volatile var logStartOffset: Long, } } + private def maybeIncrementLocalLogStartOffset(newLocalLogStartOffset: Long, reason: LogStartOffsetIncrementReason): Unit = { + lock synchronized { + if (newLocalLogStartOffset > localLogStartOffset()) { + _localLogStartOffset = newLocalLogStartOffset + info(s"Incremented local log start offset to ${localLogStartOffset()} due to reason $reason") + } + } + } + /** * Increment the log start offset if the provided offset is larger. * @@ -967,7 +1001,8 @@ class UnifiedLog(@volatile var logStartOffset: Long, * @throws OffsetOutOfRangeException if the log start offset is greater than the high watermark * @return true if the log start offset was updated; otherwise false */ - def maybeIncrementLogStartOffset(newLogStartOffset: Long, reason: LogStartOffsetIncrementReason): Boolean = { + def maybeIncrementLogStartOffset(newLogStartOffset: Long, + reason: LogStartOffsetIncrementReason): Boolean = { // We don't have to write the log start offset to log-start-offset-checkpoint immediately. // The deleteRecordsOffset may be lost only if all in-sync replicas of this broker are shutdown // in an unclean manner within log.flush.start.offset.checkpoint.interval.ms. The chance of this happening is low. @@ -978,11 +1013,15 @@ class UnifiedLog(@volatile var logStartOffset: Long, throw new OffsetOutOfRangeException(s"Cannot increment the log start offset to $newLogStartOffset of partition $topicPartition " + s"since it is larger than the high watermark $highWatermark") + if (remoteLogEnabled()) { + // This should be set log-start-offset is set more than the current local-log-start-offset + _localLogStartOffset = math.max(newLogStartOffset, localLogStartOffset()) + } + localLog.checkIfMemoryMappedBufferClosed() if (newLogStartOffset > logStartOffset) { updatedLogStartOffset = true updateLogStartOffset(newLogStartOffset) - _localLogStartOffset = newLogStartOffset info(s"Incremented log start offset to $newLogStartOffset due to $reason") leaderEpochCache.foreach(_.truncateFromStart(logStartOffset)) producerStateManager.onLogStartOffsetIncremented(newLogStartOffset) @@ -1293,7 +1332,7 @@ class UnifiedLog(@volatile var logStartOffset: Long, latestEpochAsOptional(leaderEpochCache))) } else { // We need to search the first segment whose largest timestamp is >= the target timestamp if there is one. - val remoteOffset = if (remoteLogEnabled()) { + if (remoteLogEnabled()) { if (remoteLogManager.isEmpty) { throw new KafkaException("RemoteLogManager is empty even though the remote log storage is enabled.") } @@ -1301,25 +1340,28 @@ class UnifiedLog(@volatile var logStartOffset: Long, throw new KafkaException("Tiered storage is supported only with versions supporting leader epochs, that means RecordVersion must be >= 2.") } - remoteLogManager.get.findOffsetByTimestamp(topicPartition, targetTimestamp, logStartOffset, leaderEpochCache.get) - } else Optional.empty() - - if (remoteOffset.isPresent) { - remoteOffset.asScala + val remoteOffset = remoteLogManager.get.findOffsetByTimestamp(topicPartition, targetTimestamp, logStartOffset, leaderEpochCache.get) + if (remoteOffset.isPresent) { + remoteOffset.asScala + } else { + // If it is not found in remote log storage, search in the local log storage from local log start offset. + searchOffsetInLocalLog(targetTimestamp, localLogStartOffset()) + } } else { - // If it is not found in remote storage, search in the local storage starting with local log start offset. - - // Cache to avoid race conditions. `toBuffer` is faster than most alternatives and provides - // constant time access while being safe to use with concurrent collections unlike `toArray`. - val segmentsCopy = logSegments.toBuffer - - val targetSeg = segmentsCopy.find(_.largestTimestamp >= targetTimestamp) - targetSeg.flatMap(_.findOffsetByTimestamp(targetTimestamp, _localLogStartOffset)) + searchOffsetInLocalLog(targetTimestamp, logStartOffset) } } } } + private def searchOffsetInLocalLog(targetTimestamp: Long, startOffset: Long): Option[TimestampAndOffset] = { + // Cache to avoid race conditions. `toBuffer` is faster than most alternatives and provides + // constant time access while being safe to use with concurrent collections unlike `toArray`. + val segmentsCopy = logSegments.toBuffer + val targetSeg = segmentsCopy.find(_.largestTimestamp >= targetTimestamp) + targetSeg.flatMap(_.findOffsetByTimestamp(targetTimestamp, startOffset)) + } + def legacyFetchOffsetsBefore(timestamp: Long, maxNumOffsets: Int): Seq[Long] = { // Cache to avoid race conditions. `toBuffer` is faster than most alternatives and provides // constant time access while being safe to use with concurrent collections unlike `toArray`. @@ -1391,7 +1433,13 @@ class UnifiedLog(@volatile var logStartOffset: Long, private def deleteOldSegments(predicate: (LogSegment, Option[LogSegment]) => Boolean, reason: SegmentDeletionReason): Int = { def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment]): Boolean = { - highWatermark >= nextSegmentOpt.map(_.baseOffset).getOrElse(localLog.logEndOffset) && + val upperBoundOffset = nextSegmentOpt.map(_.baseOffset).getOrElse(localLog.logEndOffset) + + // Check not to delete segments which are not yet copied to tiered storage if remote log is enabled. + (!remoteLogEnabled() || (upperBoundOffset > 0 && upperBoundOffset - 1 <= highestOffsetInRemoteStorage)) && + // We don't delete segments with offsets at or beyond the high watermark to ensure that the log start + // offset can never exceed it. + highWatermark >= upperBoundOffset && predicate(segment, nextSegmentOpt) } lock synchronized { @@ -1403,6 +1451,11 @@ class UnifiedLog(@volatile var logStartOffset: Long, } } + private def incrementStartOffset(startOffset: Long, reason: LogStartOffsetIncrementReason): Unit = { + if (remoteLogEnabled()) maybeIncrementLocalLogStartOffset(startOffset, reason) + else maybeIncrementLogStartOffset(startOffset, reason) + } + private def deleteSegments(deletable: Iterable[LogSegment], reason: SegmentDeletionReason): Int = { maybeHandleIOException(s"Error while deleting segments for $topicPartition in dir ${dir.getParent}") { val numToDelete = deletable.size @@ -1420,7 +1473,7 @@ class UnifiedLog(@volatile var logStartOffset: Long, // remove the segments for lookups localLog.removeAndDeleteSegments(segmentsToDelete, asyncDelete = true, reason) deleteProducerSnapshots(deletable, asyncDelete = true) - maybeIncrementLogStartOffset(localLog.segments.firstSegmentBaseOffset.get, LogStartOffsetIncrementReason.SegmentDeletion) + incrementStartOffset(localLog.segments.firstSegmentBaseOffset.get, LogStartOffsetIncrementReason.SegmentDeletion) } numToDelete } @@ -1443,19 +1496,21 @@ class UnifiedLog(@volatile var logStartOffset: Long, } private def deleteRetentionMsBreachedSegments(): Int = { - if (config.retentionMs < 0) return 0 + val retentionMs = localRetentionMs(config, remoteLogEnabled()) + if (retentionMs < 0) return 0 val startMs = time.milliseconds def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment]): Boolean = { - startMs - segment.largestTimestamp > config.retentionMs + startMs - segment.largestTimestamp > retentionMs } - deleteOldSegments(shouldDelete, RetentionMsBreach(this)) + deleteOldSegments(shouldDelete, RetentionMsBreach(this, remoteLogEnabled())) } private def deleteRetentionSizeBreachedSegments(): Int = { - if (config.retentionSize < 0 || size < config.retentionSize) return 0 - var diff = size - config.retentionSize + val retentionSize: Long = localRetentionSize(config, remoteLogEnabled()) + if (retentionSize < 0 || size < retentionSize) return 0 + var diff = size - retentionSize def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment]): Boolean = { if (diff - segment.size >= 0) { diff -= segment.size @@ -1465,15 +1520,15 @@ class UnifiedLog(@volatile var logStartOffset: Long, } } - deleteOldSegments(shouldDelete, RetentionSizeBreach(this)) + deleteOldSegments(shouldDelete, RetentionSizeBreach(this, remoteLogEnabled())) } private def deleteLogStartOffsetBreachedSegments(): Int = { def shouldDelete(segment: LogSegment, nextSegmentOpt: Option[LogSegment]): Boolean = { - nextSegmentOpt.exists(_.baseOffset <= logStartOffset) + nextSegmentOpt.exists(_.baseOffset <= (if (remoteLogEnabled()) localLogStartOffset() else logStartOffset)) } - deleteOldSegments(shouldDelete, StartOffsetBreach(this)) + deleteOldSegments(shouldDelete, StartOffsetBreach(this, remoteLogEnabled())) } def isFuture: Boolean = localLog.isFuture @@ -1483,6 +1538,11 @@ class UnifiedLog(@volatile var logStartOffset: Long, */ def size: Long = localLog.segments.sizeInBytes + /** + * The log size in bytes for all segments that are only in local log but not yet in remote log. + */ + def onlyLocalLogSegmentsSize: Long = UnifiedLog.sizeInBytes(logSegments.filter(_.baseOffset >= highestOffsetInRemoteStorage)) + /** * The offset of the next message that will be appended to the log */ @@ -2174,6 +2234,14 @@ object UnifiedLog extends Logging { } } + private[log] def localRetentionMs(config: LogConfig, remoteLogEnabled: Boolean): Long = { + if (remoteLogEnabled) config.remoteLogConfig.localRetentionMs else config.retentionMs + } + + private[log] def localRetentionSize(config: LogConfig, remoteLogEnabled: Boolean): Long = { + if (remoteLogEnabled) config.remoteLogConfig.localRetentionBytes else config.retentionSize + } + } object LogMetricNames { @@ -2187,35 +2255,48 @@ object LogMetricNames { } } -case class RetentionMsBreach(log: UnifiedLog) extends SegmentDeletionReason { +case class RetentionMsBreach(log: UnifiedLog, remoteLogEnabled: Boolean) extends SegmentDeletionReason { override def logReason(toDelete: List[LogSegment]): Unit = { - val retentionMs = log.config.retentionMs + val retentionMs = UnifiedLog.localRetentionMs(log.config, remoteLogEnabled) toDelete.foreach { segment => segment.largestRecordTimestamp match { case Some(_) => - log.info(s"Deleting segment $segment due to retention time ${retentionMs}ms breach based on the largest " + - s"record timestamp in the segment") + if (remoteLogEnabled) + log.info(s"Deleting segment $segment due to local log retention time ${retentionMs}ms breach based on the largest " + + s"record timestamp in the segment") + else + log.info(s"Deleting segment $segment due to log retention time ${retentionMs}ms breach based on the largest " + + s"record timestamp in the segment") case None => - log.info(s"Deleting segment $segment due to retention time ${retentionMs}ms breach based on the " + - s"last modified time of the segment") + if (remoteLogEnabled) + log.info(s"Deleting segment $segment due to local log retention time ${retentionMs}ms breach based on the " + + s"last modified time of the segment") + else + log.info(s"Deleting segment $segment due to log retention time ${retentionMs}ms breach based on the " + + s"last modified time of the segment") } } } } -case class RetentionSizeBreach(log: UnifiedLog) extends SegmentDeletionReason { +case class RetentionSizeBreach(log: UnifiedLog, remoteLogEnabled: Boolean) extends SegmentDeletionReason { override def logReason(toDelete: List[LogSegment]): Unit = { var size = log.size toDelete.foreach { segment => size -= segment.size - log.info(s"Deleting segment $segment due to retention size ${log.config.retentionSize} breach. Log size " + + if (remoteLogEnabled) log.info(s"Deleting segment $segment due to local log retention size ${UnifiedLog.localRetentionSize(log.config, remoteLogEnabled)} breach. " + + s"Local log size after deletion will be $size.") + else log.info(s"Deleting segment $segment due to log retention size ${log.config.retentionSize} breach. Log size " + s"after deletion will be $size.") } } } -case class StartOffsetBreach(log: UnifiedLog) extends SegmentDeletionReason { +case class StartOffsetBreach(log: UnifiedLog, remoteLogEnabled: Boolean) extends SegmentDeletionReason { override def logReason(toDelete: List[LogSegment]): Unit = { - log.info(s"Deleting segments due to log start offset ${log.logStartOffset} breach: ${toDelete.mkString(",")}") + if (remoteLogEnabled) + log.info(s"Deleting segments due to local log start offset ${log.localLogStartOffset()} breach: ${toDelete.mkString(",")}") + else + log.info(s"Deleting segments due to log start offset ${log.logStartOffset} breach: ${toDelete.mkString(",")}") } } diff --git a/core/src/main/scala/kafka/server/BrokerServer.scala b/core/src/main/scala/kafka/server/BrokerServer.scala index 80155f654339b..523959e037c85 100644 --- a/core/src/main/scala/kafka/server/BrokerServer.scala +++ b/core/src/main/scala/kafka/server/BrokerServer.scala @@ -575,7 +575,13 @@ class BrokerServer( } Some(new RemoteLogManager(config.remoteLogManagerConfig, config.brokerId, config.logDirs.head, clusterId, time, - (tp: TopicPartition) => logManager.getLog(tp).asJava, brokerTopicStats)); + (tp: TopicPartition) => logManager.getLog(tp).asJava, + (tp: TopicPartition, remoteLogStartOffset: java.lang.Long) => { + logManager.getLog(tp).foreach { log => + log.updateLogStartOffsetFromRemoteTier(remoteLogStartOffset) + } + }, + brokerTopicStats)) } else { None } diff --git a/core/src/main/scala/kafka/server/KafkaServer.scala b/core/src/main/scala/kafka/server/KafkaServer.scala index 3b2ad0893160b..bcc12ef3978d5 100755 --- a/core/src/main/scala/kafka/server/KafkaServer.scala +++ b/core/src/main/scala/kafka/server/KafkaServer.scala @@ -614,7 +614,13 @@ class KafkaServer( } Some(new RemoteLogManager(config.remoteLogManagerConfig, config.brokerId, config.logDirs.head, clusterId, time, - (tp: TopicPartition) => logManager.getLog(tp).asJava, brokerTopicStats)); + (tp: TopicPartition) => logManager.getLog(tp).asJava, + (tp: TopicPartition, remoteLogStartOffset: java.lang.Long) => { + logManager.getLog(tp).foreach { log => + log.updateLogStartOffsetFromRemoteTier(remoteLogStartOffset) + } + }, + brokerTopicStats)); } else { None } diff --git a/core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java b/core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java index f0b89aca85578..588b31786c2d7 100644 --- a/core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java +++ b/core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java @@ -73,7 +73,6 @@ import java.io.ByteArrayInputStream; import java.io.File; -import java.io.FileInputStream; import java.io.IOException; import java.nio.file.Files; import java.util.ArrayList; @@ -93,6 +92,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.function.BiConsumer; +import java.util.function.Supplier; import java.util.stream.Collectors; import static org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManagerConfig.REMOTE_LOG_METADATA_COMMON_CLIENT_PREFIX; @@ -127,39 +127,40 @@ import static org.mockito.Mockito.when; public class RemoteLogManagerTest { - Time time = new MockTime(); - int brokerId = 0; - String logDir = TestUtils.tempDirectory("kafka-").toString(); - String clusterId = "dummyId"; - String remoteLogStorageTestProp = "remote.log.storage.test"; - String remoteLogStorageTestVal = "storage.test"; - String remoteLogMetadataTestProp = "remote.log.metadata.test"; - String remoteLogMetadataTestVal = "metadata.test"; - String remoteLogMetadataCommonClientTestProp = REMOTE_LOG_METADATA_COMMON_CLIENT_PREFIX + "common.client.test"; - String remoteLogMetadataCommonClientTestVal = "common.test"; - String remoteLogMetadataProducerTestProp = REMOTE_LOG_METADATA_PRODUCER_PREFIX + "producer.test"; - String remoteLogMetadataProducerTestVal = "producer.test"; - String remoteLogMetadataConsumerTestProp = REMOTE_LOG_METADATA_CONSUMER_PREFIX + "consumer.test"; - String remoteLogMetadataConsumerTestVal = "consumer.test"; - String remoteLogMetadataTopicPartitionsNum = "1"; - - RemoteStorageManager remoteStorageManager = mock(RemoteStorageManager.class); - RemoteLogMetadataManager remoteLogMetadataManager = mock(RemoteLogMetadataManager.class); - RemoteLogManagerConfig remoteLogManagerConfig = null; - - BrokerTopicStats brokerTopicStats = null; - RemoteLogManager remoteLogManager = null; - - TopicIdPartition leaderTopicIdPartition = new TopicIdPartition(Uuid.randomUuid(), new TopicPartition("Leader", 0)); - TopicIdPartition followerTopicIdPartition = new TopicIdPartition(Uuid.randomUuid(), new TopicPartition("Follower", 0)); - Map topicIds = new HashMap<>(); - TopicPartition tp = new TopicPartition("TestTopic", 5); - EpochEntry epochEntry0 = new EpochEntry(0, 0); - EpochEntry epochEntry1 = new EpochEntry(1, 100); - EpochEntry epochEntry2 = new EpochEntry(2, 200); - List totalEpochEntries = Arrays.asList(epochEntry0, epochEntry1, epochEntry2); - LeaderEpochCheckpoint checkpoint = new LeaderEpochCheckpoint() { + private final Time time = new MockTime(); + private final int brokerId = 0; + private final String logDir = TestUtils.tempDirectory("kafka-").toString(); + private final String clusterId = "dummyId"; + private final String remoteLogStorageTestProp = "remote.log.storage.test"; + private final String remoteLogStorageTestVal = "storage.test"; + private final String remoteLogMetadataTestProp = "remote.log.metadata.test"; + private final String remoteLogMetadataTestVal = "metadata.test"; + private final String remoteLogMetadataCommonClientTestProp = REMOTE_LOG_METADATA_COMMON_CLIENT_PREFIX + "common.client.test"; + private final String remoteLogMetadataCommonClientTestVal = "common.test"; + private final String remoteLogMetadataProducerTestProp = REMOTE_LOG_METADATA_PRODUCER_PREFIX + "producer.test"; + private final String remoteLogMetadataProducerTestVal = "producer.test"; + private final String remoteLogMetadataConsumerTestProp = REMOTE_LOG_METADATA_CONSUMER_PREFIX + "consumer.test"; + private final String remoteLogMetadataConsumerTestVal = "consumer.test"; + private final String remoteLogMetadataTopicPartitionsNum = "1"; + + private final RemoteStorageManager remoteStorageManager = mock(RemoteStorageManager.class); + private final RemoteLogMetadataManager remoteLogMetadataManager = mock(RemoteLogMetadataManager.class); + private RemoteLogManagerConfig remoteLogManagerConfig = null; + + private BrokerTopicStats brokerTopicStats = null; + private RemoteLogManager remoteLogManager = null; + + private final TopicIdPartition leaderTopicIdPartition = new TopicIdPartition(Uuid.randomUuid(), new TopicPartition("Leader", 0)); + private final TopicIdPartition followerTopicIdPartition = new TopicIdPartition(Uuid.randomUuid(), new TopicPartition("Follower", 0)); + private final Map topicIds = new HashMap<>(); + private final TopicPartition tp = new TopicPartition("TestTopic", 5); + private final EpochEntry epochEntry0 = new EpochEntry(0, 0); + private final EpochEntry epochEntry1 = new EpochEntry(1, 100); + private final EpochEntry epochEntry2 = new EpochEntry(2, 200); + private final List totalEpochEntries = Arrays.asList(epochEntry0, epochEntry1, epochEntry2); + private final LeaderEpochCheckpoint checkpoint = new LeaderEpochCheckpoint() { List epochs = Collections.emptyList(); + @Override public void write(Collection epochs) { this.epochs = new ArrayList<>(epochs); @@ -171,7 +172,7 @@ public List read() { } }; - UnifiedLog mockLog = mock(UnifiedLog.class); + private final UnifiedLog mockLog = mock(UnifiedLog.class); @BeforeEach void setUp() throws Exception { @@ -183,8 +184,10 @@ void setUp() throws Exception { brokerTopicStats = new BrokerTopicStats(Optional.of(KafkaConfig.fromProps(props))); kafka.utils.TestUtils.clearYammerMetrics(); - - remoteLogManager = new RemoteLogManager(remoteLogManagerConfig, brokerId, logDir, clusterId, time, tp -> Optional.of(mockLog), brokerTopicStats) { + remoteLogManager = new RemoteLogManager(remoteLogManagerConfig, brokerId, logDir, clusterId, time, + tp -> Optional.of(mockLog), + (topicPartition, offset) -> { }, + brokerTopicStats) { public RemoteStorageManager createRemoteStorageManager() { return remoteStorageManager; } @@ -273,7 +276,15 @@ void testRemoteLogMetadataManagerWithEndpointConfigOverridden() throws IOExcepti Properties props = new Properties(); // override common security.protocol by adding "RLMM prefix" and "remote log metadata common client prefix" props.put(DEFAULT_REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX + REMOTE_LOG_METADATA_COMMON_CLIENT_PREFIX + "security.protocol", "SSL"); - try (RemoteLogManager remoteLogManager = new RemoteLogManager(createRLMConfig(props), brokerId, logDir, clusterId, time, tp -> Optional.of(mockLog), brokerTopicStats) { + try (RemoteLogManager remoteLogManager = new RemoteLogManager( + createRLMConfig(props), + brokerId, + logDir, + clusterId, + time, + tp -> Optional.of(mockLog), + (topicPartition, offset) -> { }, + brokerTopicStats) { public RemoteStorageManager createRemoteStorageManager() { return remoteStorageManager; } @@ -790,7 +801,10 @@ private void verifyLogSegmentData(LogSegmentData logSegmentData, void testGetClassLoaderAwareRemoteStorageManager() throws Exception { ClassLoaderAwareRemoteStorageManager rsmManager = mock(ClassLoaderAwareRemoteStorageManager.class); try (RemoteLogManager remoteLogManager = - new RemoteLogManager(remoteLogManagerConfig, brokerId, logDir, clusterId, time, t -> Optional.empty(), brokerTopicStats) { + new RemoteLogManager(remoteLogManagerConfig, brokerId, logDir, clusterId, time, + t -> Optional.empty(), + (topicPartition, offset) -> { }, + brokerTopicStats) { public RemoteStorageManager createRemoteStorageManager() { return rsmManager; } @@ -887,77 +901,122 @@ void testRLMTaskShouldSetLeaderEpochCorrectly() { @Test void testFindOffsetByTimestamp() throws IOException, RemoteStorageException { TopicPartition tp = leaderTopicIdPartition.topicPartition(); - RemoteLogSegmentId remoteLogSegmentId = new RemoteLogSegmentId(leaderTopicIdPartition, Uuid.randomUuid()); + + long ts = time.milliseconds(); + long startOffset = 120; + int targetLeaderEpoch = 10; + + TreeMap validSegmentEpochs = new TreeMap<>(); + validSegmentEpochs.put(targetLeaderEpoch, startOffset); + + LeaderEpochFileCache leaderEpochFileCache = new LeaderEpochFileCache(tp, checkpoint); + leaderEpochFileCache.assign(4, 99L); + leaderEpochFileCache.assign(5, 99L); + leaderEpochFileCache.assign(targetLeaderEpoch, startOffset); + leaderEpochFileCache.assign(12, 500L); + + doTestFindOffsetByTimestamp(ts, startOffset, targetLeaderEpoch, validSegmentEpochs); + + // Fetching message for timestamp `ts` will return the message with startOffset+1, and `ts+1` as there are no + // messages starting with the startOffset and with `ts`. + Optional maybeTimestampAndOffset1 = remoteLogManager.findOffsetByTimestamp(tp, ts, startOffset, leaderEpochFileCache); + assertEquals(Optional.of(new FileRecords.TimestampAndOffset(ts + 1, startOffset + 1, Optional.of(targetLeaderEpoch))), maybeTimestampAndOffset1); + + // Fetching message for `ts+2` will return the message with startOffset+2 and its timestamp value is `ts+2`. + Optional maybeTimestampAndOffset2 = remoteLogManager.findOffsetByTimestamp(tp, ts + 2, startOffset, leaderEpochFileCache); + assertEquals(Optional.of(new FileRecords.TimestampAndOffset(ts + 2, startOffset + 2, Optional.of(targetLeaderEpoch))), maybeTimestampAndOffset2); + + // Fetching message for `ts+3` will return None as there are no records with timestamp >= ts+3. + Optional maybeTimestampAndOffset3 = remoteLogManager.findOffsetByTimestamp(tp, ts + 3, startOffset, leaderEpochFileCache); + assertEquals(Optional.empty(), maybeTimestampAndOffset3); + } + + @Test + void testFindOffsetByTimestampWithInvalidEpochSegments() throws IOException, RemoteStorageException { + TopicPartition tp = leaderTopicIdPartition.topicPartition(); + long ts = time.milliseconds(); long startOffset = 120; int targetLeaderEpoch = 10; + TreeMap validSegmentEpochs = new TreeMap<>(); + validSegmentEpochs.put(targetLeaderEpoch - 1, startOffset - 1); // invalid epochs not aligning with leader epoch cache + validSegmentEpochs.put(targetLeaderEpoch, startOffset); + + LeaderEpochFileCache leaderEpochFileCache = new LeaderEpochFileCache(tp, checkpoint); + leaderEpochFileCache.assign(4, 99L); + leaderEpochFileCache.assign(5, 99L); + leaderEpochFileCache.assign(targetLeaderEpoch, startOffset); + leaderEpochFileCache.assign(12, 500L); + + doTestFindOffsetByTimestamp(ts, startOffset, targetLeaderEpoch, validSegmentEpochs); + + // Fetch offsets for this segment returns empty as the segment epochs are not with in the leader epoch cache. + Optional maybeTimestampAndOffset1 = remoteLogManager.findOffsetByTimestamp(tp, ts, startOffset, leaderEpochFileCache); + assertEquals(Optional.empty(), maybeTimestampAndOffset1); + + Optional maybeTimestampAndOffset2 = remoteLogManager.findOffsetByTimestamp(tp, ts + 2, startOffset, leaderEpochFileCache); + assertEquals(Optional.empty(), maybeTimestampAndOffset2); + + Optional maybeTimestampAndOffset3 = remoteLogManager.findOffsetByTimestamp(tp, ts + 3, startOffset, leaderEpochFileCache); + assertEquals(Optional.empty(), maybeTimestampAndOffset3); + } + + private void doTestFindOffsetByTimestamp(long ts, long startOffset, int targetLeaderEpoch, + TreeMap validSegmentEpochs) throws IOException, RemoteStorageException { + TopicPartition tp = leaderTopicIdPartition.topicPartition(); + RemoteLogSegmentId remoteLogSegmentId = new RemoteLogSegmentId(leaderTopicIdPartition, Uuid.randomUuid()); + RemoteLogSegmentMetadata segmentMetadata = mock(RemoteLogSegmentMetadata.class); when(segmentMetadata.remoteLogSegmentId()).thenReturn(remoteLogSegmentId); when(segmentMetadata.maxTimestampMs()).thenReturn(ts + 2); when(segmentMetadata.startOffset()).thenReturn(startOffset); when(segmentMetadata.endOffset()).thenReturn(startOffset + 2); + when(segmentMetadata.segmentLeaderEpochs()).thenReturn(validSegmentEpochs); File tpDir = new File(logDir, tp.toString()); Files.createDirectory(tpDir.toPath()); File txnIdxFile = new File(tpDir, "txn-index" + UnifiedLog.TxnIndexFileSuffix()); txnIdxFile.createNewFile(); when(remoteStorageManager.fetchIndex(any(RemoteLogSegmentMetadata.class), any(IndexType.class))) - .thenAnswer(ans -> { - RemoteLogSegmentMetadata metadata = ans.getArgument(0); - IndexType indexType = ans.getArgument(1); - int maxEntries = (int) (metadata.endOffset() - metadata.startOffset()); - OffsetIndex offsetIdx = new OffsetIndex(new File(tpDir, String.valueOf(metadata.startOffset()) + UnifiedLog.IndexFileSuffix()), - metadata.startOffset(), maxEntries * 8); - TimeIndex timeIdx = new TimeIndex(new File(tpDir, String.valueOf(metadata.startOffset()) + UnifiedLog.TimeIndexFileSuffix()), - metadata.startOffset(), maxEntries * 12); - switch (indexType) { - case OFFSET: - return new FileInputStream(offsetIdx.file()); - case TIMESTAMP: - return new FileInputStream(timeIdx.file()); - case TRANSACTION: - return new FileInputStream(txnIdxFile); - } - return null; - }); + .thenAnswer(ans -> { + RemoteLogSegmentMetadata metadata = ans.getArgument(0); + IndexType indexType = ans.getArgument(1); + int maxEntries = (int) (metadata.endOffset() - metadata.startOffset()); + OffsetIndex offsetIdx = new OffsetIndex(new File(tpDir, String.valueOf(metadata.startOffset()) + UnifiedLog.IndexFileSuffix()), + metadata.startOffset(), maxEntries * 8); + TimeIndex timeIdx = new TimeIndex(new File(tpDir, String.valueOf(metadata.startOffset()) + UnifiedLog.TimeIndexFileSuffix()), + metadata.startOffset(), maxEntries * 12); + switch (indexType) { + case OFFSET: + return Files.newInputStream(offsetIdx.file().toPath()); + case TIMESTAMP: + return Files.newInputStream(timeIdx.file().toPath()); + case TRANSACTION: + return Files.newInputStream(txnIdxFile.toPath()); + } + return null; + }); when(remoteLogMetadataManager.listRemoteLogSegments(eq(leaderTopicIdPartition), anyInt())) - .thenAnswer(ans -> { - int leaderEpoch = ans.getArgument(1); - if (leaderEpoch == targetLeaderEpoch) - return Collections.singleton(segmentMetadata).iterator(); - else - return Collections.emptyList().iterator(); - }); - - + .thenAnswer(ans -> { + int leaderEpoch = ans.getArgument(1); + if (leaderEpoch == targetLeaderEpoch) + return Collections.singleton(segmentMetadata).iterator(); + else + return Collections.emptyIterator(); + }); // 3 messages are added with offset, and timestamp as below // startOffset , ts-1 // startOffset+1 , ts+1 // startOffset+2 , ts+2 when(remoteStorageManager.fetchLogSegment(segmentMetadata, 0)) - .thenAnswer(a -> new ByteArrayInputStream(records(ts, startOffset, targetLeaderEpoch).buffer().array())); + .thenAnswer(a -> new ByteArrayInputStream(records(ts, startOffset, targetLeaderEpoch).buffer().array())); - LeaderEpochFileCache leaderEpochFileCache = new LeaderEpochFileCache(tp, checkpoint); - leaderEpochFileCache.assign(5, 99L); - leaderEpochFileCache.assign(targetLeaderEpoch, startOffset); - leaderEpochFileCache.assign(12, 500L); + when(mockLog.logEndOffset()).thenReturn(600L); remoteLogManager.onLeadershipChange(Collections.singleton(mockPartition(leaderTopicIdPartition)), Collections.emptySet(), topicIds); - // Fetching message for timestamp `ts` will return the message with startOffset+1, and `ts+1` as there are no - // messages starting with the startOffset and with `ts`. - Optional maybeTimestampAndOffset1 = remoteLogManager.findOffsetByTimestamp(tp, ts, startOffset, leaderEpochFileCache); - assertEquals(Optional.of(new FileRecords.TimestampAndOffset(ts + 1, startOffset + 1, Optional.of(targetLeaderEpoch))), maybeTimestampAndOffset1); - - // Fetching message for `ts+2` will return the message with startOffset+2 and its timestamp value is `ts+2`. - Optional maybeTimestampAndOffset2 = remoteLogManager.findOffsetByTimestamp(tp, ts + 2, startOffset, leaderEpochFileCache); - assertEquals(Optional.of(new FileRecords.TimestampAndOffset(ts + 2, startOffset + 2, Optional.of(targetLeaderEpoch))), maybeTimestampAndOffset2); - - // Fetching message for `ts+3` will return None as there are no records with timestamp >= ts+3. - Optional maybeTimestampAndOffset3 = remoteLogManager.findOffsetByTimestamp(tp, ts + 3, startOffset, leaderEpochFileCache); - assertEquals(Optional.empty(), maybeTimestampAndOffset3); } @Test @@ -974,7 +1033,7 @@ public void testRemoveMetricsOnClose() throws IOException { MockedConstruction mockMetricsGroupCtor = mockConstruction(KafkaMetricsGroup.class); try { RemoteLogManager remoteLogManager = new RemoteLogManager(remoteLogManagerConfig, brokerId, logDir, clusterId, - time, tp -> Optional.of(mockLog), brokerTopicStats) { + time, tp -> Optional.of(mockLog), (topicPartition, offset) -> { }, brokerTopicStats) { public RemoteStorageManager createRemoteStorageManager() { return remoteStorageManager; } @@ -1007,6 +1066,155 @@ public RemoteLogMetadataManager createRemoteLogMetadataManager() { } } + private static RemoteLogSegmentMetadata createRemoteLogSegmentMetadata(long startOffset, long endOffset, Map segmentEpochs) { + return new RemoteLogSegmentMetadata( + new RemoteLogSegmentId(new TopicIdPartition(Uuid.randomUuid(), + new TopicPartition("topic", 0)), Uuid.randomUuid()), + startOffset, endOffset, + 100000L, + 1, + 100000L, + 1000, + Optional.empty(), + RemoteLogSegmentState.COPY_SEGMENT_FINISHED, segmentEpochs); + } + + @Test + public void testBuildFilteredLeaderEpochMap() { + TreeMap leaderEpochToStartOffset = new TreeMap<>(); + leaderEpochToStartOffset.put(0, 0L); + leaderEpochToStartOffset.put(1, 0L); + leaderEpochToStartOffset.put(2, 0L); + leaderEpochToStartOffset.put(3, 30L); + leaderEpochToStartOffset.put(4, 40L); + leaderEpochToStartOffset.put(5, 60L); + leaderEpochToStartOffset.put(6, 60L); + leaderEpochToStartOffset.put(7, 70L); + leaderEpochToStartOffset.put(8, 70L); + + TreeMap expectedLeaderEpochs = new TreeMap<>(); + expectedLeaderEpochs.put(2, 0L); + expectedLeaderEpochs.put(3, 30L); + expectedLeaderEpochs.put(4, 40L); + expectedLeaderEpochs.put(6, 60L); + expectedLeaderEpochs.put(8, 70L); + + NavigableMap refinedLeaderEpochMap = RemoteLogManager.buildFilteredLeaderEpochMap(leaderEpochToStartOffset); + assertEquals(expectedLeaderEpochs, refinedLeaderEpochMap); + } + + @Test + public void testRemoteSegmentWithinLeaderEpochs() { + // Test whether a remote segment is within the leader epochs + final long logEndOffset = 90L; + + TreeMap leaderEpochToStartOffset = new TreeMap<>(); + leaderEpochToStartOffset.put(0, 0L); + leaderEpochToStartOffset.put(1, 10L); + leaderEpochToStartOffset.put(2, 20L); + leaderEpochToStartOffset.put(3, 30L); + leaderEpochToStartOffset.put(4, 40L); + leaderEpochToStartOffset.put(5, 50L); + leaderEpochToStartOffset.put(7, 70L); + + // Test whether a remote segment's epochs/offsets(multiple) are within the range of leader epochs + TreeMap segmentEpochs1 = new TreeMap<>(); + segmentEpochs1.put(1, 15L); + segmentEpochs1.put(2, 20L); + segmentEpochs1.put(3, 30L); + + assertTrue(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 15, + 35, + segmentEpochs1), logEndOffset, leaderEpochToStartOffset)); + + // Test whether a remote segment's epochs/offsets(single) are within the range of leader epochs + TreeMap segmentEpochs2 = new TreeMap<>(); + segmentEpochs2.put(1, 15L); + assertTrue(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 15, + 19, + segmentEpochs2), logEndOffset, leaderEpochToStartOffset)); + + // Test whether a remote segment's start offset is same as the offset of the respective leader epoch entry. + TreeMap segmentEpochs3 = new TreeMap<>(); + segmentEpochs3.put(0, 0L); // same as leader epoch's start offset + assertTrue(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 0, + 5, + segmentEpochs3), logEndOffset, leaderEpochToStartOffset)); + + // Test whether a remote segment's start offset is same as the offset of the respective leader epoch entry. + TreeMap segmentEpochs4 = new TreeMap<>(); + segmentEpochs4.put(7, 70L); // same as leader epoch's start offset + assertTrue(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 70, + 75, + segmentEpochs4), logEndOffset, leaderEpochToStartOffset)); + + + // Test whether a remote segment's end offset is same as the end offset of the respective leader epoch entry. + TreeMap segmentEpochs5 = new TreeMap<>(); + segmentEpochs5.put(1, 15L); + segmentEpochs5.put(2, 20L); + + assertTrue(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 15, + 29, // same as end offset for epoch 2 in leaderEpochToStartOffset + segmentEpochs5), logEndOffset, leaderEpochToStartOffset)); + + // Test whether any of the epoch's is not with in the leader epoch chain. + TreeMap segmentEpochs6 = new TreeMap(); + segmentEpochs6.put(5, 55L); + segmentEpochs6.put(6, 60L); // epoch 6 exists here but it is missing in leaderEpochToStartOffset + segmentEpochs6.put(7, 70L); + + assertFalse(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 55, + 85, + segmentEpochs6), logEndOffset, leaderEpochToStartOffset)); + + // Test whether an epoch existing in remote segment does not exist in leader epoch chain. + TreeMap segmentEpochs7 = new TreeMap<>(); + segmentEpochs7.put(1, 15L); + segmentEpochs7.put(2, 20L); // epoch 3 is missing here which exists in leaderEpochToStartOffset + segmentEpochs7.put(4, 40L); + + assertFalse(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 15, + 45, + segmentEpochs7), logEndOffset, leaderEpochToStartOffset)); + + // Test a remote segment having larger end offset than the log end offset + TreeMap segmentEpochs8 = new TreeMap<>(); + segmentEpochs8.put(1, 15L); + segmentEpochs8.put(2, 20L); + + assertFalse(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 15, + 95, // larger than log end offset + segmentEpochs8), logEndOffset, leaderEpochToStartOffset)); + + // Test whether a segment's first offset is earlier to the respective epoch's start offset + TreeMap segmentEpochs9 = new TreeMap<>(); + segmentEpochs9.put(1, 5L); + segmentEpochs9.put(2, 20L); + + assertFalse(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 5, // earlier to epoch 1's start offset + 25, + segmentEpochs9), logEndOffset, leaderEpochToStartOffset)); + + // Test whether a segment's last offset is more than the respective epoch's end offset + TreeMap segmentEpochs10 = new TreeMap<>(); + segmentEpochs10.put(1, 15L); + segmentEpochs10.put(2, 20L); + assertFalse(RemoteLogManager.isRemoteSegmentWithinLeaderEpochs(createRemoteLogSegmentMetadata( + 15, + 35, // more than epoch 2's end offset + segmentEpochs10), logEndOffset, leaderEpochToStartOffset)); + } + @Test public void testCandidateLogSegmentsSkipsActiveSegment() { UnifiedLog log = mock(UnifiedLog.class); @@ -1057,6 +1265,35 @@ public void testCandidateLogSegmentsSkipsSegmentsAfterLastStableOffset() { assertEquals(expected, actual); } + @Test + public void testRemoteSizeData() { + Supplier[] invalidRetentionSizeData = + new Supplier[]{ + () -> new RemoteLogManager.RetentionSizeData(10, 0), + () -> new RemoteLogManager.RetentionSizeData(10, -1), + () -> new RemoteLogManager.RetentionSizeData(-1, 10), + () -> new RemoteLogManager.RetentionSizeData(-1, -1), + () -> new RemoteLogManager.RetentionSizeData(-1, 0) + }; + + for (Supplier invalidRetentionSizeDataEntry : invalidRetentionSizeData) { + assertThrows(IllegalArgumentException.class, invalidRetentionSizeDataEntry::get); + } + } + + @Test + public void testRemoteSizeTime() { + Supplier[] invalidRetentionTimeData = + new Supplier[] { + () -> new RemoteLogManager.RetentionTimeData(-1, 10), + () -> new RemoteLogManager.RetentionTimeData(10, -1), + }; + + for (Supplier invalidRetentionTimeDataEntry : invalidRetentionTimeData) { + assertThrows(IllegalArgumentException.class, invalidRetentionTimeDataEntry::get); + } + } + @Test public void testStopPartitionsWithoutDeletion() throws RemoteStorageException { BiConsumer errorHandler = (topicPartition, throwable) -> fail("shouldn't be called"); diff --git a/core/src/test/scala/unit/kafka/log/UnifiedLogTest.scala b/core/src/test/scala/unit/kafka/log/UnifiedLogTest.scala index 8c98bfb927899..0104c55e4f2a2 100755 --- a/core/src/test/scala/unit/kafka/log/UnifiedLogTest.scala +++ b/core/src/test/scala/unit/kafka/log/UnifiedLogTest.scala @@ -3585,9 +3585,25 @@ class UnifiedLogTest { log.updateHighWatermark(90L) log.maybeIncrementLogStartOffset(20L, LogStartOffsetIncrementReason.SegmentDeletion) assertEquals(20, log.logStartOffset) - assertEquals(log.logStartOffset, log.localLogStartOffset()) } + @Test + def testStartOffsetsRemoteLogStorageIsEnabled(): Unit = { + val logConfig = LogTestUtils.createLogConfig(remoteLogStorageEnable = true) + val log = createLog(logDir, logConfig, remoteStorageSystemEnable = true) + + for (i <- 0 until 100) { + val records = TestUtils.singletonRecords(value = s"test$i".getBytes) + log.appendAsLeader(records, leaderEpoch = 0) + } + + log.updateHighWatermark(80L) + val newLogStartOffset = 40L; + log.maybeIncrementLogStartOffset(newLogStartOffset, LogStartOffsetIncrementReason.SegmentDeletion) + assertEquals(newLogStartOffset, log.logStartOffset) + assertEquals(log.logStartOffset, log.localLogStartOffset()) + } + private class MockLogOffsetsListener extends LogOffsetsListener { private var highWatermark: Long = -1L diff --git a/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala index 868f3c76ecec2..d9cd8cb0ab769 100644 --- a/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala +++ b/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala @@ -3631,6 +3631,7 @@ class ReplicaManagerTest { "clusterId", time, _ => Optional.of(mockLog), + (TopicPartition, Long) => {}, brokerTopicStats) val spyRLM = spy(remoteLogManager) diff --git a/storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java b/storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java index 55d7be4029af4..4cb7744957960 100644 --- a/storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java +++ b/storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java @@ -27,6 +27,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.NavigableMap; import java.util.Optional; import java.util.OptionalInt; import java.util.TreeMap; @@ -398,7 +399,6 @@ public void clear() { } } - // Visible for testing public List epochEntries() { lock.readLock().lock(); try { @@ -408,6 +408,19 @@ public List epochEntries() { } } + public NavigableMap epochWithOffsets() { + lock.readLock().lock(); + try { + NavigableMap epochWithOffsets = new TreeMap<>(); + for (EpochEntry epochEntry : epochs.values()) { + epochWithOffsets.put(epochEntry.epoch, epochEntry.startOffset); + } + return epochWithOffsets; + } finally { + lock.readLock().unlock(); + } + } + private void flushTo(LeaderEpochCheckpoint leaderEpochCheckpoint) { lock.readLock().lock(); try { diff --git a/storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java b/storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java index f4b402b8e3b9f..e177dfcfcb7d4 100644 --- a/storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java +++ b/storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java @@ -102,9 +102,9 @@ public String topicWarningMessage(String topicName) { public static class RemoteLogConfig { - private final boolean remoteStorageEnable; - private final long localRetentionMs; - private final long localRetentionBytes; + public final boolean remoteStorageEnable; + public final long localRetentionMs; + public final long localRetentionBytes; private RemoteLogConfig(LogConfig config) { this.remoteStorageEnable = config.getBoolean(TopicConfig.REMOTE_LOG_STORAGE_ENABLE_CONFIG); From c9715a3485f2dbad1accbbf88c66af67febfe489 Mon Sep 17 00:00:00 2001 From: Luke Chen Date: Fri, 25 Aug 2023 11:52:37 +0800 Subject: [PATCH 07/11] MINOR: Use "add-exports" only when jdk >= 16 in minikdc (#14232) Use "add-exports" only when jdk >= 16 in minikdc Reviewers: Greg Harris --- tests/kafkatest/services/security/minikdc.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/kafkatest/services/security/minikdc.py b/tests/kafkatest/services/security/minikdc.py index 29ae0fb51b91f..14cf2a6d1e654 100644 --- a/tests/kafkatest/services/security/minikdc.py +++ b/tests/kafkatest/services/security/minikdc.py @@ -22,6 +22,7 @@ from ducktape.services.service import Service from kafkatest.directory_layout.kafka_path import KafkaPathResolverMixin, CORE_LIBS_JAR_NAME, CORE_DEPENDANT_TEST_LIBS_JAR_NAME +from kafkatest.utils.remote_account import java_version from kafkatest.version import DEV_BRANCH @@ -107,8 +108,12 @@ def start_node(self, node): cmd = "for file in %s; do CLASSPATH=$CLASSPATH:$file; done;" % core_libs_jar cmd += " for file in %s; do CLASSPATH=$CLASSPATH:$file; done;" % core_dependant_test_libs_jar cmd += " export CLASSPATH;" - # avoids java.lang.IllegalAccessException due to usage of internal `sun.security.krb5.Config` in `MiniKdc` - cmd += " export KAFKA_OPTS=\"--add-exports java.security.jgss/sun.security.krb5=ALL-UNNAMED\";" + + jdk_version = java_version(node) + if jdk_version >= 16: + # avoids java.lang.IllegalAccessException due to usage of internal `sun.security.krb5.Config` in `MiniKdc` + cmd += " export KAFKA_OPTS=\"--add-exports java.security.jgss/sun.security.krb5=ALL-UNNAMED\";" + cmd += " %s kafka.security.minikdc.MiniKdc %s %s %s %s 1>> %s 2>> %s &" % (self.path.script("kafka-run-class.sh", node), MiniKdc.WORK_DIR, MiniKdc.PROPS_FILE, MiniKdc.KEYTAB_FILE, principals, MiniKdc.LOG_FILE, MiniKdc.LOG_FILE) self.logger.debug("Attempting to start MiniKdc on %s with command: %s" % (str(node.account), cmd)) with node.account.monitor_log(MiniKdc.LOG_FILE) as monitor: From 30de2bb5efe7d163115dd358067c98b17c3a6730 Mon Sep 17 00:00:00 2001 From: Mickael Maison Date: Fri, 25 Aug 2023 10:34:17 +0200 Subject: [PATCH 08/11] MINOR: Missing space in ProducerStateManager LogContext (#14275) Reviewers: Divij Vaidya --- .../kafka/storage/internals/log/ProducerStateManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManager.java b/storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManager.java index da847982c1cf4..d3e48ef7057b1 100644 --- a/storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManager.java +++ b/storage/src/main/java/org/apache/kafka/storage/internals/log/ProducerStateManager.java @@ -141,7 +141,7 @@ public ProducerStateManager(TopicPartition topicPartition, File logDir, int maxT this.maxTransactionTimeoutMs = maxTransactionTimeoutMs; this.producerStateManagerConfig = producerStateManagerConfig; this.time = time; - log = new LogContext("[ProducerStateManager partition=" + topicPartition + "]").logger(ProducerStateManager.class); + log = new LogContext("[ProducerStateManager partition=" + topicPartition + "] ").logger(ProducerStateManager.class); snapshots = loadSnapshots(); } From 5785796f985aa294c12e670da221d086a7fa887c Mon Sep 17 00:00:00 2001 From: Maros Orsak Date: Fri, 25 Aug 2023 11:31:21 +0200 Subject: [PATCH 09/11] MINOR: Add a few test cases to clients (#14211) Reviewers: Mickael Maison --- .../clients/producer/KafkaProducerTest.java | 26 +++++++++++++++++++ .../internals/BuiltInPartitionerTest.java | 8 ++++++ 2 files changed, 34 insertions(+) diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java index 8360bf2b9ff99..1610c208f1a9c 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java @@ -118,6 +118,7 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -493,6 +494,12 @@ public void testNoSerializerProvided() { Properties producerProps = new Properties(); producerProps.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9000"); assertThrows(ConfigException.class, () -> new KafkaProducer(producerProps)); + + final Map configs = new HashMap<>(); + configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + + // Invalid value null for configuration key.serializer: must be non-null. + assertThrows(ConfigException.class, () -> new KafkaProducer(configs)); } @Test @@ -2399,4 +2406,23 @@ public KafkaProducer newKafkaProducer() { } } + @Test + void testDeliveryTimeoutAndLingerMsConfig() { + final Map configs = new HashMap<>(); + configs.put(ProducerConfig.CLIENT_ID_CONFIG, "testDeliveryTimeoutAndLingerMsConfig"); + configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + configs.put(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG, 1000); + configs.put(ProducerConfig.LINGER_MS_CONFIG, 1000); + configs.put(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG, 1); + + // delivery.timeout.ms should be equal to or larger than linger.ms + request.timeout.ms + assertThrows(KafkaException.class, () -> new KafkaProducer<>(configs, new StringSerializer(), new StringSerializer())); + + configs.put(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG, 1000); + configs.put(ProducerConfig.LINGER_MS_CONFIG, 999); + configs.put(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG, 1); + + assertDoesNotThrow(() -> new KafkaProducer<>(configs, new StringSerializer(), new StringSerializer())); + } + } diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BuiltInPartitionerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BuiltInPartitionerTest.java index 734aedc483ad1..69546aaa6ed71 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BuiltInPartitionerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BuiltInPartitionerTest.java @@ -29,8 +29,10 @@ import java.util.concurrent.atomic.AtomicInteger; import static java.util.Arrays.asList; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; public class BuiltInPartitionerTest { @@ -195,4 +197,10 @@ public void adaptivePartitionsTest() { "Partition " + i + " was chosen " + frequencies[i] + " times"); } } + + @Test + void testStickyBatchSizeMoreThatZero() { + assertThrows(IllegalArgumentException.class, () -> new BuiltInPartitioner(logContext, TOPIC_A, 0)); + assertDoesNotThrow(() -> new BuiltInPartitioner(logContext, TOPIC_A, 1)); + } } From f2d499e25a1ab8674dda7c6e5a05a12fb8687dbd Mon Sep 17 00:00:00 2001 From: David Arthur Date: Fri, 25 Aug 2023 13:41:43 -0400 Subject: [PATCH 10/11] KAFKA-15389: Don't publish until we have replayed at least one record (#14282) When starting up a controller for the first time (i.e., with an empty log), it is possible for MetadataLoader to publish an empty MetadataImage before the activation records of the controller have been written. While this is not a bug, it could be confusing. This patch closes that gap by waiting for at least one controller record to be committed before the MetadataLoader starts publishing images. Reviewers: Colin P. McCabe --- .../image/loader/MetadataBatchLoader.java | 15 +++++- .../kafka/image/loader/MetadataLoader.java | 8 ++- .../image/loader/MetadataLoaderTest.java | 53 ++++++++++++++++++- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/metadata/src/main/java/org/apache/kafka/image/loader/MetadataBatchLoader.java b/metadata/src/main/java/org/apache/kafka/image/loader/MetadataBatchLoader.java index 33f4846ac7084..b1d22364cc058 100644 --- a/metadata/src/main/java/org/apache/kafka/image/loader/MetadataBatchLoader.java +++ b/metadata/src/main/java/org/apache/kafka/image/loader/MetadataBatchLoader.java @@ -67,6 +67,7 @@ public interface MetadataUpdater { private int numBatches; private long totalBatchElapsedNs; private TransactionState transactionState; + private boolean hasSeenRecord; public MetadataBatchLoader( LogContext logContext, @@ -78,16 +79,27 @@ public MetadataBatchLoader( this.time = time; this.faultHandler = faultHandler; this.callback = callback; + this.resetToImage(MetadataImage.EMPTY); + this.hasSeenRecord = false; + } + + /** + * @return True if this batch loader has seen at least one record. + */ + public boolean hasSeenRecord() { + return hasSeenRecord; } /** * Reset the state of this batch loader to the given image. Any un-flushed state will be - * discarded. + * discarded. This is called after applying a delta and passing it back to MetadataLoader, or + * when MetadataLoader loads a snapshot. * * @param image Metadata image to reset this batch loader's state to. */ public void resetToImage(MetadataImage image) { this.image = image; + this.hasSeenRecord = true; this.delta = new MetadataDelta.Builder().setImage(image).build(); this.transactionState = TransactionState.NO_TRANSACTION; this.lastOffset = image.provenance().lastContainedOffset(); @@ -241,6 +253,7 @@ private void replay(ApiMessageAndVersion record) { default: break; } + hasSeenRecord = true; delta.replay(record.message()); } } diff --git a/metadata/src/main/java/org/apache/kafka/image/loader/MetadataLoader.java b/metadata/src/main/java/org/apache/kafka/image/loader/MetadataLoader.java index c5ba2af0b503c..307fda8d218bd 100644 --- a/metadata/src/main/java/org/apache/kafka/image/loader/MetadataLoader.java +++ b/metadata/src/main/java/org/apache/kafka/image/loader/MetadataLoader.java @@ -212,7 +212,6 @@ private MetadataLoader( time, faultHandler, this::maybePublishMetadata); - this.batchLoader.resetToImage(this.image); this.eventQueue = new KafkaEventQueue( Time.SYSTEM, logContext, @@ -241,6 +240,11 @@ private boolean stillNeedToCatchUp(String where, long offset) { offset + ", but the high water mark is {}", where, highWaterMark.getAsLong()); return true; } + if (!batchLoader.hasSeenRecord()) { + log.info("{}: The loader is still catching up because we have not loaded a controller record as of offset " + + offset + " and high water mark is {}", where, highWaterMark.getAsLong()); + return true; + } log.info("{}: The loader finished catching up to the current high water mark of {}", where, highWaterMark.getAsLong()); catchingUp = false; @@ -387,8 +391,8 @@ public void handleLoadSnapshot(SnapshotReader reader) { image.provenance().lastContainedOffset(), NANOSECONDS.toMicros(manifest.elapsedNs())); MetadataImage image = delta.apply(manifest.provenance()); - maybePublishMetadata(delta, image, manifest); batchLoader.resetToImage(image); + maybePublishMetadata(delta, image, manifest); } catch (Throwable e) { // This is a general catch-all block where we don't expect to end up; // failure-prone operations should have individual try/catch blocks around them. diff --git a/metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java b/metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java index 199d907a44773..62d974b8b5c38 100644 --- a/metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java +++ b/metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java @@ -18,9 +18,11 @@ package org.apache.kafka.image.loader; import org.apache.kafka.common.Uuid; +import org.apache.kafka.common.config.ConfigResource; import org.apache.kafka.common.message.SnapshotHeaderRecord; import org.apache.kafka.common.metadata.AbortTransactionRecord; import org.apache.kafka.common.metadata.BeginTransactionRecord; +import org.apache.kafka.common.metadata.ConfigRecord; import org.apache.kafka.common.metadata.EndTransactionRecord; import org.apache.kafka.common.metadata.FeatureLevelRecord; import org.apache.kafka.common.metadata.PartitionRecord; @@ -48,6 +50,7 @@ import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Iterator; @@ -338,8 +341,8 @@ public void testLoadEmptySnapshot() throws Exception { setHighWaterMarkAccessor(() -> OptionalLong.of(0L)). build()) { loader.installPublishers(publishers).get(); - publishers.get(0).firstPublish.get(10, TimeUnit.SECONDS); loadEmptySnapshot(loader, 200); + publishers.get(0).firstPublish.get(10, TimeUnit.SECONDS); assertEquals(200L, loader.lastAppliedOffset()); loadEmptySnapshot(loader, 300); assertEquals(300L, loader.lastAppliedOffset()); @@ -668,6 +671,7 @@ public void testPublishTransaction(boolean abortTxn) throws Exception { .setTopicId(Uuid.fromString("dMCqhcK4T5miGH5wEX7NsQ")), (short) 0) ))); loader.waitForAllEventsToBeHandled(); + publisher.firstPublish.get(30, TimeUnit.SECONDS); assertNull(publisher.latestImage.topics().getTopic("foo"), "Topic should not be visible since we started transaction"); @@ -732,6 +736,7 @@ public void testPublishTransactionWithinBatch() throws Exception { // After MetadataLoader is fixed to handle arbitrary transactions, we would expect "foo" // to be visible at this point. + publisher.firstPublish.get(30, TimeUnit.SECONDS); assertNotNull(publisher.latestImage.topics().getTopic("foo")); } faultHandler.maybeRethrowFirstException(); @@ -758,6 +763,7 @@ public void testSnapshotDuringTransaction() throws Exception { .setTopicId(Uuid.fromString("HQSM3ccPQISrHqYK_C8GpA")), (short) 0) ))); loader.waitForAllEventsToBeHandled(); + publisher.firstPublish.get(30, TimeUnit.SECONDS); assertNull(publisher.latestImage.topics().getTopic("foo")); // loading a snapshot discards any in-flight transaction @@ -773,4 +779,49 @@ public void testSnapshotDuringTransaction() throws Exception { } faultHandler.maybeRethrowFirstException(); } + + @Test + public void testNoPublishEmptyImage() throws Exception { + MockFaultHandler faultHandler = new MockFaultHandler("testNoPublishEmptyImage"); + List capturedImages = new ArrayList<>(); + CompletableFuture firstPublish = new CompletableFuture<>(); + MetadataPublisher capturingPublisher = new MetadataPublisher() { + @Override + public String name() { + return "testNoPublishEmptyImage"; + } + + @Override + public void onMetadataUpdate(MetadataDelta delta, MetadataImage newImage, LoaderManifest manifest) { + if (!firstPublish.isDone()) { + firstPublish.complete(null); + } + capturedImages.add(newImage); + } + }; + + try (MetadataLoader loader = new MetadataLoader.Builder(). + setFaultHandler(faultHandler). + setHighWaterMarkAccessor(() -> OptionalLong.of(1)). + build()) { + loader.installPublishers(Collections.singletonList(capturingPublisher)).get(); + loader.handleCommit( + MockBatchReader.newSingleBatchReader(0, 1, Collections.singletonList( + // Any record will work here + new ApiMessageAndVersion(new ConfigRecord() + .setResourceType(ConfigResource.Type.BROKER.id()) + .setResourceName("3000") + .setName("foo") + .setValue("bar"), (short) 0) + ))); + firstPublish.get(30, TimeUnit.SECONDS); + + assertFalse(capturedImages.isEmpty()); + capturedImages.forEach(metadataImage -> { + assertFalse(metadataImage.isEmpty()); + }); + + } + faultHandler.maybeRethrowFirstException(); + } } From ff3e6842ff99a600fc02e69ebefb09eef93decb3 Mon Sep 17 00:00:00 2001 From: Abhijeet Kumar Date: Sat, 26 Aug 2023 05:52:26 +0530 Subject: [PATCH 11/11] KAFKA-15181: Wait for RemoteLogMetadataCache to initialize after assigning partitions (#14127) This PR adds the following changes to the `TopicBasedRemoteLogMetadataManager` 1. Added a guard in RemoteLogMetadataCache so that the incoming request can be served from the cache iff the corresponding user-topic-partition is initalized 2. Improve error handling in ConsumerTask thread so that is not killed when there are errors in reading the internal topic 3. ConsumerTask initialization should handle the case when there are no records to read and some other minor changes Added Unit Tests for the changes Co-authored-by: Kamal Chandraprakash Reviewers: Luke Chen , Jorge Esteban Quilcate Otoya , Christo Lolov , Satish Duggana --- .../metadata/storage/ConsumerManager.java | 32 +- .../remote/metadata/storage/ConsumerTask.java | 546 +++++++++++------- .../storage/RemoteLogMetadataCache.java | 11 + .../RemotePartitionMetadataEventHandler.java | 5 + .../storage/RemotePartitionMetadataStore.java | 19 + .../TopicBasedRemoteLogMetadataManager.java | 20 +- .../metadata/storage/ConsumerTaskTest.java | 417 +++++++++++++ ...cBasedRemoteLogMetadataManagerHarness.java | 10 +- ...adataManagerMultipleSubscriptionsTest.java | 178 ++++++ ...edRemoteLogMetadataManagerRestartTest.java | 34 +- ...opicBasedRemoteLogMetadataManagerTest.java | 10 +- 11 files changed, 990 insertions(+), 292 deletions(-) create mode 100644 storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTaskTest.java create mode 100644 storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerMultipleSubscriptionsTest.java diff --git a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerManager.java b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerManager.java index 14ec707a2ebb4..186cbb17c56f1 100644 --- a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerManager.java +++ b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerManager.java @@ -27,9 +27,7 @@ import org.slf4j.LoggerFactory; import java.io.Closeable; -import java.io.File; import java.io.IOException; -import java.nio.file.Path; import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeoutException; @@ -41,8 +39,6 @@ */ public class ConsumerManager implements Closeable { - public static final String COMMITTED_OFFSETS_FILE_NAME = "_rlmm_committed_offsets"; - private static final Logger log = LoggerFactory.getLogger(ConsumerManager.class); private static final long CONSUME_RECHECK_INTERVAL_MS = 50L; @@ -60,15 +56,13 @@ public ConsumerManager(TopicBasedRemoteLogMetadataManagerConfig rlmmConfig, //Create a task to consume messages and submit the respective events to RemotePartitionMetadataEventHandler. KafkaConsumer consumer = new KafkaConsumer<>(rlmmConfig.consumerProperties()); - Path committedOffsetsPath = new File(rlmmConfig.logDir(), COMMITTED_OFFSETS_FILE_NAME).toPath(); consumerTask = new ConsumerTask( - consumer, - rlmmConfig.remoteLogMetadataTopicName(), - remotePartitionMetadataEventHandler, - topicPartitioner, - committedOffsetsPath, - time, - 60_000L + remotePartitionMetadataEventHandler, + topicPartitioner, + consumer, + 100L, + 300_000L, + time ); consumerTaskThread = KafkaThread.nonDaemon("RLMMConsumerTask", consumerTask); } @@ -110,7 +104,7 @@ public void waitTillConsumptionCatchesUp(RecordMetadata recordMetadata, log.info("Waiting until consumer is caught up with the target partition: [{}]", partition); // If the current assignment does not have the subscription for this partition then return immediately. - if (!consumerTask.isPartitionAssigned(partition)) { + if (!consumerTask.isMetadataPartitionAssigned(partition)) { throw new KafkaException("This consumer is not assigned to the target partition " + partition + ". " + "Partitions currently assigned: " + consumerTask.metadataPartitionsAssigned()); } @@ -119,17 +113,17 @@ public void waitTillConsumptionCatchesUp(RecordMetadata recordMetadata, long startTimeMs = time.milliseconds(); while (true) { log.debug("Checking if partition [{}] is up to date with offset [{}]", partition, offset); - long receivedOffset = consumerTask.receivedOffsetForPartition(partition).orElse(-1L); - if (receivedOffset >= offset) { + long readOffset = consumerTask.readOffsetForMetadataPartition(partition).orElse(-1L); + if (readOffset >= offset) { return; } log.debug("Expected offset [{}] for partition [{}], but the committed offset: [{}], Sleeping for [{}] to retry again", - offset, partition, receivedOffset, consumeCheckIntervalMs); + offset, partition, readOffset, consumeCheckIntervalMs); if (time.milliseconds() - startTimeMs > timeoutMs) { log.warn("Expected offset for partition:[{}] is : [{}], but the committed offset: [{}] ", - partition, receivedOffset, offset); + partition, readOffset, offset); throw new TimeoutException("Timed out in catching up with the expected offset by consumer."); } @@ -158,7 +152,7 @@ public void removeAssignmentsForPartitions(Set partitions) { consumerTask.removeAssignmentsForPartitions(partitions); } - public Optional receivedOffsetForPartition(int metadataPartition) { - return consumerTask.receivedOffsetForPartition(metadataPartition); + public Optional readOffsetForPartition(int metadataPartition) { + return consumerTask.readOffsetForMetadataPartition(metadataPartition); } } diff --git a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTask.java b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTask.java index 2c95bf399a52d..b53c4ee337484 100644 --- a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTask.java +++ b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTask.java @@ -16,12 +16,13 @@ */ package org.apache.kafka.server.log.remote.metadata.storage; +import org.apache.kafka.clients.consumer.Consumer; import org.apache.kafka.clients.consumer.ConsumerRecord; import org.apache.kafka.clients.consumer.ConsumerRecords; -import org.apache.kafka.clients.consumer.KafkaConsumer; -import org.apache.kafka.common.KafkaException; import org.apache.kafka.common.TopicIdPartition; import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.errors.RetriableException; +import org.apache.kafka.common.errors.WakeupException; import org.apache.kafka.common.utils.Time; import org.apache.kafka.server.log.remote.metadata.storage.serialization.RemoteLogMetadataSerde; import org.apache.kafka.server.log.remote.storage.RemoteLogMetadata; @@ -30,8 +31,6 @@ import org.slf4j.LoggerFactory; import java.io.Closeable; -import java.io.IOException; -import java.nio.file.Path; import java.time.Duration; import java.util.Collections; import java.util.HashMap; @@ -64,302 +63,403 @@ class ConsumerTask implements Runnable, Closeable { private static final Logger log = LoggerFactory.getLogger(ConsumerTask.class); - private static final long POLL_INTERVAL_MS = 100L; - private final RemoteLogMetadataSerde serde = new RemoteLogMetadataSerde(); - private final KafkaConsumer consumer; - private final String metadataTopicName; + private final Consumer consumer; private final RemotePartitionMetadataEventHandler remotePartitionMetadataEventHandler; private final RemoteLogMetadataTopicPartitioner topicPartitioner; + // The timeout for the consumer to poll records from the remote log metadata topic. + private final long pollTimeoutMs; private final Time time; - // It indicates whether the closing process has been started or not. If it is set as true, - // consumer will stop consuming messages, and it will not allow partition assignments to be updated. - private volatile boolean closing = false; - - // It indicates whether the consumer needs to assign the partitions or not. This is set when it is - // determined that the consumer needs to be assigned with the updated partitions. - private volatile boolean assignPartitions = false; + // It indicates whether the ConsumerTask is closed or not. + private volatile boolean isClosed = false; + // It indicates whether the user topic partition assignment to the consumer has changed or not. If the assignment + // has changed, the consumer will eventually start tracking the newly assigned partitions and stop tracking the + // ones it is no longer assigned to. + // The initial value is set to true to wait for partition assignment on the first execution; otherwise thread will + // be busy without actually doing anything + private volatile boolean hasAssignmentChanged = true; // It represents a lock for any operations related to the assignedTopicPartitions. private final Object assignPartitionsLock = new Object(); // Remote log metadata topic partitions that consumer is assigned to. - private volatile Set assignedMetaPartitions = Collections.emptySet(); + private volatile Set assignedMetadataPartitions = Collections.emptySet(); // User topic partitions that this broker is a leader/follower for. - private Set assignedTopicPartitions = Collections.emptySet(); + private volatile Map assignedUserTopicIdPartitions = Collections.emptyMap(); + private volatile Set processedAssignmentOfUserTopicIdPartitions = Collections.emptySet(); - // Map of remote log metadata topic partition to consumed offsets. Received consumer records - // may or may not have been processed based on the assigned topic partitions. - private final Map partitionToConsumedOffsets = new ConcurrentHashMap<>(); + private long uninitializedAt; + private boolean isAllUserTopicPartitionsInitialized; - // Map of remote log metadata topic partition to processed offsets that were synced in committedOffsetsFile. - private Map lastSyncedPartitionToConsumedOffsets = Collections.emptyMap(); + // Map of remote log metadata topic partition to consumed offsets. + private final Map readOffsetsByMetadataPartition = new ConcurrentHashMap<>(); + private final Map readOffsetsByUserTopicPartition = new HashMap<>(); - private final long committedOffsetSyncIntervalMs; - private CommittedOffsetsFile committedOffsetsFile; - private long lastSyncedTimeMs; + private Map offsetHolderByMetadataPartition = new HashMap<>(); + private boolean hasLastOffsetsFetchFailed = false; + private long lastFailedFetchOffsetsTimestamp; + // The interval between retries to fetch the start and end offsets for the metadata partitions after a failed fetch. + private final long offsetFetchRetryIntervalMs; - public ConsumerTask(KafkaConsumer consumer, - String metadataTopicName, - RemotePartitionMetadataEventHandler remotePartitionMetadataEventHandler, + public ConsumerTask(RemotePartitionMetadataEventHandler remotePartitionMetadataEventHandler, RemoteLogMetadataTopicPartitioner topicPartitioner, - Path committedOffsetsPath, - Time time, - long committedOffsetSyncIntervalMs) { - this.consumer = Objects.requireNonNull(consumer); - this.metadataTopicName = Objects.requireNonNull(metadataTopicName); + Consumer consumer, + long pollTimeoutMs, + long offsetFetchRetryIntervalMs, + Time time) { + this.consumer = consumer; this.remotePartitionMetadataEventHandler = Objects.requireNonNull(remotePartitionMetadataEventHandler); this.topicPartitioner = Objects.requireNonNull(topicPartitioner); + this.pollTimeoutMs = pollTimeoutMs; + this.offsetFetchRetryIntervalMs = offsetFetchRetryIntervalMs; this.time = Objects.requireNonNull(time); - this.committedOffsetSyncIntervalMs = committedOffsetSyncIntervalMs; - - initializeConsumerAssignment(committedOffsetsPath); - } - - private void initializeConsumerAssignment(Path committedOffsetsPath) { - try { - committedOffsetsFile = new CommittedOffsetsFile(committedOffsetsPath.toFile()); - } catch (IOException e) { - throw new KafkaException(e); - } - - Map committedOffsets = Collections.emptyMap(); - try { - // Load committed offset and assign them in the consumer. - committedOffsets = committedOffsetsFile.readEntries(); - } catch (IOException e) { - // Ignore the error and consumer consumes from the earliest offset. - log.error("Encountered error while building committed offsets from the file. " + - "Consumer will consume from the earliest offset for the assigned partitions.", e); - } - - if (!committedOffsets.isEmpty()) { - // Assign topic partitions from the earlier committed offsets file. - Set earlierAssignedPartitions = committedOffsets.keySet(); - assignedMetaPartitions = Collections.unmodifiableSet(earlierAssignedPartitions); - Set metadataTopicPartitions = earlierAssignedPartitions.stream() - .map(x -> new TopicPartition(REMOTE_LOG_METADATA_TOPIC_NAME, x)) - .collect(Collectors.toSet()); - consumer.assign(metadataTopicPartitions); - - // Seek to the committed offsets - for (Map.Entry entry : committedOffsets.entrySet()) { - log.debug("Updating consumed offset: [{}] for partition [{}]", entry.getValue(), entry.getKey()); - partitionToConsumedOffsets.put(entry.getKey(), entry.getValue()); - consumer.seek(new TopicPartition(REMOTE_LOG_METADATA_TOPIC_NAME, entry.getKey()), entry.getValue()); - } - - lastSyncedPartitionToConsumedOffsets = Collections.unmodifiableMap(committedOffsets); - } + this.uninitializedAt = time.milliseconds(); } @Override public void run() { - log.info("Started Consumer task thread."); - lastSyncedTimeMs = time.milliseconds(); - try { - while (!closing) { - maybeWaitForPartitionsAssignment(); + log.info("Starting consumer task thread."); + while (!isClosed) { + try { + if (hasAssignmentChanged) { + maybeWaitForPartitionAssignments(); + } log.trace("Polling consumer to receive remote log metadata topic records"); - ConsumerRecords consumerRecords = consumer.poll(Duration.ofMillis(POLL_INTERVAL_MS)); + final ConsumerRecords consumerRecords = consumer.poll(Duration.ofMillis(pollTimeoutMs)); for (ConsumerRecord record : consumerRecords) { processConsumerRecord(record); } - - maybeSyncCommittedDataAndOffsets(false); + maybeMarkUserPartitionsAsReady(); + } catch (final WakeupException ex) { + // ignore logging the error + isClosed = true; + } catch (final RetriableException ex) { + log.warn("Retriable error occurred while processing the records. Retrying...", ex); + } catch (final Exception ex) { + isClosed = true; + log.error("Error occurred while processing the records", ex); } - } catch (Exception e) { - log.error("Error occurred in consumer task, close:[{}]", closing, e); - } finally { - maybeSyncCommittedDataAndOffsets(true); - closeConsumer(); - log.info("Exiting from consumer task thread"); } + try { + consumer.close(Duration.ofSeconds(30)); + } catch (final Exception e) { + log.error("Error encountered while closing the consumer", e); + } + log.info("Exited from consumer task thread"); } private void processConsumerRecord(ConsumerRecord record) { - // Taking assignPartitionsLock here as updateAssignmentsForPartitions changes assignedTopicPartitions - // and also calls remotePartitionMetadataEventHandler.clearTopicPartition(removedPartition) for the removed - // partitions. - RemoteLogMetadata remoteLogMetadata = serde.deserialize(record.value()); - synchronized (assignPartitionsLock) { - if (assignedTopicPartitions.contains(remoteLogMetadata.topicIdPartition())) { - remotePartitionMetadataEventHandler.handleRemoteLogMetadata(remoteLogMetadata); - } else { - log.debug("This event {} is skipped as the topic partition is not assigned for this instance.", remoteLogMetadata); - } - log.debug("Updating consumed offset: [{}] for partition [{}]", record.offset(), record.partition()); - partitionToConsumedOffsets.put(record.partition(), record.offset()); + final RemoteLogMetadata remoteLogMetadata = serde.deserialize(record.value()); + if (shouldProcess(remoteLogMetadata, record.offset())) { + remotePartitionMetadataEventHandler.handleRemoteLogMetadata(remoteLogMetadata); + readOffsetsByUserTopicPartition.put(remoteLogMetadata.topicIdPartition(), record.offset()); + } else { + log.debug("The event {} is skipped because it is either already processed or not assigned to this consumer", remoteLogMetadata); } + log.debug("Updating consumed offset: [{}] for partition [{}]", record.offset(), record.partition()); + readOffsetsByMetadataPartition.put(record.partition(), record.offset()); } - private void maybeSyncCommittedDataAndOffsets(boolean forceSync) { - // Return immediately if there is no consumption from last time. - boolean noConsumedOffsetUpdates = partitionToConsumedOffsets.equals(lastSyncedPartitionToConsumedOffsets); - if (noConsumedOffsetUpdates || !forceSync && time.milliseconds() - lastSyncedTimeMs < committedOffsetSyncIntervalMs) { - log.debug("Skip syncing committed offsets, noConsumedOffsetUpdates: {}, forceSync: {}", noConsumedOffsetUpdates, forceSync); + private boolean shouldProcess(final RemoteLogMetadata metadata, final long recordOffset) { + final TopicIdPartition tpId = metadata.topicIdPartition(); + final Long readOffset = readOffsetsByUserTopicPartition.get(tpId); + return processedAssignmentOfUserTopicIdPartitions.contains(tpId) && (readOffset == null || readOffset < recordOffset); + } + + private void maybeMarkUserPartitionsAsReady() { + if (isAllUserTopicPartitionsInitialized) { return; } - - try { - // Need to take lock on assignPartitionsLock as assignedTopicPartitions might - // get updated by other threads. - synchronized (assignPartitionsLock) { - for (TopicIdPartition topicIdPartition : assignedTopicPartitions) { - int metadataPartition = topicPartitioner.metadataPartition(topicIdPartition); - Long offset = partitionToConsumedOffsets.get(metadataPartition); - if (offset != null) { - remotePartitionMetadataEventHandler.syncLogMetadataSnapshot(topicIdPartition, metadataPartition, offset); + maybeFetchStartAndEndOffsets(); + boolean isAllInitialized = true; + for (final UserTopicIdPartition utp : assignedUserTopicIdPartitions.values()) { + if (utp.isAssigned && !utp.isInitialized) { + final Integer metadataPartition = utp.metadataPartition; + final StartAndEndOffsetHolder holder = offsetHolderByMetadataPartition.get(toRemoteLogPartition(metadataPartition)); + // The offset-holder can be null, when the recent assignment wasn't picked up by the consumer. + if (holder != null) { + final Long readOffset = readOffsetsByMetadataPartition.getOrDefault(metadataPartition, -1L); + // 1) The end-offset was fetched only once during reassignment. The metadata-partition can receive + // new stream of records, so the consumer can read records more than the last-fetched end-offset. + // 2) When the internal topic becomes empty due to breach by size/time/start-offset, then there + // are no records to read. + if (readOffset + 1 >= holder.endOffset || holder.endOffset.equals(holder.startOffset)) { + markInitialized(utp); } else { - log.debug("Skipping sync-up of the remote-log-metadata-file for partition: [{}] , with remote log metadata partition{}, and no offset", - topicIdPartition, metadataPartition); + log.debug("The user-topic-partition {} could not be marked initialized since the read-offset is {} " + + "but the end-offset is {} for the metadata-partition {}", utp, readOffset, holder.endOffset, + metadataPartition); } + } else { + log.debug("The offset-holder is null for the metadata-partition {}. The consumer may not have picked" + + " up the recent assignment", metadataPartition); } - - // Write partitionToConsumedOffsets into committed offsets file as we do not want to process them again - // in case of restarts. - committedOffsetsFile.writeEntries(partitionToConsumedOffsets); - lastSyncedPartitionToConsumedOffsets = new HashMap<>(partitionToConsumedOffsets); } - - lastSyncedTimeMs = time.milliseconds(); - } catch (IOException e) { - throw new KafkaException("Error encountered while writing committed offsets to a local file", e); + isAllInitialized = isAllInitialized && utp.isAssigned && utp.isInitialized; } - } - - private void closeConsumer() { - log.info("Closing the consumer instance"); - try { - consumer.close(Duration.ofSeconds(30)); - } catch (Exception e) { - log.error("Error encountered while closing the consumer", e); + if (isAllInitialized) { + log.info("Initialized for all the {} assigned user-partitions mapped to the {} meta-partitions in {} ms", + assignedUserTopicIdPartitions.size(), assignedMetadataPartitions.size(), + time.milliseconds() - uninitializedAt); } + isAllUserTopicPartitionsInitialized = isAllInitialized; } - private void maybeWaitForPartitionsAssignment() { - Set assignedMetaPartitionsSnapshot = Collections.emptySet(); + void maybeWaitForPartitionAssignments() throws InterruptedException { + // Snapshots of the metadata-partition and user-topic-partition are used to reduce the scope of the + // synchronization block. + // 1) LEADER_AND_ISR and STOP_REPLICA requests adds / removes the user-topic-partitions from the request + // handler threads. Those threads should not be blocked for a long time, therefore scope of the + // synchronization block is reduced to bare minimum. + // 2) Note that the consumer#position, consumer#seekToBeginning, consumer#seekToEnd and the other consumer APIs + // response times are un-predictable. Those should not be kept in the synchronization block. + final Set metadataPartitionSnapshot = new HashSet<>(); + final Set assignedUserTopicIdPartitionsSnapshot = new HashSet<>(); synchronized (assignPartitionsLock) { - // If it is closing, return immediately. This should be inside the assignPartitionsLock as the closing is updated - // in close() method with in the same lock to avoid any race conditions. - if (closing) { - return; + while (!isClosed && assignedUserTopicIdPartitions.isEmpty()) { + log.debug("Waiting for remote log metadata partitions to be assigned"); + assignPartitionsLock.wait(); } - - while (assignedMetaPartitions.isEmpty()) { - // If no partitions are assigned, wait until they are assigned. - log.debug("Waiting for assigned remote log metadata partitions.."); - try { - // No timeout is set here, as it is always notified. Even when it is closed, the race can happen - // between the thread calling this method and the thread calling close(). We should have a check - // for closing as that might have been set and notified with assignPartitionsLock by `close` - // method. - assignPartitionsLock.wait(); - - if (closing) { - return; - } - } catch (InterruptedException e) { - throw new KafkaException(e); - } - } - - if (assignPartitions) { - assignedMetaPartitionsSnapshot = new HashSet<>(assignedMetaPartitions); - // Removing unassigned meta partitions from partitionToConsumedOffsets and partitionToCommittedOffsets - partitionToConsumedOffsets.entrySet().removeIf(entry -> !assignedMetaPartitions.contains(entry.getKey())); - - assignPartitions = false; + if (!isClosed && hasAssignmentChanged) { + assignedUserTopicIdPartitions.values().forEach(utp -> { + metadataPartitionSnapshot.add(utp.metadataPartition); + assignedUserTopicIdPartitionsSnapshot.add(utp); + }); + hasAssignmentChanged = false; } } - - if (!assignedMetaPartitionsSnapshot.isEmpty()) { - executeReassignment(assignedMetaPartitionsSnapshot); + if (!metadataPartitionSnapshot.isEmpty()) { + final Set remoteLogPartitions = toRemoteLogPartitions(metadataPartitionSnapshot); + consumer.assign(remoteLogPartitions); + this.assignedMetadataPartitions = Collections.unmodifiableSet(metadataPartitionSnapshot); + // for newly assigned user-partitions, read from the beginning of the corresponding metadata partition + final Set seekToBeginOffsetPartitions = assignedUserTopicIdPartitionsSnapshot + .stream() + .filter(utp -> !utp.isAssigned) + .map(utp -> toRemoteLogPartition(utp.metadataPartition)) + .collect(Collectors.toSet()); + consumer.seekToBeginning(seekToBeginOffsetPartitions); + // for other metadata partitions, read from the offset where the processing left last time. + remoteLogPartitions.stream() + .filter(tp -> !seekToBeginOffsetPartitions.contains(tp) && + readOffsetsByMetadataPartition.containsKey(tp.partition())) + .forEach(tp -> consumer.seek(tp, readOffsetsByMetadataPartition.get(tp.partition()))); + Set processedAssignmentPartitions = new HashSet<>(); + // mark all the user-topic-partitions as assigned to the consumer. + assignedUserTopicIdPartitionsSnapshot.forEach(utp -> { + if (!utp.isAssigned) { + // Note that there can be a race between `remove` and `add` partition assignment. Calling the + // `maybeLoadPartition` here again to be sure that the partition gets loaded on the handler. + remotePartitionMetadataEventHandler.maybeLoadPartition(utp.topicIdPartition); + utp.isAssigned = true; + } + processedAssignmentPartitions.add(utp.topicIdPartition); + }); + processedAssignmentOfUserTopicIdPartitions = new HashSet<>(processedAssignmentPartitions); + clearResourcesForUnassignedUserTopicPartitions(processedAssignmentPartitions); + isAllUserTopicPartitionsInitialized = false; + uninitializedAt = time.milliseconds(); + fetchStartAndEndOffsets(); } } - private void executeReassignment(Set assignedMetaPartitionsSnapshot) { - Set assignedMetaTopicPartitions = - assignedMetaPartitionsSnapshot.stream() - .map(partitionNum -> new TopicPartition(REMOTE_LOG_METADATA_TOPIC_NAME, partitionNum)) - .collect(Collectors.toSet()); - log.info("Reassigning partitions to consumer task [{}]", assignedMetaTopicPartitions); - consumer.assign(assignedMetaTopicPartitions); + private void clearResourcesForUnassignedUserTopicPartitions(Set assignedPartitions) { + // Note that there can be previously assigned user-topic-partitions where no records are there to read + // (eg) none of the segments for a partition were uploaded. Those partition resources won't be cleared. + // It can be fixed later when required since they are empty resources. + Set unassignedPartitions = readOffsetsByUserTopicPartition.keySet() + .stream() + .filter(e -> !assignedPartitions.contains(e)) + .collect(Collectors.toSet()); + unassignedPartitions.forEach(unassignedPartition -> { + remotePartitionMetadataEventHandler.clearTopicPartition(unassignedPartition); + readOffsetsByUserTopicPartition.remove(unassignedPartition); + }); + log.info("Unassigned user-topic-partitions: {}", unassignedPartitions.size()); + } + + public void addAssignmentsForPartitions(final Set partitions) { + updateAssignments(Objects.requireNonNull(partitions), Collections.emptySet()); + } + + public void removeAssignmentsForPartitions(final Set partitions) { + updateAssignments(Collections.emptySet(), Objects.requireNonNull(partitions)); } - public void addAssignmentsForPartitions(Set partitions) { - updateAssignmentsForPartitions(partitions, Collections.emptySet()); + private void updateAssignments(final Set addedPartitions, + final Set removedPartitions) { + log.info("Updating assignments for partitions added: {} and removed: {}", addedPartitions, removedPartitions); + if (!addedPartitions.isEmpty() || !removedPartitions.isEmpty()) { + synchronized (assignPartitionsLock) { + // Make a copy of the existing assignments and update the copy. + final Map updatedUserPartitions = new HashMap<>(assignedUserTopicIdPartitions); + addedPartitions.forEach(tpId -> updatedUserPartitions.putIfAbsent(tpId, newUserTopicIdPartition(tpId))); + removedPartitions.forEach(updatedUserPartitions::remove); + if (!updatedUserPartitions.equals(assignedUserTopicIdPartitions)) { + assignedUserTopicIdPartitions = Collections.unmodifiableMap(updatedUserPartitions); + hasAssignmentChanged = true; + log.debug("Assigned user-topic-partitions: {}", assignedUserTopicIdPartitions); + assignPartitionsLock.notifyAll(); + } + } + } } - public void removeAssignmentsForPartitions(Set partitions) { - updateAssignmentsForPartitions(Collections.emptySet(), partitions); + public Optional readOffsetForMetadataPartition(final int partition) { + return Optional.ofNullable(readOffsetsByMetadataPartition.get(partition)); } - private void updateAssignmentsForPartitions(Set addedPartitions, - Set removedPartitions) { - log.info("Updating assignments for addedPartitions: {} and removedPartition: {}", addedPartitions, removedPartitions); + public boolean isMetadataPartitionAssigned(final int partition) { + return assignedMetadataPartitions.contains(partition); + } - Objects.requireNonNull(addedPartitions, "addedPartitions must not be null"); - Objects.requireNonNull(removedPartitions, "removedPartitions must not be null"); + public boolean isUserPartitionAssigned(final TopicIdPartition partition) { + final UserTopicIdPartition utp = assignedUserTopicIdPartitions.get(partition); + return utp != null && utp.isAssigned; + } - if (addedPartitions.isEmpty() && removedPartitions.isEmpty()) { - return; + @Override + public void close() { + if (!isClosed) { + log.info("Closing the instance"); + synchronized (assignPartitionsLock) { + isClosed = true; + assignedUserTopicIdPartitions.values().forEach(this::markInitialized); + consumer.wakeup(); + assignPartitionsLock.notifyAll(); + } } + } - synchronized (assignPartitionsLock) { - Set updatedReassignedPartitions = new HashSet<>(assignedTopicPartitions); - updatedReassignedPartitions.addAll(addedPartitions); - updatedReassignedPartitions.removeAll(removedPartitions); - Set updatedAssignedMetaPartitions = new HashSet<>(); - for (TopicIdPartition tp : updatedReassignedPartitions) { - updatedAssignedMetaPartitions.add(topicPartitioner.metadataPartition(tp)); - } + public Set metadataPartitionsAssigned() { + return Collections.unmodifiableSet(assignedMetadataPartitions); + } + + private void fetchStartAndEndOffsets() { + try { + final Set uninitializedPartitions = assignedUserTopicIdPartitions.values().stream() + .filter(utp -> utp.isAssigned && !utp.isInitialized) + .map(utp -> toRemoteLogPartition(utp.metadataPartition)) + .collect(Collectors.toSet()); + // Removing the previous offset holder if it exists. During reassignment, if the list-offset + // call to `earliest` and `latest` offset fails, then we should not use the previous values. + uninitializedPartitions.forEach(tp -> offsetHolderByMetadataPartition.remove(tp)); + if (!uninitializedPartitions.isEmpty()) { + Map endOffsets = consumer.endOffsets(uninitializedPartitions); + Map startOffsets = consumer.beginningOffsets(uninitializedPartitions); + offsetHolderByMetadataPartition = endOffsets.entrySet() + .stream() + .collect(Collectors.toMap(Map.Entry::getKey, + e -> new StartAndEndOffsetHolder(startOffsets.get(e.getKey()), e.getValue()))); - // Clear removed topic partitions from in-memory cache. - for (TopicIdPartition removedPartition : removedPartitions) { - remotePartitionMetadataEventHandler.clearTopicPartition(removedPartition); } + hasLastOffsetsFetchFailed = false; + } catch (final RetriableException ex) { + // ignore LEADER_NOT_AVAILABLE error, this can happen when the partition leader is not yet assigned. + hasLastOffsetsFetchFailed = true; + lastFailedFetchOffsetsTimestamp = time.milliseconds(); + } + } - assignedTopicPartitions = Collections.unmodifiableSet(updatedReassignedPartitions); - log.debug("Assigned topic partitions: {}", assignedTopicPartitions); + private void maybeFetchStartAndEndOffsets() { + // If the leader for a `__remote_log_metadata` partition is not available, then the call to `ListOffsets` + // will fail after the default timeout of 1 min. Added a delay between the retries to prevent the thread from + // aggressively fetching the list offsets. During this time, the recently reassigned user-topic-partitions + // won't be marked as initialized. + if (hasLastOffsetsFetchFailed && lastFailedFetchOffsetsTimestamp + offsetFetchRetryIntervalMs < time.milliseconds()) { + fetchStartAndEndOffsets(); + } + } - if (!updatedAssignedMetaPartitions.equals(assignedMetaPartitions)) { - assignedMetaPartitions = Collections.unmodifiableSet(updatedAssignedMetaPartitions); - log.debug("Assigned metadata topic partitions: {}", assignedMetaPartitions); + private UserTopicIdPartition newUserTopicIdPartition(final TopicIdPartition tpId) { + return new UserTopicIdPartition(tpId, topicPartitioner.metadataPartition(tpId)); + } - assignPartitions = true; - assignPartitionsLock.notifyAll(); - } else { - log.debug("No change in assigned metadata topic partitions: {}", assignedMetaPartitions); - } + private void markInitialized(final UserTopicIdPartition utp) { + // Silently not initialize the utp + if (!utp.isAssigned) { + log.warn("Tried to initialize a UTP: {} that was not yet assigned!", utp); + return; + } + if (!utp.isInitialized) { + remotePartitionMetadataEventHandler.markInitialized(utp.topicIdPartition); + utp.isInitialized = true; } } - public Optional receivedOffsetForPartition(int partition) { - return Optional.ofNullable(partitionToConsumedOffsets.get(partition)); + static Set toRemoteLogPartitions(final Set partitions) { + return partitions.stream() + .map(ConsumerTask::toRemoteLogPartition) + .collect(Collectors.toSet()); } - public boolean isPartitionAssigned(int partition) { - return assignedMetaPartitions.contains(partition); + static TopicPartition toRemoteLogPartition(int partition) { + return new TopicPartition(REMOTE_LOG_METADATA_TOPIC_NAME, partition); } - public void close() { - if (!closing) { - synchronized (assignPartitionsLock) { - // Closing should be updated only after acquiring the lock to avoid race in - // maybeWaitForPartitionsAssignment() where it waits on assignPartitionsLock. It should not wait - // if the closing is already set. - closing = true; - consumer.wakeup(); - assignPartitionsLock.notifyAll(); - } + static class UserTopicIdPartition { + private final TopicIdPartition topicIdPartition; + private final Integer metadataPartition; + // The `utp` will be initialized once it reads all the existing events from the remote log metadata topic. + boolean isInitialized; + // denotes whether this `utp` is assigned to the consumer + boolean isAssigned; + + /** + * UserTopicIdPartition denotes the user topic-partitions for which this broker acts as a leader/follower of. + * + * @param tpId the unique topic partition identifier + * @param metadataPartition the remote log metadata partition mapped for this user-topic-partition. + */ + public UserTopicIdPartition(final TopicIdPartition tpId, final Integer metadataPartition) { + this.topicIdPartition = Objects.requireNonNull(tpId); + this.metadataPartition = Objects.requireNonNull(metadataPartition); + this.isInitialized = false; + this.isAssigned = false; + } + + @Override + public String toString() { + return "UserTopicIdPartition{" + + "topicIdPartition=" + topicIdPartition + + ", metadataPartition=" + metadataPartition + + ", isInitialized=" + isInitialized + + ", isAssigned=" + isAssigned + + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + UserTopicIdPartition that = (UserTopicIdPartition) o; + return topicIdPartition.equals(that.topicIdPartition) && metadataPartition.equals(that.metadataPartition); + } + + @Override + public int hashCode() { + return Objects.hash(topicIdPartition, metadataPartition); } } - public Set metadataPartitionsAssigned() { - return Collections.unmodifiableSet(assignedMetaPartitions); + static class StartAndEndOffsetHolder { + Long startOffset; + Long endOffset; + + public StartAndEndOffsetHolder(Long startOffset, Long endOffset) { + this.startOffset = startOffset; + this.endOffset = endOffset; + } + + @Override + public String toString() { + return "StartAndEndOffsetHolder{" + + "startOffset=" + startOffset + + ", endOffset=" + endOffset + + '}'; + } } -} +} \ No newline at end of file diff --git a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataCache.java b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataCache.java index 8c89df3df2c8f..758a024e25c54 100644 --- a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataCache.java +++ b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemoteLogMetadataCache.java @@ -32,6 +32,7 @@ import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.CountDownLatch; /** * This class provides an in-memory cache of remote log segment metadata. This maintains the lineage of segments @@ -104,6 +105,16 @@ public class RemoteLogMetadataCache { // https://issues.apache.org/jira/browse/KAFKA-12641 protected final ConcurrentMap leaderEpochEntries = new ConcurrentHashMap<>(); + private final CountDownLatch initializedLatch = new CountDownLatch(1); + + public void markInitialized() { + initializedLatch.countDown(); + } + + public boolean isInitialized() { + return initializedLatch.getCount() == 0; + } + /** * Returns {@link RemoteLogSegmentMetadata} if it exists for the given leader-epoch containing the offset and with * {@link RemoteLogSegmentState#COPY_SEGMENT_FINISHED} state, else returns {@link Optional#empty()}. diff --git a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataEventHandler.java b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataEventHandler.java index c92a51ecacac6..f4f43b0d88377 100644 --- a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataEventHandler.java +++ b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataEventHandler.java @@ -50,4 +50,9 @@ public abstract void syncLogMetadataSnapshot(TopicIdPartition topicIdPartition, public abstract void clearTopicPartition(TopicIdPartition topicIdPartition); + public abstract void markInitialized(TopicIdPartition partition); + + public abstract boolean isInitialized(TopicIdPartition partition); + + public abstract void maybeLoadPartition(TopicIdPartition partition); } \ No newline at end of file diff --git a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataStore.java b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataStore.java index 7051d184aad5b..f9394eee99f36 100644 --- a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataStore.java +++ b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/RemotePartitionMetadataStore.java @@ -18,6 +18,7 @@ import org.apache.kafka.common.TopicIdPartition; import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.errors.ReplicaNotAvailableException; import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentId; import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentMetadata; import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentMetadataUpdate; @@ -151,6 +152,12 @@ private FileBasedRemoteLogMetadataCache getRemoteLogMetadataCache(TopicIdPartiti throw new RemoteResourceNotFoundException("No resource found for partition: " + topicIdPartition); } + if (!remoteLogMetadataCache.isInitialized()) { + // Throwing a retriable ReplicaNotAvailableException here for clients retry. We can introduce a new more + // appropriate exception with a KIP in the future. + throw new ReplicaNotAvailableException("Remote log metadata cache is not initialized for partition: " + topicIdPartition); + } + return remoteLogMetadataCache; } @@ -180,9 +187,21 @@ public void close() throws IOException { idToRemoteLogMetadataCache = Collections.emptyMap(); } + @Override public void maybeLoadPartition(TopicIdPartition partition) { idToRemoteLogMetadataCache.computeIfAbsent(partition, topicIdPartition -> new FileBasedRemoteLogMetadataCache(topicIdPartition, partitionLogDirectory(topicIdPartition.topicPartition()))); } + @Override + public void markInitialized(TopicIdPartition partition) { + idToRemoteLogMetadataCache.get(partition).markInitialized(); + log.trace("Remote log components are initialized for user-partition: {}", partition); + } + + @Override + public boolean isInitialized(TopicIdPartition topicIdPartition) { + RemoteLogMetadataCache metadataCache = idToRemoteLogMetadataCache.get(topicIdPartition); + return metadataCache != null && metadataCache.isInitialized(); + } } diff --git a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java index 4b02b9b676367..e1bf145bbd888 100644 --- a/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java +++ b/storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManager.java @@ -84,7 +84,7 @@ public class TopicBasedRemoteLogMetadataManager implements RemoteLogMetadataMana private RemotePartitionMetadataStore remotePartitionMetadataStore; private volatile TopicBasedRemoteLogMetadataManagerConfig rlmmConfig; - private volatile RemoteLogMetadataTopicPartitioner rlmmTopicPartitioner; + private volatile RemoteLogMetadataTopicPartitioner rlmTopicPartitioner; private final Set pendingAssignPartitions = Collections.synchronizedSet(new HashSet<>()); private volatile boolean initializationFailed; @@ -260,12 +260,12 @@ public Iterator listRemoteLogSegments(TopicIdPartition } public int metadataPartition(TopicIdPartition topicIdPartition) { - return rlmmTopicPartitioner.metadataPartition(topicIdPartition); + return rlmTopicPartitioner.metadataPartition(topicIdPartition); } // Visible For Testing - public Optional receivedOffsetForPartition(int metadataPartition) { - return consumerManager.receivedOffsetForPartition(metadataPartition); + public Optional readOffsetForPartition(int metadataPartition) { + return consumerManager.readOffsetForPartition(metadataPartition); } @Override @@ -357,7 +357,7 @@ public void configure(Map configs) { log.info("Started configuring topic-based RLMM with configs: {}", configs); rlmmConfig = new TopicBasedRemoteLogMetadataManagerConfig(configs); - rlmmTopicPartitioner = new RemoteLogMetadataTopicPartitioner(rlmmConfig.metadataTopicPartitionsCount()); + rlmTopicPartitioner = new RemoteLogMetadataTopicPartitioner(rlmmConfig.metadataTopicPartitionsCount()); remotePartitionMetadataStore = new RemotePartitionMetadataStore(new File(rlmmConfig.logDir()).toPath()); configured = true; log.info("Successfully configured topic-based RLMM with config: {}", rlmmConfig); @@ -416,8 +416,8 @@ private void initializeResources() { // Create producer and consumer managers. lock.writeLock().lock(); try { - producerManager = new ProducerManager(rlmmConfig, rlmmTopicPartitioner); - consumerManager = new ConsumerManager(rlmmConfig, remotePartitionMetadataStore, rlmmTopicPartitioner, time); + producerManager = new ProducerManager(rlmmConfig, rlmTopicPartitioner); + consumerManager = new ConsumerManager(rlmmConfig, remotePartitionMetadataStore, rlmTopicPartitioner, time); if (startConsumerThread) { consumerManager.startConsumerThread(); } else { @@ -509,10 +509,8 @@ public TopicBasedRemoteLogMetadataManagerConfig config() { } // Visible for testing. - public void startConsumerThread() { - if (consumerManager != null) { - consumerManager.startConsumerThread(); - } + void setRlmTopicPartitioner(RemoteLogMetadataTopicPartitioner rlmTopicPartitioner) { + this.rlmTopicPartitioner = Objects.requireNonNull(rlmTopicPartitioner); } @Override diff --git a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTaskTest.java b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTaskTest.java new file mode 100644 index 0000000000000..2b36c4bb03968 --- /dev/null +++ b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/ConsumerTaskTest.java @@ -0,0 +1,417 @@ +/* + * 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.server.log.remote.metadata.storage; + +import org.apache.kafka.clients.consumer.ConsumerRecord; +import org.apache.kafka.clients.consumer.MockConsumer; +import org.apache.kafka.clients.consumer.OffsetResetStrategy; +import org.apache.kafka.common.TopicIdPartition; +import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.Uuid; +import org.apache.kafka.common.errors.AuthorizationException; +import org.apache.kafka.common.errors.LeaderNotAvailableException; +import org.apache.kafka.common.errors.TimeoutException; +import org.apache.kafka.common.utils.SystemTime; +import org.apache.kafka.server.log.remote.metadata.storage.serialization.RemoteLogMetadataSerde; +import org.apache.kafka.server.log.remote.storage.RemoteLogMetadata; +import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentId; +import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentMetadata; +import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentMetadataUpdate; +import org.apache.kafka.server.log.remote.storage.RemotePartitionDeleteMetadata; +import org.apache.kafka.test.TestCondition; +import org.apache.kafka.test.TestUtils; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +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.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.IntStream; + +import static org.apache.kafka.server.log.remote.metadata.storage.ConsumerTask.UserTopicIdPartition; +import static org.apache.kafka.server.log.remote.metadata.storage.ConsumerTask.toRemoteLogPartition; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +public class ConsumerTaskTest { + + private final int numMetadataTopicPartitions = 5; + private final RemoteLogMetadataTopicPartitioner partitioner = new RemoteLogMetadataTopicPartitioner(numMetadataTopicPartitions); + private final DummyEventHandler handler = new DummyEventHandler(); + private final Set remoteLogPartitions = IntStream.range(0, numMetadataTopicPartitions).boxed() + .map(ConsumerTask::toRemoteLogPartition).collect(Collectors.toSet()); + private final Uuid topicId = Uuid.randomUuid(); + private final RemoteLogMetadataSerde serde = new RemoteLogMetadataSerde(); + + private ConsumerTask consumerTask; + private MockConsumer consumer; + private Thread thread; + + @BeforeEach + public void beforeEach() { + final Map offsets = remoteLogPartitions.stream() + .collect(Collectors.toMap(Function.identity(), e -> 0L)); + consumer = new MockConsumer<>(OffsetResetStrategy.EARLIEST); + consumer.updateBeginningOffsets(offsets); + consumerTask = new ConsumerTask(handler, partitioner, consumer, 10L, 300_000L, new SystemTime()); + thread = new Thread(consumerTask); + } + + @AfterEach + public void afterEach() throws InterruptedException { + if (thread != null) { + assertDoesNotThrow(() -> consumerTask.close(), "Close method threw exception"); + thread.join(10_000); + assertFalse(thread.isAlive(), "Consumer task thread is still alive"); + } + } + + /** + * Tests that the consumer task shuts down gracefully when there were no assignments. + */ + @Test + public void testCloseOnNoAssignment() throws InterruptedException { + thread.start(); + Thread.sleep(10); + assertDoesNotThrow(() -> consumerTask.close(), "Close method threw exception"); + } + + @Test + public void testIdempotentClose() { + thread.start(); + consumerTask.close(); + consumerTask.close(); + } + + @Test + public void testUserTopicIdPartitionEquals() { + final TopicIdPartition tpId = new TopicIdPartition(topicId, new TopicPartition("sample", 0)); + final UserTopicIdPartition utp1 = new UserTopicIdPartition(tpId, partitioner.metadataPartition(tpId)); + final UserTopicIdPartition utp2 = new UserTopicIdPartition(tpId, partitioner.metadataPartition(tpId)); + utp1.isInitialized = true; + utp1.isAssigned = true; + + assertFalse(utp2.isInitialized); + assertFalse(utp2.isAssigned); + assertEquals(utp1, utp2); + } + + @Test + public void testAddAssignmentsForPartitions() throws InterruptedException { + final List idPartitions = getIdPartitions("sample", 3); + final Map endOffsets = idPartitions.stream() + .map(idp -> toRemoteLogPartition(partitioner.metadataPartition(idp))) + .collect(Collectors.toMap(Function.identity(), e -> 0L, (a, b) -> b)); + consumer.updateEndOffsets(endOffsets); + consumerTask.addAssignmentsForPartitions(new HashSet<>(idPartitions)); + thread.start(); + for (final TopicIdPartition idPartition : idPartitions) { + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(idPartition), "Timed out waiting for " + idPartition + " to be assigned"); + assertTrue(consumerTask.isMetadataPartitionAssigned(partitioner.metadataPartition(idPartition))); + assertTrue(handler.isPartitionLoaded.get(idPartition)); + } + } + + @Test + public void testRemoveAssignmentsForPartitions() throws InterruptedException { + final List allPartitions = getIdPartitions("sample", 3); + final Map endOffsets = allPartitions.stream() + .map(idp -> toRemoteLogPartition(partitioner.metadataPartition(idp))) + .collect(Collectors.toMap(Function.identity(), e -> 0L, (a, b) -> b)); + consumer.updateEndOffsets(endOffsets); + consumerTask.addAssignmentsForPartitions(new HashSet<>(allPartitions)); + thread.start(); + + final TopicIdPartition tpId = allPartitions.get(0); + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(tpId), "Timed out waiting for " + tpId + " to be assigned"); + addRecord(consumer, partitioner.metadataPartition(tpId), tpId, 0); + TestUtils.waitForCondition(() -> consumerTask.readOffsetForMetadataPartition(partitioner.metadataPartition(tpId)).isPresent(), + "Couldn't read record"); + + final Set removePartitions = Collections.singleton(tpId); + consumerTask.removeAssignmentsForPartitions(removePartitions); + for (final TopicIdPartition idPartition : allPartitions) { + final TestCondition condition = () -> removePartitions.contains(idPartition) == !consumerTask.isUserPartitionAssigned(idPartition); + TestUtils.waitForCondition(condition, "Timed out waiting for " + idPartition + " to be removed"); + } + for (TopicIdPartition removePartition : removePartitions) { + TestUtils.waitForCondition(() -> handler.isPartitionCleared.containsKey(removePartition), + "Timed out waiting for " + removePartition + " to be cleared"); + } + } + + @Test + public void testConcurrentPartitionAssignments() throws InterruptedException, ExecutionException { + final List allPartitions = getIdPartitions("sample", 100); + final Map endOffsets = allPartitions.stream() + .map(idp -> toRemoteLogPartition(partitioner.metadataPartition(idp))) + .collect(Collectors.toMap(Function.identity(), e -> 0L, (a, b) -> b)); + consumer.updateEndOffsets(endOffsets); + + final AtomicBoolean isAllPartitionsAssigned = new AtomicBoolean(false); + CountDownLatch latch = new CountDownLatch(1); + Thread assignor = new Thread(() -> { + int partitionsAssigned = 0; + for (TopicIdPartition partition : allPartitions) { + if (partitionsAssigned == 50) { + // Once half the topic partitions are assigned, wait for the consumer to catch up. This ensures + // that the consumer is already running when the rest of the partitions are assigned. + try { + latch.await(1, TimeUnit.MINUTES); + } catch (InterruptedException e) { + fail(e.getMessage()); + } + } + consumerTask.addAssignmentsForPartitions(Collections.singleton(partition)); + partitionsAssigned++; + } + isAllPartitionsAssigned.set(true); + }); + Runnable consumerRunnable = () -> { + try { + while (!isAllPartitionsAssigned.get()) { + consumerTask.maybeWaitForPartitionAssignments(); + latch.countDown(); + } + } catch (Exception e) { + fail(e.getMessage()); + } + }; + + ExecutorService consumerExecutor = Executors.newSingleThreadExecutor(); + Future future = consumerExecutor.submit(consumerRunnable); + assignor.start(); + + assignor.join(); + future.get(); + } + + @Test + public void testCanProcessRecord() throws InterruptedException { + final Uuid topicId = Uuid.fromString("Bp9TDduJRGa9Q5rlvCJOxg"); + final TopicIdPartition tpId0 = new TopicIdPartition(topicId, new TopicPartition("sample", 0)); + final TopicIdPartition tpId1 = new TopicIdPartition(topicId, new TopicPartition("sample", 1)); + final TopicIdPartition tpId2 = new TopicIdPartition(topicId, new TopicPartition("sample", 2)); + assertEquals(partitioner.metadataPartition(tpId0), partitioner.metadataPartition(tpId1)); + assertEquals(partitioner.metadataPartition(tpId0), partitioner.metadataPartition(tpId2)); + + final int metadataPartition = partitioner.metadataPartition(tpId0); + consumer.updateEndOffsets(Collections.singletonMap(toRemoteLogPartition(metadataPartition), 0L)); + final Set assignments = Collections.singleton(tpId0); + consumerTask.addAssignmentsForPartitions(assignments); + thread.start(); + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(tpId0), "Timed out waiting for " + tpId0 + " to be assigned"); + + addRecord(consumer, metadataPartition, tpId0, 0); + addRecord(consumer, metadataPartition, tpId0, 1); + TestUtils.waitForCondition(() -> consumerTask.readOffsetForMetadataPartition(metadataPartition).equals(Optional.of(1L)), "Couldn't read record"); + assertEquals(2, handler.metadataCounter); + + // should only read the tpId1 records + consumerTask.addAssignmentsForPartitions(Collections.singleton(tpId1)); + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(tpId1), "Timed out waiting for " + tpId1 + " to be assigned"); + addRecord(consumer, metadataPartition, tpId1, 2); + TestUtils.waitForCondition(() -> consumerTask.readOffsetForMetadataPartition(metadataPartition).equals(Optional.of(2L)), "Couldn't read record"); + assertEquals(3, handler.metadataCounter); + + // shouldn't read tpId2 records because it's not assigned + addRecord(consumer, metadataPartition, tpId2, 3); + TestUtils.waitForCondition(() -> consumerTask.readOffsetForMetadataPartition(metadataPartition).equals(Optional.of(3L)), "Couldn't read record"); + assertEquals(3, handler.metadataCounter); + } + + @Test + public void testMaybeMarkUserPartitionsAsReady() throws InterruptedException { + final TopicIdPartition tpId = getIdPartitions("hello", 1).get(0); + final int metadataPartition = partitioner.metadataPartition(tpId); + consumer.updateEndOffsets(Collections.singletonMap(toRemoteLogPartition(metadataPartition), 2L)); + consumerTask.addAssignmentsForPartitions(Collections.singleton(tpId)); + thread.start(); + + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(tpId), "Waiting for " + tpId + " to be assigned"); + assertTrue(consumerTask.isMetadataPartitionAssigned(metadataPartition)); + assertFalse(handler.isPartitionInitialized.containsKey(tpId)); + IntStream.range(0, 5).forEach(offset -> addRecord(consumer, metadataPartition, tpId, offset)); + TestUtils.waitForCondition(() -> consumerTask.readOffsetForMetadataPartition(metadataPartition).equals(Optional.of(4L)), "Couldn't read record"); + assertTrue(handler.isPartitionInitialized.get(tpId)); + } + + @ParameterizedTest + @CsvSource(value = {"0, 0", "500, 500"}) + public void testMaybeMarkUserPartitionAsReadyWhenTopicIsEmpty(long beginOffset, + long endOffset) throws InterruptedException { + final TopicIdPartition tpId = getIdPartitions("world", 1).get(0); + final int metadataPartition = partitioner.metadataPartition(tpId); + consumer.updateBeginningOffsets(Collections.singletonMap(toRemoteLogPartition(metadataPartition), beginOffset)); + consumer.updateEndOffsets(Collections.singletonMap(toRemoteLogPartition(metadataPartition), endOffset)); + consumerTask.addAssignmentsForPartitions(Collections.singleton(tpId)); + thread.start(); + + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(tpId), "Waiting for " + tpId + " to be assigned"); + assertTrue(consumerTask.isMetadataPartitionAssigned(metadataPartition)); + TestUtils.waitForCondition(() -> handler.isPartitionInitialized.containsKey(tpId), + "should have initialized the partition"); + assertFalse(consumerTask.readOffsetForMetadataPartition(metadataPartition).isPresent()); + } + + @Test + public void testConcurrentAccess() throws InterruptedException { + thread.start(); + final CountDownLatch latch = new CountDownLatch(1); + final TopicIdPartition tpId = getIdPartitions("concurrent", 1).get(0); + consumer.updateEndOffsets(Collections.singletonMap(toRemoteLogPartition(partitioner.metadataPartition(tpId)), 0L)); + final Thread assignmentThread = new Thread(() -> { + try { + latch.await(); + consumerTask.addAssignmentsForPartitions(Collections.singleton(tpId)); + } catch (final InterruptedException e) { + fail("Shouldn't have thrown an exception"); + } + }); + final Thread closeThread = new Thread(() -> { + try { + latch.await(); + consumerTask.close(); + } catch (final InterruptedException e) { + fail("Shouldn't have thrown an exception"); + } + }); + assignmentThread.start(); + closeThread.start(); + + latch.countDown(); + assignmentThread.join(); + closeThread.join(); + } + + @Test + public void testConsumerShouldNotCloseOnRetriableError() throws InterruptedException { + final TopicIdPartition tpId = getIdPartitions("world", 1).get(0); + final int metadataPartition = partitioner.metadataPartition(tpId); + consumer.updateEndOffsets(Collections.singletonMap(toRemoteLogPartition(metadataPartition), 1L)); + consumerTask.addAssignmentsForPartitions(Collections.singleton(tpId)); + thread.start(); + + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(tpId), "Waiting for " + tpId + " to be assigned"); + assertTrue(consumerTask.isMetadataPartitionAssigned(metadataPartition)); + + consumer.setPollException(new LeaderNotAvailableException("leader not available!")); + addRecord(consumer, metadataPartition, tpId, 0); + consumer.setPollException(new TimeoutException("Not able to complete the operation within the timeout")); + addRecord(consumer, metadataPartition, tpId, 1); + + TestUtils.waitForCondition(() -> consumerTask.readOffsetForMetadataPartition(metadataPartition).equals(Optional.of(1L)), "Couldn't read record"); + assertEquals(2, handler.metadataCounter); + } + + @Test + public void testConsumerShouldCloseOnNonRetriableError() throws InterruptedException { + final TopicIdPartition tpId = getIdPartitions("world", 1).get(0); + final int metadataPartition = partitioner.metadataPartition(tpId); + consumer.updateEndOffsets(Collections.singletonMap(toRemoteLogPartition(metadataPartition), 1L)); + consumerTask.addAssignmentsForPartitions(Collections.singleton(tpId)); + thread.start(); + + TestUtils.waitForCondition(() -> consumerTask.isUserPartitionAssigned(tpId), "Waiting for " + tpId + " to be assigned"); + assertTrue(consumerTask.isMetadataPartitionAssigned(metadataPartition)); + + consumer.setPollException(new AuthorizationException("Unauthorized to read the topic!")); + TestUtils.waitForCondition(() -> consumer.closed(), "Should close the consume on non-retriable error"); + } + + private void addRecord(final MockConsumer consumer, + final int metadataPartition, + final TopicIdPartition idPartition, + final long recordOffset) { + final RemoteLogSegmentId segmentId = new RemoteLogSegmentId(idPartition, Uuid.randomUuid()); + final RemoteLogMetadata metadata = new RemoteLogSegmentMetadata(segmentId, 0L, 1L, 0L, 0, 0L, 1, Collections.singletonMap(0, 0L)); + final ConsumerRecord record = new ConsumerRecord<>(TopicBasedRemoteLogMetadataManagerConfig.REMOTE_LOG_METADATA_TOPIC_NAME, metadataPartition, recordOffset, null, serde.serialize(metadata)); + consumer.addRecord(record); + } + + private List getIdPartitions(final String topic, final int partitionCount) { + final List idPartitions = new ArrayList<>(); + for (int partition = 0; partition < partitionCount; partition++) { + idPartitions.add(new TopicIdPartition(topicId, new TopicPartition(topic, partition))); + } + return idPartitions; + } + + private static class DummyEventHandler extends RemotePartitionMetadataEventHandler { + private int metadataCounter = 0; + private final Map isPartitionInitialized = new HashMap<>(); + private final Map isPartitionLoaded = new HashMap<>(); + private final Map isPartitionCleared = new HashMap<>(); + + @Override + protected void handleRemoteLogSegmentMetadata(RemoteLogSegmentMetadata remoteLogSegmentMetadata) { + metadataCounter++; + } + + @Override + protected void handleRemoteLogSegmentMetadataUpdate(RemoteLogSegmentMetadataUpdate remoteLogSegmentMetadataUpdate) { + } + + @Override + protected void handleRemotePartitionDeleteMetadata(RemotePartitionDeleteMetadata remotePartitionDeleteMetadata) { + } + + @Override + public void syncLogMetadataSnapshot(TopicIdPartition topicIdPartition, int metadataPartition, Long metadataPartitionOffset) { + } + + @Override + public void clearTopicPartition(TopicIdPartition topicIdPartition) { + isPartitionCleared.put(topicIdPartition, true); + } + + @Override + public void markInitialized(TopicIdPartition partition) { + isPartitionInitialized.put(partition, true); + } + + @Override + public boolean isInitialized(TopicIdPartition partition) { + return true; + } + + @Override + public void maybeLoadPartition(TopicIdPartition partition) { + isPartitionLoaded.put(partition, true); + } + } +} diff --git a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerHarness.java b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerHarness.java index e39d872744a42..abad6ea767605 100644 --- a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerHarness.java +++ b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerHarness.java @@ -63,11 +63,12 @@ public void initialize(Set topicIdPartitions, // Call setup to start the cluster. super.setUp(new EmptyTestInfo()); - initializeRemoteLogMetadataManager(topicIdPartitions, startConsumerThread); + initializeRemoteLogMetadataManager(topicIdPartitions, startConsumerThread, null); } public void initializeRemoteLogMetadataManager(Set topicIdPartitions, - boolean startConsumerThread) { + boolean startConsumerThread, + RemoteLogMetadataTopicPartitioner remoteLogMetadataTopicPartitioner) { String logDir = TestUtils.tempDirectory("rlmm_segs_").getAbsolutePath(); topicBasedRemoteLogMetadataManager = new TopicBasedRemoteLogMetadataManager(startConsumerThread) { @Override @@ -104,6 +105,9 @@ public void onPartitionLeadershipChanges(Set leaderPartitions, log.debug("TopicBasedRemoteLogMetadataManager configs after adding overridden properties: {}", configs); topicBasedRemoteLogMetadataManager.configure(configs); + if (remoteLogMetadataTopicPartitioner != null) { + topicBasedRemoteLogMetadataManager.setRlmTopicPartitioner(remoteLogMetadataTopicPartitioner); + } try { waitUntilInitialized(60_000); } catch (TimeoutException e) { @@ -145,4 +149,4 @@ public void close() throws IOException { public void closeRemoteLogMetadataManager() { Utils.closeQuietly(topicBasedRemoteLogMetadataManager, "TopicBasedRemoteLogMetadataManager"); } -} \ No newline at end of file +} diff --git a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerMultipleSubscriptionsTest.java b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerMultipleSubscriptionsTest.java new file mode 100644 index 0000000000000..3386b94f8957f --- /dev/null +++ b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerMultipleSubscriptionsTest.java @@ -0,0 +1,178 @@ +/* + * 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.server.log.remote.metadata.storage; + + +import kafka.utils.EmptyTestInfo; +import org.apache.kafka.common.TopicIdPartition; +import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.Uuid; +import org.apache.kafka.common.errors.TimeoutException; +import org.apache.kafka.common.utils.MockTime; +import org.apache.kafka.common.utils.Time; +import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentId; +import org.apache.kafka.server.log.remote.storage.RemoteLogSegmentMetadata; +import org.apache.kafka.server.log.remote.storage.RemoteStorageException; +import org.apache.kafka.test.TestUtils; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import scala.collection.JavaConverters; +import scala.collection.Seq; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.concurrent.ExecutionException; + +@SuppressWarnings("deprecation") // Added for Scala 2.12 compatibility for usages of JavaConverters +public class TopicBasedRemoteLogMetadataManagerMultipleSubscriptionsTest { + private static final Logger log = LoggerFactory.getLogger(TopicBasedRemoteLogMetadataManagerMultipleSubscriptionsTest.class); + + private static final int SEG_SIZE = 1024 * 1024; + + private final Time time = new MockTime(1); + private final TopicBasedRemoteLogMetadataManagerHarness remoteLogMetadataManagerHarness = new TopicBasedRemoteLogMetadataManagerHarness(); + + private TopicBasedRemoteLogMetadataManager rlmm() { + return remoteLogMetadataManagerHarness.remoteLogMetadataManager(); + } + + @BeforeEach + public void setup() { + // Start the cluster only. + remoteLogMetadataManagerHarness.setUp(new EmptyTestInfo()); + } + + @AfterEach + public void teardown() throws IOException { + remoteLogMetadataManagerHarness.close(); + } + + @Test + public void testMultiplePartitionSubscriptions() throws Exception { + // Create topics. + String leaderTopic = "leader"; + HashMap> assignedLeaderTopicReplicas = new HashMap<>(); + List leaderTopicReplicas = new ArrayList<>(); + // Set broker id 0 as the first entry which is taken as the leader. + leaderTopicReplicas.add(0); + leaderTopicReplicas.add(1); + leaderTopicReplicas.add(2); + assignedLeaderTopicReplicas.put(0, JavaConverters.asScalaBuffer(leaderTopicReplicas)); + remoteLogMetadataManagerHarness.createTopicWithAssignment(leaderTopic, + JavaConverters.mapAsScalaMap(assignedLeaderTopicReplicas), + remoteLogMetadataManagerHarness.listenerName()); + + String followerTopic = "follower"; + HashMap> assignedFollowerTopicReplicas = new HashMap<>(); + List followerTopicReplicas = new ArrayList<>(); + // Set broker id 1 as the first entry which is taken as the leader. + followerTopicReplicas.add(1); + followerTopicReplicas.add(2); + followerTopicReplicas.add(0); + assignedFollowerTopicReplicas.put(0, JavaConverters.asScalaBuffer(followerTopicReplicas)); + remoteLogMetadataManagerHarness.createTopicWithAssignment( + followerTopic, JavaConverters.mapAsScalaMap(assignedFollowerTopicReplicas), + remoteLogMetadataManagerHarness.listenerName()); + + String topicWithNoMessages = "no-messages-topic"; + HashMap> assignedTopicReplicas = new HashMap<>(); + List noMessagesTopicReplicas = new ArrayList<>(); + // Set broker id 1 as the first entry which is taken as the leader. + noMessagesTopicReplicas.add(1); + noMessagesTopicReplicas.add(2); + noMessagesTopicReplicas.add(0); + assignedTopicReplicas.put(0, JavaConverters.asScalaBuffer(noMessagesTopicReplicas)); + remoteLogMetadataManagerHarness.createTopicWithAssignment( + topicWithNoMessages, JavaConverters.mapAsScalaMap(assignedTopicReplicas), + remoteLogMetadataManagerHarness.listenerName()); + + final TopicIdPartition leaderTopicIdPartition = new TopicIdPartition(Uuid.randomUuid(), new TopicPartition(leaderTopic, 0)); + final TopicIdPartition followerTopicIdPartition = new TopicIdPartition(Uuid.randomUuid(), new TopicPartition(followerTopic, 0)); + final TopicIdPartition emptyTopicIdPartition = new TopicIdPartition(Uuid.randomUuid(), new TopicPartition(topicWithNoMessages, 0)); + + RemoteLogMetadataTopicPartitioner partitioner = new RemoteLogMetadataTopicPartitioner(10) { + @Override + public int metadataPartition(TopicIdPartition topicIdPartition) { + // Always return partition 0 except for noMessagesTopicIdPartition. So that, any new user + // partition(other than noMessagesTopicIdPartition) added to RLMM will use the same metadata partition. + // That will make the secondary consumer assignment. + if (emptyTopicIdPartition.equals(topicIdPartition)) { + return 1; + } else { + return 0; + } + } + }; + + remoteLogMetadataManagerHarness.initializeRemoteLogMetadataManager(Collections.emptySet(), true, partitioner); + + // Add segments for these partitions but an exception is received as they have not yet been subscribed. + // These messages would have been published to the respective metadata topic partitions but the ConsumerManager + // has not yet been subscribing as they are not yet registered. + RemoteLogSegmentMetadata leaderSegmentMetadata = new RemoteLogSegmentMetadata(new RemoteLogSegmentId(leaderTopicIdPartition, Uuid.randomUuid()), + 0, 100, -1L, 0, + time.milliseconds(), SEG_SIZE, Collections.singletonMap(0, 0L)); + ExecutionException exception = Assertions.assertThrows(ExecutionException.class, () -> rlmm().addRemoteLogSegmentMetadata(leaderSegmentMetadata).get()); + Assertions.assertEquals("org.apache.kafka.common.KafkaException: This consumer is not assigned to the target partition 0. Partitions currently assigned: []", + exception.getMessage()); + + RemoteLogSegmentMetadata followerSegmentMetadata = new RemoteLogSegmentMetadata(new RemoteLogSegmentId(followerTopicIdPartition, Uuid.randomUuid()), + 0, 100, -1L, 0, + time.milliseconds(), SEG_SIZE, Collections.singletonMap(0, 0L)); + exception = Assertions.assertThrows(ExecutionException.class, () -> rlmm().addRemoteLogSegmentMetadata(followerSegmentMetadata).get()); + Assertions.assertEquals("org.apache.kafka.common.KafkaException: This consumer is not assigned to the target partition 0. Partitions currently assigned: []", + exception.getMessage()); + + // `listRemoteLogSegments` will receive an exception as these topic partitions are not yet registered. + Assertions.assertThrows(RemoteStorageException.class, () -> rlmm().listRemoteLogSegments(leaderTopicIdPartition)); + Assertions.assertThrows(RemoteStorageException.class, () -> rlmm().listRemoteLogSegments(followerTopicIdPartition)); + + rlmm().onPartitionLeadershipChanges(Collections.singleton(leaderTopicIdPartition), + Collections.emptySet()); + + // RemoteLogSegmentMetadata events are already published, and topicBasedRlmm's consumer manager will start + // fetching those events and build the cache. + waitUntilConsumerCatchesUp(30_000L); + // leader partitions would have received as it is registered, but follower partition is not yet registered, + // hence it throws an exception. + Assertions.assertTrue(rlmm().listRemoteLogSegments(leaderTopicIdPartition).hasNext()); + Assertions.assertThrows(RemoteStorageException.class, () -> rlmm().listRemoteLogSegments(followerTopicIdPartition)); + + // Register follower partition + rlmm().onPartitionLeadershipChanges(Collections.singleton(emptyTopicIdPartition), + Collections.singleton(followerTopicIdPartition)); + + // In this state, all the metadata should be available in RLMM for both leader and follower partitions. + TestUtils.waitForCondition(() -> rlmm().listRemoteLogSegments(leaderTopicIdPartition).hasNext(), "No segments found"); + TestUtils.waitForCondition(() -> rlmm().listRemoteLogSegments(followerTopicIdPartition).hasNext(), "No segments found"); + } + + private void waitUntilConsumerCatchesUp(long timeoutMs) throws TimeoutException, InterruptedException { + TestUtils.waitForCondition(() -> { + // If both the leader and follower partitions are mapped to the same metadata partition which is 0, it + // should have at least 2 messages. That means, read offset should be >= 1 (including duplicate messages if any). + return rlmm().readOffsetForPartition(0).orElse(-1L) >= 1; + }, timeoutMs, "Consumer did not catch up"); + } +} diff --git a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerRestartTest.java b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerRestartTest.java index 714405a30e918..46ffeba214272 100644 --- a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerRestartTest.java +++ b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerRestartTest.java @@ -31,18 +31,14 @@ import scala.collection.JavaConverters; import scala.collection.Seq; -import java.io.File; import java.io.IOException; -import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Optional; -import static org.apache.kafka.server.log.remote.metadata.storage.ConsumerManager.COMMITTED_OFFSETS_FILE_NAME; import static org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManagerConfig.LOG_DIR; @SuppressWarnings("deprecation") // Added for Scala 2.12 compatibility for usages of JavaConverters @@ -69,7 +65,7 @@ protected Map overrideRemoteLogMetadataManagerProps() { } private void startTopicBasedRemoteLogMetadataManagerHarness(boolean startConsumerThread) { - remoteLogMetadataManagerHarness.initializeRemoteLogMetadataManager(Collections.emptySet(), startConsumerThread); + remoteLogMetadataManagerHarness.initializeRemoteLogMetadataManager(Collections.emptySet(), startConsumerThread, null); } @AfterEach @@ -136,9 +132,8 @@ public void testRLMMAPIsAfterRestart() throws Exception { // Stop TopicBasedRemoteLogMetadataManager only. stopTopicBasedRemoteLogMetadataManagerHarness(); - // Start TopicBasedRemoteLogMetadataManager but do not start consumer thread to check whether the stored metadata is - // loaded successfully or not. - startTopicBasedRemoteLogMetadataManagerHarness(false); + // Start TopicBasedRemoteLogMetadataManager + startTopicBasedRemoteLogMetadataManagerHarness(true); // Register these partitions to RLMM, which loads the respective metadata snapshots. topicBasedRlmm().onPartitionLeadershipChanges(Collections.singleton(leaderTopicIdPartition), Collections.singleton(followerTopicIdPartition)); @@ -148,29 +143,6 @@ public void testRLMMAPIsAfterRestart() throws Exception { topicBasedRlmm().listRemoteLogSegments(leaderTopicIdPartition))); Assertions.assertTrue(TestUtils.sameElementsWithoutOrder(Collections.singleton(followerSegmentMetadata).iterator(), topicBasedRlmm().listRemoteLogSegments(followerTopicIdPartition))); - // Check whether the check-pointed consumer offsets are stored or not. - Path committedOffsetsPath = new File(logDir, COMMITTED_OFFSETS_FILE_NAME).toPath(); - Assertions.assertTrue(committedOffsetsPath.toFile().exists()); - CommittedOffsetsFile committedOffsetsFile = new CommittedOffsetsFile(committedOffsetsPath.toFile()); - - int metadataPartition1 = topicBasedRlmm().metadataPartition(leaderTopicIdPartition); - int metadataPartition2 = topicBasedRlmm().metadataPartition(followerTopicIdPartition); - Optional receivedOffsetForPartition1 = topicBasedRlmm().receivedOffsetForPartition(metadataPartition1); - Optional receivedOffsetForPartition2 = topicBasedRlmm().receivedOffsetForPartition(metadataPartition2); - Assertions.assertTrue(receivedOffsetForPartition1.isPresent()); - Assertions.assertTrue(receivedOffsetForPartition2.isPresent()); - - // Make sure these offsets are at least 0. - Assertions.assertTrue(receivedOffsetForPartition1.get() >= 0); - Assertions.assertTrue(receivedOffsetForPartition2.get() >= 0); - - // Check the stored entries and the offsets that were set on consumer are the same. - Map partitionToOffset = committedOffsetsFile.readEntries(); - Assertions.assertEquals(partitionToOffset.get(metadataPartition1), receivedOffsetForPartition1.get()); - Assertions.assertEquals(partitionToOffset.get(metadataPartition2), receivedOffsetForPartition2.get()); - - // Start Consumer thread - topicBasedRlmm().startConsumerThread(); // Add one more segment RemoteLogSegmentMetadata leaderSegmentMetadata2 = new RemoteLogSegmentMetadata( diff --git a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java index eaf62edfea7ee..96e48de8a7359 100644 --- a/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java +++ b/storage/src/test/java/org/apache/kafka/server/log/remote/metadata/storage/TopicBasedRemoteLogMetadataManagerTest.java @@ -149,17 +149,17 @@ private void waitUntilConsumerCatchesUp(TopicIdPartition newLeaderTopicIdPartiti } // If both the leader and follower partitions are mapped to the same metadata partition then it should have at least - // 2 messages. That means, received offset should be >= 1 (including duplicate messages if any). + // 2 messages. That means, read offset should be >= 1 (including duplicate messages if any). if (leaderMetadataPartition == followerMetadataPartition) { - if (topicBasedRlmm().receivedOffsetForPartition(leaderMetadataPartition).orElse(-1L) >= 1) { + if (topicBasedRlmm().readOffsetForPartition(leaderMetadataPartition).orElse(-1L) >= 1) { break; } } else { // If the leader partition and the follower partition are mapped to different metadata partitions then - // each of those metadata partitions will have at least 1 message. That means, received offset should + // each of those metadata partitions will have at least 1 message. That means, read offset should // be >= 0 (including duplicate messages if any). - if (topicBasedRlmm().receivedOffsetForPartition(leaderMetadataPartition).orElse(-1L) >= 0 || - topicBasedRlmm().receivedOffsetForPartition(followerMetadataPartition).orElse(-1L) >= 0) { + if (topicBasedRlmm().readOffsetForPartition(leaderMetadataPartition).orElse(-1L) >= 0 || + topicBasedRlmm().readOffsetForPartition(followerMetadataPartition).orElse(-1L) >= 0) { break; } }