You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "alok123t (via GitHub)" <gi...@apache.org> on 2023/05/27 09:56:33 UTC

[GitHub] [kafka] alok123t opened a new pull request, #13772: MINOR: Add helper util `Snapshots.lastContainedLogTimestamp`

alok123t opened a new pull request, #13772:
URL: https://github.com/apache/kafka/pull/13772

   This PR refactors the `lastContainedLogTimestamp` to the Snapshots class, for reusability
   Introduces `IdentitySerde` based on ByteBuffer, required for using RecordsSnapshotReader
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


-- 
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] jsancio merged pull request #13772: MINOR: Add helper util `Snapshots.lastContainedLogTimestamp`

Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio merged PR #13772:
URL: https://github.com/apache/kafka/pull/13772


-- 
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] jsancio commented on a diff in pull request #13772: MINOR: Add helper util `Snapshots.lastContainedLogTimestamp`

Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13772:
URL: https://github.com/apache/kafka/pull/13772#discussion_r1210417483


##########
raft/src/main/java/org/apache/kafka/snapshot/Snapshots.java:
##########
@@ -151,4 +155,24 @@ public static Path markForDelete(Path logDir, OffsetAndEpoch snapshotId) {
             );
         }
     }
+
+    public static long lastContainedLogTimestamp(RawSnapshotReader reader) {
+        try (RecordsSnapshotReader<ByteBuffer> recordsSnapshotReader =
+             RecordsSnapshotReader.of(
+                 reader,
+                 new IdentitySerde(),

Review Comment:
   `RecordSerde` objects are reusable. You can add an `INSTANCE` static field to `IndentitySerde` and use it here. Take a look at `MetadataRecordSerde` for an example.



##########
core/src/main/scala/kafka/raft/KafkaMetadataLog.scala:
##########
@@ -377,19 +375,7 @@ final class KafkaMetadataLog private (
    */
   private def readSnapshotTimestamp(snapshotId: OffsetAndEpoch): Option[Long] = {
     readSnapshot(snapshotId).asScala.map { reader =>
-      val recordsSnapshotReader = RecordsSnapshotReader.of(
-        reader,
-        recordSerde,

Review Comment:
   I think you should be able to remove `recordSerde` from the constructor for `KafkaMetadataLog`.



-- 
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] alok123t commented on a diff in pull request #13772: MINOR: Add helper util `Snapshots.lastContainedLogTimestamp`

Posted by "alok123t (via GitHub)" <gi...@apache.org>.
alok123t commented on code in PR #13772:
URL: https://github.com/apache/kafka/pull/13772#discussion_r1210872444


##########
raft/src/main/java/org/apache/kafka/snapshot/Snapshots.java:
##########
@@ -151,4 +155,24 @@ public static Path markForDelete(Path logDir, OffsetAndEpoch snapshotId) {
             );
         }
     }
+
+    public static long lastContainedLogTimestamp(RawSnapshotReader reader) {
+        try (RecordsSnapshotReader<ByteBuffer> recordsSnapshotReader =
+             RecordsSnapshotReader.of(
+                 reader,
+                 new IdentitySerde(),

Review Comment:
   done in https://github.com/apache/kafka/pull/13772/commits/b10d32c85c9047b15d355239c44da62d9921bb6d



-- 
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