[GitHub] [kafka] divijvaidya commented on a diff in pull request #14057: KAFKA-15194-Prepend-Offset-as-Filename

[via GitHub]

divijvaidya commented on code in PR #14057:
URL: https://github.com/apache/kafka/pull/14057#discussion_r1269646588


##
storage/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogSegmentFileset.java:
##
@@ -59,9 +59,9 @@
  * the local tiered storage:
  *
  * 
- * / storage-directory / topic-partition-uuidBase64 / 
oAtiIQ95REujbuzNd_lkLQ.log
- *  . 
oAtiIQ95REujbuzNd_lkLQ.index
- *  . 
oAtiIQ95REujbuzNd_lkLQ.timeindex
+ * / storage-directory / topic-partition-uuidBase64 / 
startOffset-oAtiIQ95REujbuzNd_lkLQ.log

Review Comment:
   nit
   
   Please replace "startOffset" with dummy values.



##
storage/src/test/java/org/apache/kafka/server/log/remote/storage/LocalTieredStorageTest.java:
##
@@ -399,20 +403,21 @@ public Verifier(final LocalTieredStorage remoteStorage, 
final TopicIdPartition t
 this.topicIdPartition = requireNonNull(topicIdPartition);
 }
 
-private List expectedPaths(final RemoteLogSegmentId id) {
+private List expectedPaths(final RemoteLogSegmentMetadata 
metadata) {
 final String rootPath = getStorageRootDirectory();
 TopicPartition tp = topicIdPartition.topicPartition();
 final String topicPartitionSubpath = format("%s-%d-%s", 
tp.topic(), tp.partition(),
 topicIdPartition.topicId());
-final String uuid = id.id().toString();
+final String uuid = metadata.remoteLogSegmentId().id().toString();
+final long startOffset = metadata.startOffset();
 
 return Arrays.asList(
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.LOG_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.INDEX_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.TIME_INDEX_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.TXN_INDEX_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LEADER_EPOCH_CHECKPOINT.getSuffix()),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.PRODUCER_SNAPSHOT_FILE_SUFFIX)
+Paths.get(rootPath, topicPartitionSubpath, startOffset + 
"-" + uuid + LogFileUtils.LOG_FILE_SUFFIX),

Review Comment:
   > I don't think it will allow us to insert a uuid in the middle as part of 
the filename.
   
   Ack. I missed that.
   
   > maybe we should make LogFileUtils#filenamePrefixFromOffset(long offset) as 
a public method so that we can construct a real offset using this method. What 
do you think ?
   
   Yes please. Let's use that.
   
   



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] divijvaidya commented on a diff in pull request #14057: KAFKA-15194-Prepend-Offset-as-Filename

[via GitHub]

divijvaidya commented on code in PR #14057:
URL: https://github.com/apache/kafka/pull/14057#discussion_r1269189609


##
storage/src/test/java/org/apache/kafka/server/log/remote/storage/LocalTieredStorageTest.java:
##
@@ -399,20 +403,21 @@ public Verifier(final LocalTieredStorage remoteStorage, 
final TopicIdPartition t
 this.topicIdPartition = requireNonNull(topicIdPartition);
 }
 
-private List expectedPaths(final RemoteLogSegmentId id) {
+private List expectedPaths(final RemoteLogSegmentMetadata 
metadata) {
 final String rootPath = getStorageRootDirectory();
 TopicPartition tp = topicIdPartition.topicPartition();
 final String topicPartitionSubpath = format("%s-%d-%s", 
tp.topic(), tp.partition(),
 topicIdPartition.topicId());
-final String uuid = id.id().toString();
+final String uuid = metadata.remoteLogSegmentId().id().toString();
+final long startOffset = metadata.startOffset();
 
 return Arrays.asList(
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.LOG_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.INDEX_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.TIME_INDEX_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.TXN_INDEX_FILE_SUFFIX),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LEADER_EPOCH_CHECKPOINT.getSuffix()),
-Paths.get(rootPath, topicPartitionSubpath, uuid + 
LogFileUtils.PRODUCER_SNAPSHOT_FILE_SUFFIX)
+Paths.get(rootPath, topicPartitionSubpath, startOffset + 
"-" + uuid + LogFileUtils.LOG_FILE_SUFFIX),

Review Comment:
   Ideally, we want the test implementation to be as close to the actual log 
file implementation as possible. Considering that, could we use 
`LogFileUtils#logFile(File dir, long offset)` here? Same for index file names.



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org