You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Dong Lin <li...@gmail.com> on 2014/08/06 06:28:12 UTC
Re: Review Request 23567: Patch for KAFKA-1522
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23567/
-----------------------------------------------------------
(Updated Aug. 6, 2014, 4:28 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 (updated)
-----
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
Re: Review Request 23567: Patch for KAFKA-1522
Posted by Dong Lin <li...@gmail.com>.
> 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
>
>
Re: Review Request 23567: Patch for KAFKA-1522
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23567/#review50081
-----------------------------------------------------------
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/23567/#comment87615>
Although this is "sort-of" a constructor it is a regular method so start with lower case. Also, maybe call it transactionRequestWithNewControl
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/23567/#comment87613>
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?
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/23567/#comment87616>
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?
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/23567/#comment87617>
Same as above - thought this was no longer required (see comments from previous review)
- Joel Koshy
On Aug. 6, 2014, 4:28 a.m., Dong Lin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23567/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2014, 4:28 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
>
>
Re: Review Request 23567: Patch for KAFKA-1522
Posted by Dong Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23567/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 6:37 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 (updated)
-----
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
Re: Review Request 23567: Patch for KAFKA-1522
Posted by Dong Lin <li...@gmail.com>.
> On Aug. 14, 2014, 11:15 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionRequest.scala, line 47
> > <https://reviews.apache.org/r/23567/diff/7/?file=657006#file657006line47>
> >
> > It would help to have comments here describing each type's role.
Please check the updated patch. Thanks!
- Dong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23567/#review50663
-----------------------------------------------------------
On Aug. 15, 2014, 6:37 p.m., Dong Lin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23567/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 6:37 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 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
>
>
Re: Review Request 23567: Patch for KAFKA-1522
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23567/#review50663
-----------------------------------------------------------
Ship it!
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/23567/#comment88514>
It would help to have comments here describing each type's role.
- Joel Koshy
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
>
>
Re: Review Request 23567: Patch for KAFKA-1522
Posted by Dong Lin <li...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-------
KAFKA-1522; Tansactional messaging request/response definitions
Diffs (updated)
-----
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