You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "showuon (via GitHub)" <gi...@apache.org> on 2023/02/24 01:15:17 UTC

[GitHub] [kafka] showuon opened a new pull request, #13297: Kafka-14743: update request metrics after callback

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

   Currently, the `MessageConversionsTimeMs` in fetch request will not get updated because the request metrics is recorded BEFORE the messageConversions metrics value updated. That means, even if we updated the `messageConversions` metrics value, the request metrics will never reflect the update. This patch fixes it by updating the request metric after callback completed, so that the `messageConversions` metric value can be updated correctly.
   
   ### 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] showuon commented on pull request #13297: Kafka-14743: update request metrics after callback

Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13297:
URL: https://github.com/apache/kafka/pull/13297#issuecomment-1442657395

   @chia7712 , please take a look. Thanks.


-- 
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 merged pull request #13297: Kafka-14743: update request metrics after callback

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


-- 
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] chia7712 commented on a diff in pull request #13297: Kafka-14743: update request metrics after callback

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


##########
core/src/test/scala/unit/kafka/server/FetchRequestDownConversionConfigTest.scala:
##########
@@ -222,9 +225,14 @@ class FetchRequestDownConversionConfigTest extends BaseRequestTest {
     } else {
       assertEquals(Errors.UNSUPPORTED_VERSION, error(partitionWithDownConversionDisabled))
     }
+
     TestUtils.waitUntilTrue(() => TestUtils.metersCount(BrokerTopicStats.FetchMessageConversionsPerSec) > initialFetchMessageConversionsPerSec,
       s"The `FetchMessageConversionsPerSec` metric count is not incremented after 5 seconds. " +
       s"init: $initialFetchMessageConversionsPerSec final: ${TestUtils.metersCount(BrokerTopicStats.FetchMessageConversionsPerSec)}", 5000)
+
+    TestUtils.waitUntilTrue(() => TestUtils.metersCount(fetchMessageConversionsTimeMsMetricName) > initialFetchMessageConversionsTimeMs,
+      s"The `MessageConversionsTimeMs` in fetch request metric count is not incremented after 5 seconds. " +
+      s"init: $initialFetchMessageConversionsTimeMs final: ${TestUtils.metersCount(fetchMessageConversionsTimeMsMetricName)}", 5000)

Review Comment:
   Could you add tea for `TemporaryMemoryBytes`? It seems to me `TemporaryMemoryBytes` also get fixed by this patch .



-- 
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] divijvaidya commented on a diff in pull request #13297: Kafka-14743: update request metrics after callback

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


##########
core/src/test/scala/unit/kafka/server/FetchRequestDownConversionConfigTest.scala:
##########
@@ -163,7 +164,9 @@ class FetchRequestDownConversionConfigTest extends BaseRequestTest {
   }
 
   def testV1Fetch(isFollowerFetch: Boolean): Unit = {
+    val fetchMessageConversionsTimeMsMetricName = s"$MessageConversionsTimeMs,request=Fetch"

Review Comment:
   nit
   
   Is it possible to generate this string with some constants? e.g. we can use `RequestMetrics.MessageConversionsTimeMs`



-- 
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 #13297: Kafka-14743: update request metrics after callback

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


##########
core/src/test/scala/unit/kafka/server/FetchRequestDownConversionConfigTest.scala:
##########
@@ -163,7 +164,9 @@ class FetchRequestDownConversionConfigTest extends BaseRequestTest {
   }
 
   def testV1Fetch(isFollowerFetch: Boolean): Unit = {
+    val fetchMessageConversionsTimeMsMetricName = s"$MessageConversionsTimeMs,request=Fetch"

Review Comment:
   Yes, we already used the constants from `RequestMetrics.MessageConversionsTimeMs`. It's just the fetch/produce request is another tag name that needed to be filtered out. Thanks. 



##########
core/src/test/scala/unit/kafka/server/FetchRequestDownConversionConfigTest.scala:
##########
@@ -222,9 +225,14 @@ class FetchRequestDownConversionConfigTest extends BaseRequestTest {
     } else {
       assertEquals(Errors.UNSUPPORTED_VERSION, error(partitionWithDownConversionDisabled))
     }
+
     TestUtils.waitUntilTrue(() => TestUtils.metersCount(BrokerTopicStats.FetchMessageConversionsPerSec) > initialFetchMessageConversionsPerSec,
       s"The `FetchMessageConversionsPerSec` metric count is not incremented after 5 seconds. " +
       s"init: $initialFetchMessageConversionsPerSec final: ${TestUtils.metersCount(BrokerTopicStats.FetchMessageConversionsPerSec)}", 5000)
+
+    TestUtils.waitUntilTrue(() => TestUtils.metersCount(fetchMessageConversionsTimeMsMetricName) > initialFetchMessageConversionsTimeMs,
+      s"The `MessageConversionsTimeMs` in fetch request metric count is not incremented after 5 seconds. " +
+      s"init: $initialFetchMessageConversionsTimeMs final: ${TestUtils.metersCount(fetchMessageConversionsTimeMsMetricName)}", 5000)

Review Comment:
   Good point. Updated.



-- 
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 pull request #13297: Kafka-14743: update request metrics after callback

Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13297:
URL: https://github.com/apache/kafka/pull/13297#issuecomment-1445285392

   Failed tests are unrelated:
   ```
       Build / JDK 8 and Scala 2.12 / kafka.admin.DescribeConsumerGroupTest.testDescribeExistingGroupWithNoMembers()
       Build / JDK 8 and Scala 2.12 / kafka.server.KafkaServerKRaftRegistrationTest.[1] Type=ZK, Name=testRegisterZkBrokerInKraft, MetadataVersion=3.4-IV0, Security=PLAINTEXT
       Build / JDK 17 and Scala 2.13 / org.apache.kafka.clients.consumer.internals.EagerConsumerCoordinatorTest.testOutdatedCoordinatorAssignment()
       Build / JDK 17 and Scala 2.13 / org.apache.kafka.streams.integration.EosIntegrationTest.shouldWriteLatestOffsetsToCheckpointOnShutdown[exactly_once]
       Build / JDK 17 and Scala 2.13 / org.apache.kafka.streams.processor.internals.DefaultStateUpdaterTest.shouldPauseStandbyTaskAndNotTransitToUpdateStandbyAgain()
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.processor.internals.DefaultStateUpdaterTest.shouldPauseStandbyTaskAndNotTransitToUpdateStandbyAgain()
       Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.processor.internals.DefaultStateUpdaterTest.shouldPauseStandbyTaskAndNotTransitToUpdateStandbyAgain()
   ```


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