> On Aug. 8, 2014, 10:51 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 41 > > <https://reviews.apache.org/r/23567/diff/6/?file=653421#file653421line41> > > > > Although this is "sort-of" a constructor it is a regular method so > > start with lower case. Also, maybe call it transactionRequestWithNewControl
I am sorry. I should have fixed this issue, but somehow it is lost in the patch. I just doubled checked KAFKA-1522 and KAFKA-1523 and made sure all comments are addressed. BTW, I find some errors that are introduced when I rebase KAFKA-1522 to updated HEAD of transactional_messaging branch. I fixed them in the updated patch. > On Aug. 8, 2014, 10:51 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 142 > > <https://reviews.apache.org/r/23567/diff/6/?file=653421#file653421line142> > > > > From the comments in the earlier version of the patch we concluded that > > you did not need the order - can we just do a simple groupBy? Same as above. Sorry for the inconvenience. > On Aug. 8, 2014, 10:51 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 151 > > <https://reviews.apache.org/r/23567/diff/6/?file=653421#file653421line151> > > > > Same as above - thought this was no longer required (see comments from > > previous review) Same as above. > On Aug. 8, 2014, 10:51 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/api/TransactionRequest.scala, line 50 > > <https://reviews.apache.org/r/23567/diff/6/?file=653421#file653421line50> > > > > commit vs pre-commit: I think this is needed for the txcoordinator to > > broker instruction. i.e., there is a distinction between what is stored in > > the log and the inter-broker communcation. I'm wondering if we should > > separate these better. OTOH if we were to separate these they should > > ideally be consistent (i.e., same value) which is easier if they are all in > > one place - what do you think? I think I understand your point. Do you mean we merge these two into same value? Currently I keep distinct value for commit and pre-commit because, when KafkaApis handle transactionRequest, it has different behavior for commit vs. pre-commit request. Since a broker may be transaction manger and partition leader at the same time, I have to use keep commit and pre-commit separate. Does it make sense? Thanks! - Dong ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23567/#review50081 ----------------------------------------------------------- On Aug. 9, 2014, 4:21 a.m., Dong Lin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23567/ > ----------------------------------------------------------- > > (Updated Aug. 9, 2014, 4:21 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 > > > Diffs > ----- > > core/src/main/scala/kafka/api/RequestKeys.scala > c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 > 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 > cd16ced5465d098be7a60498326b2a98c248f343 > > Diff: https://reviews.apache.org/r/23567/diff/ > > > Testing > ------- > > > Thanks, > > Dong Lin > >
