----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23567/#review48259 -----------------------------------------------------------
core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84623> Can you add these new request/responses to the request-response serialization/deserialization test? core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84624> Method names should ideally not start with a capital. copyTransactionToTxId could be a reasonable name. core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84625> 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. core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84626> 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.) core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84627> Typo - change it to transactionRequest core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84628> Would prefer calling this txGroupId core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84629> Would prefer calling groupId txGroupId core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84630> Can we use a more generic type like Seq instead? core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84632> Why does it need to be ordered? You could just use the groupedBy function. core/src/main/scala/kafka/api/TransactionRequest.scala <https://reviews.apache.org/r/23567/#comment84633> What does this method do? I have further comments on this in rb for KAFKA-1523 which I'm doing in parallel. core/src/main/scala/kafka/api/TransactionResponse.scala <https://reviews.apache.org/r/23567/#comment84634> 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. core/src/main/scala/kafka/api/TxCoordinatorMetadataRequest.scala <https://reviews.apache.org/r/23567/#comment84635> txGroupId core/src/main/scala/kafka/common/ErrorMapping.scala <https://reviews.apache.org/r/23567/#comment84636> TxCoordinatorNotAvailableCode - Joel Koshy On July 18, 2014, 3:12 a.m., Dong Lin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23567/ > ----------------------------------------------------------- > > (Updated July 18, 2014, 3:12 a.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 (version 2) > > > 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 > > Diff: https://reviews.apache.org/r/23567/diff/ > > > Testing > ------- > > > Thanks, > > Dong Lin > >
