You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/12/07 22:17:44 UTC
[GitHub] [kafka] cmccabe opened a new pull request, #12964: MINOR: Introduce MetadataProvenance and ImageReWriter
cmccabe opened a new pull request, #12964:
URL: https://github.com/apache/kafka/pull/12964
Introduce MetadataProvenance to encapsulate the three-tuple of (offset, epoch, timestamp) that is associated with each MetadataImage, as well as each on-disk snapshot.
Remove offset and epoch tracking from MetadataDelta. We do not really need to know this information until we are creating the final MetadataImage object. Therefore, this bookkeeping should be done by the metadata loading code, not inside the delta code, like the other bookkeeping. This simplifies a lot of tests, as well as simplifying RecordTestUtils. Also introduce a builder for MetadataDelta.
Add ImageReWriter, an ImageWriter that applies records to a MetadataDelta. This is useful when you need to create a MetadataDelta object that holds the contents of a MetadataImage. This will be used in the new image loader code (coming soon).
Add ImageWriterOptionsTest to test ImageWriterOptions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12964: MINOR: Introduce MetadataProvenance and ImageReWriter
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12964:
URL: https://github.com/apache/kafka/pull/12964#discussion_r1043672488
##########
metadata/src/main/java/org/apache/kafka/image/MetadataProvenance.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.image;
+
+import org.apache.kafka.raft.OffsetAndEpoch;
+
+import java.util.Objects;
+
+
+/**
+ * Information about the source of a metadata image.
+ */
+public final class MetadataProvenance {
+ public static final MetadataProvenance EMPTY = new MetadataProvenance(-1L, 0, 0L);
Review Comment:
yes, that's fair. I suppose they should all be -1 because 0 implies that you have read and seen offset / epoch / timestamp 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #12964: MINOR: Introduce MetadataProvenance and ImageReWriter
Posted by GitBox <gi...@apache.org>.
cmccabe merged PR #12964:
URL: https://github.com/apache/kafka/pull/12964
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a diff in pull request #12964: MINOR: Introduce MetadataProvenance and ImageReWriter
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12964:
URL: https://github.com/apache/kafka/pull/12964#discussion_r1043452505
##########
metadata/src/main/java/org/apache/kafka/image/MetadataProvenance.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.image;
+
+import org.apache.kafka.raft.OffsetAndEpoch;
+
+import java.util.Objects;
+
+
+/**
+ * Information about the source of a metadata image.
+ */
+public final class MetadataProvenance {
+ public static final MetadataProvenance EMPTY = new MetadataProvenance(-1L, 0, 0L);
+
+ private final long offset;
+ private final int epoch;
+ private final long lastContainedLogTimeMs;
+
+ public MetadataProvenance(
+ long offset,
+ int epoch,
+ long lastContainedLogTimeMs
+ ) {
+ this.offset = offset;
+ this.epoch = epoch;
+ this.lastContainedLogTimeMs = lastContainedLogTimeMs;
+ }
+
+ public OffsetAndEpoch offsetAndEpoch() {
+ return new OffsetAndEpoch(offset, epoch);
+ }
+
+ public long offset() {
+ return offset;
+ }
+
+ public int epoch() {
+ return epoch;
+ }
+
+ public long lastContainedLogTimeMs() {
+ return lastContainedLogTimeMs;
+ }
+
+ public String snapshotName() {
Review Comment:
Is this used anywhere? Is it meant to match the name of the snapshot file?
##########
metadata/src/main/java/org/apache/kafka/image/MetadataVersionChange.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.image;
+
+import org.apache.kafka.server.common.MetadataVersion;
+
+import java.util.Objects;
+
+
+/**
+ * A change in the MetadataVersion.
+ */
+public final class MetadataVersionChange {
Review Comment:
Would it be useful to have methods like "isUpgrade" or "isDowngrade" here?
##########
metadata/src/main/java/org/apache/kafka/image/MetadataProvenance.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.image;
+
+import org.apache.kafka.raft.OffsetAndEpoch;
+
+import java.util.Objects;
+
+
+/**
+ * Information about the source of a metadata image.
+ */
+public final class MetadataProvenance {
+ public static final MetadataProvenance EMPTY = new MetadataProvenance(-1L, 0, 0L);
Review Comment:
We should probably make these sentinels the same as the initial values used in BrokerMetadataListener. That way, if someone read `provenance()` off the listener before we processed anything, it would equal EMPTY
##########
metadata/src/main/java/org/apache/kafka/image/MetadataDelta.java:
##########
@@ -152,19 +153,7 @@ public Optional<MetadataVersion> metadataVersionChanged() {
}
}
- public void read(long highestOffset, int highestEpoch, Iterator<List<ApiMessageAndVersion>> reader) {
- while (reader.hasNext()) {
- List<ApiMessageAndVersion> batch = reader.next();
- for (ApiMessageAndVersion messageAndVersion : batch) {
- replay(highestOffset, highestEpoch, messageAndVersion.message());
- }
- }
- }
-
- public void replay(long offset, int epoch, ApiMessage record) {
- highestOffset = offset;
- highestEpoch = epoch;
-
+ public void replay(ApiMessage record) {
Review Comment:
A side effect of this is that we won't know what offset a Delta includes until we apply it. Are there any cases where that might be an issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12964: MINOR: Introduce MetadataProvenance and ImageReWriter
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12964:
URL: https://github.com/apache/kafka/pull/12964#discussion_r1043673991
##########
metadata/src/main/java/org/apache/kafka/image/MetadataProvenance.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.image;
+
+import org.apache.kafka.raft.OffsetAndEpoch;
+
+import java.util.Objects;
+
+
+/**
+ * Information about the source of a metadata image.
+ */
+public final class MetadataProvenance {
+ public static final MetadataProvenance EMPTY = new MetadataProvenance(-1L, 0, 0L);
+
+ private final long offset;
+ private final int epoch;
+ private final long lastContainedLogTimeMs;
+
+ public MetadataProvenance(
+ long offset,
+ int epoch,
+ long lastContainedLogTimeMs
+ ) {
+ this.offset = offset;
+ this.epoch = epoch;
+ this.lastContainedLogTimeMs = lastContainedLogTimeMs;
+ }
+
+ public OffsetAndEpoch offsetAndEpoch() {
+ return new OffsetAndEpoch(offset, epoch);
+ }
+
+ public long offset() {
+ return offset;
+ }
+
+ public int epoch() {
+ return epoch;
+ }
+
+ public long lastContainedLogTimeMs() {
+ return lastContainedLogTimeMs;
+ }
+
+ public String snapshotName() {
Review Comment:
It is used in the new metadata loader, which is not part of this PR. It is the name that a snapshot with the given provenance (offset, epoch) would have. I will add a Javadoc comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12964: MINOR: Introduce MetadataProvenance and ImageReWriter
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12964:
URL: https://github.com/apache/kafka/pull/12964#discussion_r1043668840
##########
metadata/src/main/java/org/apache/kafka/image/MetadataDelta.java:
##########
@@ -152,19 +153,7 @@ public Optional<MetadataVersion> metadataVersionChanged() {
}
}
- public void read(long highestOffset, int highestEpoch, Iterator<List<ApiMessageAndVersion>> reader) {
- while (reader.hasNext()) {
- List<ApiMessageAndVersion> batch = reader.next();
- for (ApiMessageAndVersion messageAndVersion : batch) {
- replay(highestOffset, highestEpoch, messageAndVersion.message());
- }
- }
- }
-
- public void replay(long offset, int epoch, ApiMessage record) {
- highestOffset = offset;
- highestEpoch = epoch;
-
+ public void replay(ApiMessage record) {
Review Comment:
Offset and epoch are a bit silly in cases like when loading a snapshot, where all records have the same offset and epoch.
This information is better tracked by the metadata loader and if we need it, we always have it there. We also have other information such as the source of the records and so on, in the loader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org