You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "satishd (via GitHub)" <gi...@apache.org> on 2023/02/26 13:51:06 UTC
[GitHub] [kafka] satishd opened a new pull request, #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
satishd opened a new pull request, #13309:
URL: https://github.com/apache/kafka/pull/13309
MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
### 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] satishd commented on pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13309:
URL: https://github.com/apache/kafka/pull/13309#issuecomment-1455007789
> @satishd How is this related to moving `LogSegment`? It doesn't seem like that was updated at all?
Methods like the below have usages in `LogSegment` class. Those will be updated as part of [KAFKA-14481](https://issues.apache.org/jira/browse/KAFKA-14481) changes.
```
UnifiedLog.offsetIndexFile(dir, baseOffset, fileSuffix)
UnifiedLog.timeIndexFile(dir, baseOffset, fileSuffix)
UnifiedLog.transactionIndexFile(dir, baseOffset, fileSuffix)
UnifiedLog.logFile(dir, baseOffset, fileSuffix)
```
--
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] satishd merged pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd merged PR #13309:
URL: https://github.com/apache/kafka/pull/13309
--
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] ijuma commented on pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13309:
URL: https://github.com/apache/kafka/pull/13309#issuecomment-1454850372
@satishd How is this related to moving `LogSegment`? It doesn't seem like that was updated at all?
--
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] junrao commented on a diff in pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #13309:
URL: https://github.com/apache/kafka/pull/13309#discussion_r1119124556
##########
core/src/main/scala/kafka/log/LocalLog.scala:
##########
@@ -686,40 +685,6 @@ object LocalLog extends Logging {
s"${topicPartition.topic}-${topicPartition.partition}"
}
- /**
- * Construct an index file name in the given dir using the given base offset and the given suffix
- *
- * @param dir The directory in which the log will reside
- * @param offset The base offset of the log file
- * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
- */
- private[log] def offsetIndexFile(dir: File, offset: Long, suffix: String = ""): File =
- new File(dir, filenamePrefixFromOffset(offset) + IndexFileSuffix + suffix)
Review Comment:
Should we remove IndexFileSuffix in LocalLog now that it's moved to LogFileUtils?
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java:
##########
@@ -72,4 +92,99 @@ private static String filenamePrefixFromOffset(long offset) {
return nf.format(offset);
}
+ /**
+ * Construct a log file name in the given dir with the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File logFile(File dir, long offset) {
+ return logFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a log file name in the given dir with the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name (e.g. "", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File logFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + LOG_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct an index file name in the given dir using the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File offsetIndexFile(File dir, long offset) {
+ return offsetIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct an index file name in the given dir using the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File offsetIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct a time index file name in the given dir using the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File timeIndexFile(File dir, long offset) {
+ return timeIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a time index file name in the given dir using the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File timeIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + TIME_INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct a transaction index file name in the given dir using the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File transactionIndexFile(File dir, long offset) {
+ return transactionIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a transaction index file name in the given dir using the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File transactionIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + TXN_INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Returns the offset from the given file. The file name is of the form: {number}.{suffix}. This method extracts
+ * the number from the given file's name.
+ *
+ * @param file file with the offset information as part of its name.
+ * @return offset of the given file
+ */
+ public static Long offsetFromFile(File file) {
Review Comment:
Should we remove `offsetFromFile` from LocalLog?
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java:
##########
@@ -72,4 +92,99 @@ private static String filenamePrefixFromOffset(long offset) {
return nf.format(offset);
}
+ /**
+ * Construct a log file name in the given dir with the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File logFile(File dir, long offset) {
Review Comment:
Should we remove `logFile` in LocalLog?
--
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] satishd commented on pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13309:
URL: https://github.com/apache/kafka/pull/13309#issuecomment-1448524298
Thanks @junrao for the review. Addressed them with the latest commit/comments.
--
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] satishd commented on pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13309:
URL: https://github.com/apache/kafka/pull/13309#issuecomment-1445829945
Thanks @showuon for the review. Addressed them with the latest commit.
--
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] satishd commented on pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13309:
URL: https://github.com/apache/kafka/pull/13309#issuecomment-1449372613
Test failures do not seem to be related to this change, merging these changes to trunk.
--
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] ijuma commented on pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13309:
URL: https://github.com/apache/kafka/pull/13309#issuecomment-1454850030
I didn't see this until now. I actually think the right approach is to push some of these methods to the respective classes (eg if it's related to an index, the method should be in the index class). I starting working on a PR for that and will submit it later.
--
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] satishd commented on a diff in pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #13309:
URL: https://github.com/apache/kafka/pull/13309#discussion_r1120421628
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java:
##########
@@ -72,4 +92,99 @@ private static String filenamePrefixFromOffset(long offset) {
return nf.format(offset);
}
+ /**
+ * Construct a log file name in the given dir with the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File logFile(File dir, long offset) {
+ return logFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a log file name in the given dir with the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name (e.g. "", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File logFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + LOG_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct an index file name in the given dir using the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File offsetIndexFile(File dir, long offset) {
+ return offsetIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct an index file name in the given dir using the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File offsetIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct a time index file name in the given dir using the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File timeIndexFile(File dir, long offset) {
+ return timeIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a time index file name in the given dir using the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File timeIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + TIME_INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct a transaction index file name in the given dir using the given base offset.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File transactionIndexFile(File dir, long offset) {
+ return transactionIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a transaction index file name in the given dir using the given base offset and the given suffix.
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File transactionIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + TXN_INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Returns the offset from the given file. The file name is of the form: {number}.{suffix}. This method extracts
+ * the number from the given file's name.
+ *
+ * @param file file with the offset information as part of its name.
+ * @return offset of the given file
+ */
+ public static Long offsetFromFile(File file) {
Review Comment:
This is already removed from LocalLog. Are you asking whether to remove this method from here and use `offsetFromFileName` instead?
--
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] showuon commented on a diff in pull request #13309: MINOR Moved a few log segment util methods from LocalLog to LogFileUtils
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13309:
URL: https://github.com/apache/kafka/pull/13309#discussion_r1118327464
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java:
##########
@@ -72,4 +92,99 @@ private static String filenamePrefixFromOffset(long offset) {
return nf.format(offset);
}
+ /**
+ * Construct a log file name in the given dir with the given base offset and the given suffix
Review Comment:
This constructor doesn't provide `suffix`. Please update the java doc.
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java:
##########
@@ -72,4 +92,99 @@ private static String filenamePrefixFromOffset(long offset) {
return nf.format(offset);
}
+ /**
+ * Construct a log file name in the given dir with the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File logFile(File dir, long offset) {
+ return logFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a log file name in the given dir with the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name (e.g. "", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File logFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + LOG_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct an index file name in the given dir using the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File offsetIndexFile(File dir, long offset) {
+ return offsetIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct an index file name in the given dir using the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File offsetIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct a time index file name in the given dir using the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File timeIndexFile(File dir, long offset) {
+ return timeIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a time index file name in the given dir using the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File timeIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + TIME_INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct a transaction index file name in the given dir using the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File transactionIndexFile(File dir, long offset) {
+ return transactionIndexFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a transaction index file name in the given dir using the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name ("", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File transactionIndexFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + TXN_INDEX_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Returns the offset for the given file. The file name is of the form: {number}.{suffix}. This method extracts
Review Comment:
nit: Returns the offset for the given file. -> Return the offset from the given file.
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java:
##########
@@ -72,4 +92,99 @@ private static String filenamePrefixFromOffset(long offset) {
return nf.format(offset);
}
+ /**
+ * Construct a log file name in the given dir with the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ */
+ public static File logFile(File dir, long offset) {
+ return logFile(dir, offset, "");
+ }
+
+ /**
+ * Construct a log file name in the given dir with the given base offset and the given suffix
+ *
+ * @param dir The directory in which the log will reside
+ * @param offset The base offset of the log file
+ * @param suffix The suffix to be appended to the file name (e.g. "", ".deleted", ".cleaned", ".swap", etc.)
+ */
+ public static File logFile(File dir, long offset, String suffix) {
+ return new File(dir, filenamePrefixFromOffset(offset) + LOG_FILE_SUFFIX + suffix);
+ }
+
+ /**
+ * Construct an index file name in the given dir using the given base offset and the given suffix
Review Comment:
ditto, here and below
--
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