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

Reply via email to