Re: [PR] MINOR: AddPartitionsToTxnManager performance optimizations [kafka]

[via GitHub]

jolshan commented on PR #15454:
URL: https://github.com/apache/kafka/pull/15454#issuecomment-1998091660

   Hmm. I was rerunning just to confirm. 


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

Re: [PR] MINOR: AddPartitionsToTxnManager performance optimizations [kafka]

[via GitHub]

mimaison merged PR #15454:
URL: https://github.com/apache/kafka/pull/15454


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

Re: [PR] MINOR: AddPartitionsToTxnManager performance optimizations [kafka]

[via GitHub]

mimaison commented on PR #15454:
URL: https://github.com/apache/kafka/pull/15454#issuecomment-1998016477

   Test failures don't seem related (all passed locally), merging 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-unsubscr...@kafka.apache.org

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

Re: [PR] MINOR: AddPartitionsToTxnManager performance optimizations [kafka]

[via GitHub]

jolshan commented on PR #15454:
URL: https://github.com/apache/kafka/pull/15454#issuecomment-1986575283

   Can you check `testTransactionCoordinatorResolution()`?


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

Re: [PR] MINOR: AddPartitionsToTxnManager performance optimizations [kafka]

[via GitHub]

jolshan commented on code in PR #15454:
URL: https://github.com/apache/kafka/pull/15454#discussion_r1518416635


##
core/src/test/scala/unit/kafka/server/AddPartitionsToTxnManagerTest.scala:
##
@@ -98,19 +99,20 @@ class AddPartitionsToTxnManagerTest {
 when(partitionFor.apply(transactionalId1)).thenReturn(0)
 when(partitionFor.apply(transactionalId2)).thenReturn(1)
 when(partitionFor.apply(transactionalId3)).thenReturn(0)
-
when(metadataCache.getTopicMetadata(Set(Topic.TRANSACTION_STATE_TOPIC_NAME), 
config.interBrokerListenerName))
-  .thenReturn(Seq(
-new MetadataResponseData.MetadataResponseTopic()
-  .setName(Topic.TRANSACTION_STATE_TOPIC_NAME)
-  .setPartitions(List(
-new MetadataResponseData.MetadataResponsePartition()
-  .setPartitionIndex(0)
-  .setLeaderId(0),
-new MetadataResponseData.MetadataResponsePartition()
-  .setPartitionIndex(1)
-  .setLeaderId(1)
-  ).asJava)
-  ))
+when(metadataCache.getPartitionInfo(Topic.TRANSACTION_STATE_TOPIC_NAME, 0))

Review Comment:
   i wonder if we could make a helper method for all of these.



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

Re: [PR] MINOR: AddPartitionsToTxnManager performance optimizations [kafka]

[via GitHub]

splett2 commented on code in PR #15454:
URL: https://github.com/apache/kafka/pull/15454#discussion_r1511954682


##
core/src/main/scala/kafka/server/AddPartitionsToTxnManager.scala:
##
@@ -80,10 +81,9 @@ class AddPartitionsToTxnManager(
 topicPartitions: Seq[TopicPartition],
 callback: AddPartitionsToTxnManager.AppendCallback
   ): Unit = {
-val (error, node) = 
getTransactionCoordinator(partitionFor(transactionalId))
-
-if (error != Errors.NONE) {
-  callback(topicPartitions.map(tp => tp -> error).toMap)
+val coordinatorNode = 
getTransactionCoordinator(partitionFor(transactionalId))
+if (coordinatorNode.isEmpty) {
+  callback(topicPartitions.map(tp => tp -> 
Errors.COORDINATOR_NOT_AVAILABLE).toMap)

Review Comment:
   we only returned either of (COORDINATOR_NOT_AVAILABLE, NoNode) or (NONE, 
SomeNode). Returning an option and translating to the appropriate error in the 
caller seemed simpler.



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

[PR] MINOR: AddPartitionsToTxnManager performance optimizations [kafka]

[via GitHub]

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

   A few minor optimizations:
   1. Cache the interbroker listener name instead of computing it each time. 
The value of the interbroker listener name cannot change without a process 
restart.
   2. we're currently grabbing all partitions for the transaction state topic 
in getTransactionCoordinator. Instead, just query the partition we care about.
   
   ### 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-unsubscr...@kafka.apache.org

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