> On July 21, 2014, 5:49 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 87 > > <https://reviews.apache.org/r/23567/diff/4/?file=635090#file635090line87> > > > > TxControlTypes would be clearer I think (also based on what I have seen > > so far in KAFKA-1523 rb - I think you intend this to be stored in the > > message key which it should not.)
Sure. Fixed! > On July 21, 2014, 5:49 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 172 > > <https://reviews.apache.org/r/23567/diff/4/?file=635090#file635090line172> > > > > Why does it need to be ordered? You could just use the groupedBy > > function. Originally the order is needed, because when a broker receives a transactionRequest, it appends the request to the first topicPartition in the txPartitions. Based on your other comments, I have batched the transactionRequest sent to the same broker. Now this order is not needed. > On July 21, 2014, 5:49 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 181 > > <https://reviews.apache.org/r/23567/diff/4/?file=635090#file635090line181> > > > > What does this method do? I have further comments on this in rb for > > KAFKA-1523 which I'm doing in parallel. This method return the first topicPartition in the txPartitions. The broker will append request to this partition. Now I have batched the transactionRequest sent to the same broker. This function is no longer needed. > On July 21, 2014, 5:49 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionResponse.scala, line 36 > > <https://reviews.apache.org/r/23567/diff/4/?file=635091#file635091line36> > > > > Would it make sense to have a per-partition error-response? E.g., after > > a prepare-commit/abort: if a transaction spans a lot of partitions and one > > of those brokers goes down the coordinator would only need to retry a > > commit/abort for that broker. Although the alternative is to just resend > > all the txcontrol messages to all the brokers. > > Sure! After batching the transactionRequest sent to the same broker, I have also updated transactionResponse to have a per-partition error-response. > On July 21, 2014, 5:49 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 41 > > <https://reviews.apache.org/r/23567/diff/4/?file=635090#file635090line41> > > > > Method names should ideally not start with a capital. > > copyTransactionToTxId could be a reasonable name. My bad. I mistake this function as TransactionRequest constructor. > On July 21, 2014, 5:49 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 42 > > <https://reviews.apache.org/r/23567/diff/4/?file=635090#file635090line42> > > > > Case classes have a copy method. i.e., you can instead do: > > oldTxRequest.copy(requestInfo = oldTxRequest.requestInfo.copy(txId = > > newTxId)) > > > > Finally, since it seems this is only used in one place in the > > coordinator and given the brevity of the above copy we can inline it there > > and not expose this at the object level. > > > > Cool. Thanks! > On July 21, 2014, 5:49 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 1 > > <https://reviews.apache.org/r/23567/diff/4/?file=635090#file635090line1> > > > > Can you add these new request/responses to the request-response > > serialization/deserialization test? Sure! - Dong ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23567/#review48259 ----------------------------------------------------------- On July 22, 2014, 11:43 p.m., Dong Lin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23567/ > ----------------------------------------------------------- > > (Updated July 22, 2014, 11:43 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1522 > https://issues.apache.org/jira/browse/KAFKA-1522 > > > Repository: kafka > > > Description > ------- > > KAFKA-1522 Tansactional messaging request/response definitions > > > Diffs > ----- > > core/src/main/scala/kafka/api/RequestKeys.scala > fbfc9d3aeaffed4ca85902125fcc1050086835db > core/src/main/scala/kafka/api/TransactionRequest.scala PRE-CREATION > core/src/main/scala/kafka/api/TransactionResponse.scala PRE-CREATION > core/src/main/scala/kafka/api/TxCoordinatorMetadataRequest.scala > PRE-CREATION > core/src/main/scala/kafka/api/TxCoordinatorMetadataResponse.scala > PRE-CREATION > core/src/main/scala/kafka/common/ErrorMapping.scala > 5559d26ba2b96059f719754a351fa4598ca8a70b > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > a2117b34c2ee3554602fe068eed0c90b075958c1 > > Diff: https://reviews.apache.org/r/23567/diff/ > > > Testing > ------- > > > Thanks, > > Dong Lin > >
