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 2021/07/31 01:11:33 UTC
[GitHub] [kafka] kowshik opened a new pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
kowshik opened a new pull request #11154:
URL: https://github.com/apache/kafka/pull/11154
This PR is a follow-up to In https://github.com/apache/kafka/pull/10280 we had refactored the Log layer introducing a new `kafka.log.LocalLog` class.
In this PR, I've renamed `kafka.log.Log` to `kafka.log.UnifiedLog`. With the advent of KIP-405 development, going forward the existing `Log` class would present a unified view of local and tiered log segments, so we rightly rename it to `UnifiedLog`. The motivation for this PR is also the same as outlined in this design document: https://docs.google.com/document/d/1dQJL4MCwqQJSPmZkVmVzshFZKuFy_bCPtubav4wBfHQ/edit.
--
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] kowshik commented on a change in pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#discussion_r680529799
##########
File path: core/src/main/scala/kafka/log/UnifiedLog.scala
##########
@@ -540,6 +542,13 @@ class Log(@volatile var logStartOffset: Long,
}
}, period = producerIdExpirationCheckIntervalMs, delay = producerIdExpirationCheckIntervalMs, unit = TimeUnit.MILLISECONDS)
+ // For compatibility, metrics are defined to be under `Log` class
+ override def metricName(name: String, tags: scala.collection.Map[String, String]): MetricName = {
+ val klass = getClass
+ val pkg = if (klass.getPackage == null) "" else klass.getPackage.getName
Review comment:
Addressed in 5a85097
--
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] ccding commented on a change in pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
ccding commented on a change in pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#discussion_r680288120
##########
File path: core/src/main/scala/kafka/log/UnifiedLog.scala
##########
@@ -540,6 +542,13 @@ class Log(@volatile var logStartOffset: Long,
}
}, period = producerIdExpirationCheckIntervalMs, delay = producerIdExpirationCheckIntervalMs, unit = TimeUnit.MILLISECONDS)
+ // For compatibility, metrics are defined to be under `Log` class
+ override def metricName(name: String, tags: scala.collection.Map[String, String]): MetricName = {
+ val klass = getClass
+ val pkg = if (klass.getPackage == null) "" else klass.getPackage.getName
Review comment:
nit: it seems better to do the following? maybe a different variable name after the change.
```suggestion
val klass = getClass.getPackage
val pkg = if (klass == null) "" else klass.getName
```
--
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 merged pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
junrao merged pull request #11154:
URL: https://github.com/apache/kafka/pull/11154
--
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] kowshik commented on pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#issuecomment-890270512
cc @junrao @ccding @satishd - this PR is ready for review.
--
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] kowshik commented on a change in pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#discussion_r683616272
##########
File path: core/src/main/scala/kafka/log/UnifiedLog.scala
##########
@@ -248,16 +250,16 @@ case object SnapshotGenerated extends LogStartOffsetIncrementReason {
* will be deleted to avoid ID conflicts upon re-upgrade.
*/
@threadsafe
-class Log(@volatile var logStartOffset: Long,
- private val localLog: LocalLog,
- brokerTopicStats: BrokerTopicStats,
- val producerIdExpirationCheckIntervalMs: Int,
- @volatile var leaderEpochCache: Option[LeaderEpochFileCache],
- val producerStateManager: ProducerStateManager,
- @volatile private var _topicId: Option[Uuid],
- val keepPartitionMetadataFile: Boolean) extends Logging with KafkaMetricsGroup {
+class UnifiedLog(@volatile var logStartOffset: Long,
+ private val localLog: LocalLog,
+ brokerTopicStats: BrokerTopicStats,
+ val producerIdExpirationCheckIntervalMs: Int,
+ @volatile var leaderEpochCache: Option[LeaderEpochFileCache],
+ val producerStateManager: ProducerStateManager,
+ @volatile private var _topicId: Option[Uuid],
+ val keepPartitionMetadataFile: Boolean) extends Logging with KafkaMetricsGroup {
- import kafka.log.Log._
+ import kafka.log.UnifiedLog._
Review comment:
Done.
--
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 change in pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#discussion_r683680502
##########
File path: core/src/main/scala/kafka/log/UnifiedLog.scala
##########
@@ -1738,12 +1747,12 @@ object Log extends Logging {
logDirFailureChannel: LogDirFailureChannel,
lastShutdownClean: Boolean = true,
topicId: Option[Uuid],
- keepPartitionMetadataFile: Boolean): Log = {
+ keepPartitionMetadataFile: Boolean): UnifiedLog = {
// create the log directory if it doesn't exist
Files.createDirectories(dir.toPath)
- val topicPartition = Log.parseTopicPartitionName(dir)
+ val topicPartition = UnifiedLog.parseTopicPartitionName(dir)
val segments = new LogSegments(topicPartition)
- val leaderEpochCache = Log.maybeCreateLeaderEpochCache(
+ val leaderEpochCache = UnifiedLog.maybeCreateLeaderEpochCache(
Review comment:
Should we change the logging prefix to UnifiedLog in line 1760 too?
--
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] kowshik commented on pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#issuecomment-895473431
@junrao Thanks for the review! I've fixed the build failure and uploaded 6d30c102a4816803f51c5b2c2350aa75b51238f2. We can wait for CI to pass.
--
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] kowshik commented on pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
kowshik commented on pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#issuecomment-895760044
@junrao I checked CI results. The build has passed and the failing tests don't look related to this PR.
--
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] kowshik commented on a change in pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#discussion_r684599225
##########
File path: core/src/main/scala/kafka/log/UnifiedLog.scala
##########
@@ -1738,12 +1747,12 @@ object Log extends Logging {
logDirFailureChannel: LogDirFailureChannel,
lastShutdownClean: Boolean = true,
topicId: Option[Uuid],
- keepPartitionMetadataFile: Boolean): Log = {
+ keepPartitionMetadataFile: Boolean): UnifiedLog = {
// create the log directory if it doesn't exist
Files.createDirectories(dir.toPath)
- val topicPartition = Log.parseTopicPartitionName(dir)
+ val topicPartition = UnifiedLog.parseTopicPartitionName(dir)
val segments = new LogSegments(topicPartition)
- val leaderEpochCache = Log.maybeCreateLeaderEpochCache(
+ val leaderEpochCache = UnifiedLog.maybeCreateLeaderEpochCache(
Review comment:
Done. Thanks for catching this!
--
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 change in pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#discussion_r682801773
##########
File path: core/src/main/scala/kafka/log/UnifiedLog.scala
##########
@@ -248,16 +250,16 @@ case object SnapshotGenerated extends LogStartOffsetIncrementReason {
* will be deleted to avoid ID conflicts upon re-upgrade.
*/
@threadsafe
-class Log(@volatile var logStartOffset: Long,
- private val localLog: LocalLog,
- brokerTopicStats: BrokerTopicStats,
- val producerIdExpirationCheckIntervalMs: Int,
- @volatile var leaderEpochCache: Option[LeaderEpochFileCache],
- val producerStateManager: ProducerStateManager,
- @volatile private var _topicId: Option[Uuid],
- val keepPartitionMetadataFile: Boolean) extends Logging with KafkaMetricsGroup {
+class UnifiedLog(@volatile var logStartOffset: Long,
+ private val localLog: LocalLog,
+ brokerTopicStats: BrokerTopicStats,
+ val producerIdExpirationCheckIntervalMs: Int,
+ @volatile var leaderEpochCache: Option[LeaderEpochFileCache],
+ val producerStateManager: ProducerStateManager,
+ @volatile private var _topicId: Option[Uuid],
+ val keepPartitionMetadataFile: Boolean) extends Logging with KafkaMetricsGroup {
- import kafka.log.Log._
+ import kafka.log.UnifiedLog._
Review comment:
Should we rename the logging prefix to UnifiedLog too?
--
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 change in pull request #11154: KAFKA-13068: Rename Log to UnifiedLog
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11154:
URL: https://github.com/apache/kafka/pull/11154#discussion_r682801773
##########
File path: core/src/main/scala/kafka/log/UnifiedLog.scala
##########
@@ -248,16 +250,16 @@ case object SnapshotGenerated extends LogStartOffsetIncrementReason {
* will be deleted to avoid ID conflicts upon re-upgrade.
*/
@threadsafe
-class Log(@volatile var logStartOffset: Long,
- private val localLog: LocalLog,
- brokerTopicStats: BrokerTopicStats,
- val producerIdExpirationCheckIntervalMs: Int,
- @volatile var leaderEpochCache: Option[LeaderEpochFileCache],
- val producerStateManager: ProducerStateManager,
- @volatile private var _topicId: Option[Uuid],
- val keepPartitionMetadataFile: Boolean) extends Logging with KafkaMetricsGroup {
+class UnifiedLog(@volatile var logStartOffset: Long,
+ private val localLog: LocalLog,
+ brokerTopicStats: BrokerTopicStats,
+ val producerIdExpirationCheckIntervalMs: Int,
+ @volatile var leaderEpochCache: Option[LeaderEpochFileCache],
+ val producerStateManager: ProducerStateManager,
+ @volatile private var _topicId: Option[Uuid],
+ val keepPartitionMetadataFile: Boolean) extends Logging with KafkaMetricsGroup {
- import kafka.log.Log._
+ import kafka.log.UnifiedLog._
Review comment:
Should we rename the logging prefix to UnifiedLog too?
--
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