You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "hudeqi (via GitHub)" <gi...@apache.org> on 2023/06/10 14:22:17 UTC
[GitHub] [kafka] hudeqi opened a new pull request, #13839: MINOR:Fill missing parameter annotations for some LogCleaner methods
hudeqi opened a new pull request, #13839:
URL: https://github.com/apache/kafka/pull/13839
When reading the LogCleaner-related code, it is found that some methods do not update and improve the parameter annotations in time, and it is hoped that added annotations will be helpful to read the relevant logic.
--
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] hudeqi commented on pull request #13839: MINOR:Fill missing parameter annotations for some LogCleaner methods
Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13839:
URL: https://github.com/apache/kafka/pull/13839#issuecomment-1591111368
Hello, could you help to review this PR? @jlprat
--
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] jlprat commented on a diff in pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on code in PR #13839:
URL: https://github.com/apache/kafka/pull/13839#discussion_r1236681971
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -803,6 +864,14 @@ private[log] class Cleaner(val id: Int,
growBuffers(maxSize)
}
+ /**
+ * Judge a batch should be discard by cleaned transaction state
+ *
+ * @param batch The batch of records to judge
Review Comment:
Yes, sorry, I did a typo.
--
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] hudeqi commented on pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13839:
URL: https://github.com/apache/kafka/pull/13839#issuecomment-1594953593
> Thanks for the PR @hudeqi. I think it's always good to improve JavaDocs or ScalaDocs.
>
> I was looking at the `LogCleaner.scala` file and I saw there are plenty of methods that are public which have only a very general documentation and they don't have any parameter annotation with documentation. For example `abortCleaning` in line 220 and some of the following methods.
>
> For the sake of completion, would you be up to adding the missing annotations to the methods that are public? Extra mile for all the ones that are package-log-protected (`private[log]`)
Hi, thanks for the comments, I have added the comments of the methods you mentioned, please help to review, thank you! @jlprat
--
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] jlprat commented on a diff in pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on code in PR #13839:
URL: https://github.com/apache/kafka/pull/13839#discussion_r1236682306
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -803,6 +864,14 @@ private[log] class Cleaner(val id: Int,
growBuffers(maxSize)
}
+ /**
+ * Judge a batch should be discard by cleaned transaction state
Review Comment:
You could also use "check" instead of "determine"
--
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] hudeqi commented on a diff in pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on code in PR #13839:
URL: https://github.com/apache/kafka/pull/13839#discussion_r1236681057
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -803,6 +864,14 @@ private[log] class Cleaner(val id: Int,
growBuffers(maxSize)
}
+ /**
+ * Judge a batch should be discard by cleaned transaction state
+ *
+ * @param batch The batch of records to judge
Review Comment:
mean 'check'?
--
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] jlprat merged pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat merged PR #13839:
URL: https://github.com/apache/kafka/pull/13839
--
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] jlprat commented on pull request #13839: MINOR:Fill missing parameter annotations for some LogCleaner methods
Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on PR #13839:
URL: https://github.com/apache/kafka/pull/13839#issuecomment-1591700252
Thanks for the PR @hudeqi. I think it's always good to improve JavaDocs or ScalaDocs.
I was looking at the `LogCleaner.scala` file and I saw there are plenty of methods that are public which have only a very general documentation and they don't have any parameter annotation with documentation. For example `abortCleaning` in line 220 and some of the following methods.
For the sake of completion, would you be up to adding the missing annotations to the methods that are public? Extra mile for all the ones that are package-log-protected (`private[log]`)
--
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] jlprat commented on pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on PR #13839:
URL: https://github.com/apache/kafka/pull/13839#issuecomment-1598437574
Hi @hudeqi, it's on my list, yes.
I'll try to review this either today or tomorrow.
--
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] hudeqi commented on pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13839:
URL: https://github.com/apache/kafka/pull/13839#issuecomment-1598433842
Hi, are you still following this pr? @jlprat
--
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] jlprat commented on a diff in pull request #13839: MINOR:Fill missing parameter annotations for LogCleaner methods
Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on code in PR #13839:
URL: https://github.com/apache/kafka/pull/13839#discussion_r1236520019
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -173,14 +174,25 @@ class LogCleaner(initialConfig: CleanerConfig,
}
}
+ /**
+ * Remove metrics when shutdown cleaner threads
Review Comment:
I find this sentence a bit confusing, I think keeping it short is more than enough:
```suggestion
* Remove metrics
```
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -173,14 +174,25 @@ class LogCleaner(initialConfig: CleanerConfig,
}
}
+ /**
+ * Remove metrics when shutdown cleaner threads
+ */
def removeMetrics(): Unit = {
LogCleaner.MetricNames.foreach(metricsGroup.removeMetric)
}
+ /**
+ * @return A set of configs that is reconfigurable in LogCleaner
+ */
override def reconfigurableConfigs: Set[String] = {
LogCleaner.ReconfigurableConfigs
}
+ /**
+ * Mainly validate the new cleaner threads num is reasonable
Review Comment:
I think we should avoid the use of `Mainly` in Javadoc (or Scaladoc). Having a "mainly" in there might imply that there are other things this method does, but are not being shared.
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -803,6 +864,14 @@ private[log] class Cleaner(val id: Int,
growBuffers(maxSize)
}
+ /**
+ * Judge a batch should be discard by cleaned transaction state
Review Comment:
I find "Judge" a bit odd in this situation.
```suggestion
* Determine if a batch should be discard by cleaned transaction state
```
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -811,6 +880,18 @@ private[log] class Cleaner(val id: Int,
transactionMetadata.onBatchRead(batch)
}
+ /**
+ * Judge a record should be retained
Review Comment:
Same as before
```suggestion
* Determine if a record should be retained
```
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -91,6 +91,7 @@ import scala.util.control.ControlThrowable
* @param initialConfig Initial configuration parameters for the cleaner. Actual config may be dynamically updated.
* @param logDirs The directories where offset checkpoints reside
* @param logs The pool of logs
+ * @param logDirFailureChannel The channel used to add offline log dirs that may occur when cleaning the log
Review Comment:
I think `occur` should be replaced with `be encountered`. But this seems to be a nitpick.
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -215,27 +230,37 @@ class LogCleaner(initialConfig: CleanerConfig,
/**
* Abort the cleaning of a particular partition, if it's in progress. This call blocks until the cleaning of
* the partition is aborted.
+ *
+ * @param topicPartition The topic and partition to be abort cleaning
*/
def abortCleaning(topicPartition: TopicPartition): Unit = {
cleanerManager.abortCleaning(topicPartition)
}
/**
* Update checkpoint file to remove partitions if necessary.
+ *
+ * @param dataDir The file object to be updated
Review Comment:
This is consistent with other JavaDocs in the code.
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -215,27 +230,37 @@ class LogCleaner(initialConfig: CleanerConfig,
/**
* Abort the cleaning of a particular partition, if it's in progress. This call blocks until the cleaning of
* the partition is aborted.
+ *
+ * @param topicPartition The topic and partition to be abort cleaning
Review Comment:
```suggestion
* @param topicPartition The topic and partition to abort cleaning
```
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -803,6 +864,14 @@ private[log] class Cleaner(val id: Int,
growBuffers(maxSize)
}
+ /**
+ * Judge a batch should be discard by cleaned transaction state
+ *
+ * @param batch The batch of records to judge
Review Comment:
```suggestion
* @param batch The batch of records to cehck
```
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -811,6 +880,18 @@ private[log] class Cleaner(val id: Int,
transactionMetadata.onBatchRead(batch)
}
+ /**
+ * Judge a record should be retained
+ *
+ * @param map The offset map(key=>offset) to use for cleaning segments
+ * @param retainDeletesForLegacyRecords Should tombstones (lower than version 2) and markers be retained while cleaning this segment
+ * @param batch The batch of records that the record belongs to
+ * @param record The record to judge
Review Comment:
```suggestion
* @param record The record to check
```
##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -215,27 +230,37 @@ class LogCleaner(initialConfig: CleanerConfig,
/**
* Abort the cleaning of a particular partition, if it's in progress. This call blocks until the cleaning of
* the partition is aborted.
+ *
+ * @param topicPartition The topic and partition to be abort cleaning
*/
def abortCleaning(topicPartition: TopicPartition): Unit = {
cleanerManager.abortCleaning(topicPartition)
}
/**
* Update checkpoint file to remove partitions if necessary.
+ *
+ * @param dataDir The file object to be updated
+ * @param partitionToRemove The topicPartition to be removed, default none
*/
def updateCheckpoints(dataDir: File, partitionToRemove: Option[TopicPartition] = None): Unit = {
cleanerManager.updateCheckpoints(dataDir, partitionToRemove = partitionToRemove)
}
/**
- * alter the checkpoint directory for the topicPartition, to remove the data in sourceLogDir, and add the data in destLogDir
+ * Alter the checkpoint directory for the topicPartition, to remove the data in sourceLogDir, and add the data in destLogDir
Review Comment:
ScalaDoc allows to use backticks when you refer to variable names (or a paramers) in the doc.
```suggestion
* Alter the checkpoint directory for the `topicPartition`, to remove the data in `sourceLogDir`, and add the data in `destLogDir`
```
##########
core/src/main/scala/kafka/log/LogSegment.scala:
##########
@@ -140,7 +140,6 @@ class LogSegment private[log] (val log: FileRecords,
* @param largestTimestamp The largest timestamp in the message set.
* @param shallowOffsetOfMaxTimestamp The offset of the message that has the largest timestamp in the messages to append.
* @param records The log entries to append.
- * @return the physical position in the file of the appended records
Review Comment:
Good catch
--
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