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/06/24 04:02:13 UTC
Review Request 22905: Patch for KAFKA-1477
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22905/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1477
https://issues.apache.org/jira/browse/KAFKA-1477
Repository: kafka
Description
-------
Add request and response for transaction control and its metadata
Diffs
-----
core/src/main/scala/kafka/api/RequestKeys.scala fbfc9d3aeaffed4ca85902125fcc1050086835db
core/src/main/scala/kafka/api/TransactionMetadataRequest.scala PRE-CREATION
core/src/main/scala/kafka/api/TransactionMetadataResponse.scala PRE-CREATION
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/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
Diff: https://reviews.apache.org/r/22905/diff/
Testing
-------
Thanks,
Dong Lin
Re: Review Request 22905: Patch for KAFKA-1477
Posted by Joel Koshy <jj...@gmail.com>.
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/RequestKeys.scala, line 36
> > <https://reviews.apache.org/r/22905/diff/1/?file=615398#file615398line36>
> >
> > Just wondering if TransactionMetadata is misleading - since we are actually inquiring about the transaction coordinator. That TransactionCoordinatorMetadata is a bit unwieldy though.
>
> Dong Lin wrote:
> When I named this variable, I followed the name of "consumerMetadataKey". The consumerMetadataKey is named after the originator, rather than "offsetManagerMetadataKey".
>
> If TransactionCoordinatorMetadata is too long, how about TxCoordinatorMetadataKey?
Yes - that's a good name. We should eventually look at whether it is reasonable to merge this with the ConsumerMetadataRequest and then call it "CoordinatorMetadataRequest" but we can do that later.
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionMetadataRequest.scala, line 60
> > <https://reviews.apache.org/r/22905/diff/1/?file=615399#file615399line60>
> >
> > typo in comment
>
> Dong Lin wrote:
> Excuse me.. But where is the typo?
Oh - it's just that it is still referring to the consumer (since this was probably based on the ConsumerMetadataRequest)
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionRequest.scala, line 42
> > <https://reviews.apache.org/r/22905/diff/1/?file=615401#file615401line42>
> >
> > We will most likely need to incorporate a generation as well to handle the single-writer requirement.
>
> Dong Lin wrote:
> I thought we will delay adding the feature for single-writer requirement. Should I add a field "genID" to the TransactionRequest now?
Either way should be fine. Maybe we can hold off and add it when we implement protection for the single-writer case.
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionRequest.scala, line 45
> > <https://reviews.apache.org/r/22905/diff/1/?file=615401#file615401line45>
> >
> > Thinking about this: wondering if we should remove this and have the broker always treat it with required acks -1
> >
> > i.e., why would a producer want to have transactions and then set this to anything other than -1
>
> Dong Lin wrote:
> One possible scenario is, the producer only wants atomicity and strict ordering among transactions, and does not care whether the whole transaction data is lost.
>
> What do you think?
I don't think you can guarantee atomicity with anything but acks=-1. Say, if acks = 1, if the producer commits, gets an ack but the control record is not replicated and the coordinator fails the transaction will be expired by the new coordinator.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22905/#review46576
-----------------------------------------------------------
On June 24, 2014, 2:02 a.m., Dong Lin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22905/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 2:02 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1477
> https://issues.apache.org/jira/browse/KAFKA-1477
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add request and response for transaction control and its metadata
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/RequestKeys.scala fbfc9d3aeaffed4ca85902125fcc1050086835db
> core/src/main/scala/kafka/api/TransactionMetadataRequest.scala PRE-CREATION
> core/src/main/scala/kafka/api/TransactionMetadataResponse.scala PRE-CREATION
> 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/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
>
> Diff: https://reviews.apache.org/r/22905/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dong Lin
>
>
Re: Review Request 22905: Patch for KAFKA-1477
Posted by Dong Lin <li...@gmail.com>.
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionMetadataRequest.scala, line 60
> > <https://reviews.apache.org/r/22905/diff/1/?file=615399#file615399line60>
> >
> > typo in comment
Excuse me.. But where is the typo?
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/RequestKeys.scala, line 36
> > <https://reviews.apache.org/r/22905/diff/1/?file=615398#file615398line36>
> >
> > Just wondering if TransactionMetadata is misleading - since we are actually inquiring about the transaction coordinator. That TransactionCoordinatorMetadata is a bit unwieldy though.
When I named this variable, I followed the name of "consumerMetadataKey". The consumerMetadataKey is named after the originator, rather than "offsetManagerMetadataKey".
If TransactionCoordinatorMetadata is too long, how about TxCoordinatorMetadataKey?
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionMetadataRequest.scala, line 65
> > <https://reviews.apache.org/r/22905/diff/1/?file=615399#file615399line65>
> >
> > same here
I forgot the change this comment after copying it from ConsumerMetadataRequest.scala.
I will update this comment to "return TransactionCoordinatorNotAvailable for all uncaught errors"
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionRequest.scala, line 42
> > <https://reviews.apache.org/r/22905/diff/1/?file=615401#file615401line42>
> >
> > We will most likely need to incorporate a generation as well to handle the single-writer requirement.
I thought we will delay adding the feature for single-writer requirement. Should I add a field "genID" to the TransactionRequest now?
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionRequest.scala, line 45
> > <https://reviews.apache.org/r/22905/diff/1/?file=615401#file615401line45>
> >
> > Thinking about this: wondering if we should remove this and have the broker always treat it with required acks -1
> >
> > i.e., why would a producer want to have transactions and then set this to anything other than -1
One possible scenario is, the producer only wants atomicity and strict ordering among transactions, and does not care whether the whole transaction data is lost.
What do you think?
> On June 26, 2014, 12:51 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/api/TransactionRequest.scala, line 89
> > <https://reviews.apache.org/r/22905/diff/1/?file=615401#file615401line89>
> >
> > Should probably append this only if details is true.
Yes I agree. Will add this to the code.
Thanks!
- Dong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22905/#review46576
-----------------------------------------------------------
On June 24, 2014, 2:02 a.m., Dong Lin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22905/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 2:02 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1477
> https://issues.apache.org/jira/browse/KAFKA-1477
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add request and response for transaction control and its metadata
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/RequestKeys.scala fbfc9d3aeaffed4ca85902125fcc1050086835db
> core/src/main/scala/kafka/api/TransactionMetadataRequest.scala PRE-CREATION
> core/src/main/scala/kafka/api/TransactionMetadataResponse.scala PRE-CREATION
> 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/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
>
> Diff: https://reviews.apache.org/r/22905/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dong Lin
>
>
Re: Review Request 22905: Patch for KAFKA-1477
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22905/#review46576
-----------------------------------------------------------
This is not terribly important since we will be moving over to the schema definitions soon, but we can also add test cases for these requests in RequestResponseSerializationTest
core/src/main/scala/kafka/api/RequestKeys.scala
<https://reviews.apache.org/r/22905/#comment82031>
Just wondering if TransactionMetadata is misleading - since we are actually inquiring about the transaction coordinator. That TransactionCoordinatorMetadata is a bit unwieldy though.
core/src/main/scala/kafka/api/TransactionMetadataRequest.scala
<https://reviews.apache.org/r/22905/#comment82249>
typo in comment
core/src/main/scala/kafka/api/TransactionMetadataRequest.scala
<https://reviews.apache.org/r/22905/#comment82250>
same here
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/22905/#comment82254>
We will most likely need to incorporate a generation as well to handle the single-writer requirement.
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/22905/#comment82251>
Thinking about this: wondering if we should remove this and have the broker always treat it with required acks -1
i.e., why would a producer want to have transactions and then set this to anything other than -1
core/src/main/scala/kafka/api/TransactionRequest.scala
<https://reviews.apache.org/r/22905/#comment82253>
Should probably append this only if details is true.
- Joel Koshy
On June 24, 2014, 2:02 a.m., Dong Lin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22905/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 2:02 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1477
> https://issues.apache.org/jira/browse/KAFKA-1477
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add request and response for transaction control and its metadata
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/api/RequestKeys.scala fbfc9d3aeaffed4ca85902125fcc1050086835db
> core/src/main/scala/kafka/api/TransactionMetadataRequest.scala PRE-CREATION
> core/src/main/scala/kafka/api/TransactionMetadataResponse.scala PRE-CREATION
> 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/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
>
> Diff: https://reviews.apache.org/r/22905/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dong Lin
>
>
Re: Review Request 22905: Patch for KAFKA-1477
Posted by Dong Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22905/
-----------------------------------------------------------
(Updated June 27, 2014, 4:34 p.m.)
Review request for kafka.
Repository: kafka
Description
-------
Add request and response for transaction control and its metadata
Diffs (updated)
-----
core/src/main/scala/kafka/api/RequestKeys.scala fbfc9d3
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 5559d26
Diff: https://reviews.apache.org/r/22905/diff/
Testing
-------
Thanks,
Dong Lin
Re: Review Request 22905: Patch for KAFKA-1477
Posted by Dong Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22905/
-----------------------------------------------------------
(Updated June 27, 2014, 1:22 a.m.)
Review request for kafka.
Repository: kafka
Description
-------
Add request and response for transaction control and its metadata
Diffs (updated)
-----
core/src/main/scala/kafka/api/RequestKeys.scala fbfc9d3
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 5559d26
Diff: https://reviews.apache.org/r/22905/diff/
Testing
-------
Thanks,
Dong Lin
Re: Review Request 22905: Patch for KAFKA-1477
Posted by Dong Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22905/
-----------------------------------------------------------
(Updated June 27, 2014, 1:20 a.m.)
Review request for kafka.
Repository: kafka
Description
-------
Add request and response for transaction control and its metadata
Diffs
-----
core/src/main/scala/kafka/api/RequestKeys.scala fbfc9d3aeaffed4ca85902125fcc1050086835db
core/src/main/scala/kafka/api/TransactionMetadataRequest.scala PRE-CREATION
core/src/main/scala/kafka/api/TransactionMetadataResponse.scala PRE-CREATION
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/common/ErrorMapping.scala 5559d26ba2b96059f719754a351fa4598ca8a70b
Diff: https://reviews.apache.org/r/22905/diff/
Testing
-------
Thanks,
Dong Lin