You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2023/01/19 00:38:18 UTC
[GitHub] [kafka] Gerrrr opened a new pull request, #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Gerrrr opened a new pull request, #13129:
URL: https://github.com/apache/kafka/pull/13129
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a diff in pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
hachikuji commented on code in PR #13129:
URL: https://github.com/apache/kafka/pull/13129#discussion_r1081878202
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -318,7 +318,8 @@ public class ProducerConfig extends AbstractConfig {
/** <code> transaction.timeout.ms </code> */
public static final String TRANSACTION_TIMEOUT_CONFIG = "transaction.timeout.ms";
- public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in ms that the transaction coordinator will wait for a transaction status update from the producer before proactively aborting the ongoing transaction." +
+ public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in milliseconds that a transaction will remain open before the coordinator proactively aborts it. " +
+ "The start of the transaction is set at the time that the first partition is added to it. " +
"If this value is larger than the transaction.max.timeout.ms setting in the broker, the request will fail with a <code>InvalidTxnTimeoutException</code> error.";
Review Comment:
nit: could we put `<code>` blocks around `transaction.max.timeout.ms` while we're in here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on a diff in pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
jolshan commented on code in PR #13129:
URL: https://github.com/apache/kafka/pull/13129#discussion_r1081661935
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -319,6 +319,7 @@ public class ProducerConfig extends AbstractConfig {
/** <code> transaction.timeout.ms </code> */
public static final String TRANSACTION_TIMEOUT_CONFIG = "transaction.timeout.ms";
public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in ms that the transaction coordinator will wait for a transaction status update from the producer before proactively aborting the ongoing transaction." +
+ "The transaction status update happens on the first producer send, on adding new partitions to the transaction, and on commit. " +
Review Comment:
Ah -- I think I also had this confusion since there are notions of expiration and timeout. But yes, looking at your links I see that timeout is affected by the start timestamp only, while expiration is affected by the other components mentioned in the current description (adding partitions etc).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a diff in pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
hachikuji commented on code in PR #13129:
URL: https://github.com/apache/kafka/pull/13129#discussion_r1081629522
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -319,6 +319,7 @@ public class ProducerConfig extends AbstractConfig {
/** <code> transaction.timeout.ms </code> */
public static final String TRANSACTION_TIMEOUT_CONFIG = "transaction.timeout.ms";
public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in ms that the transaction coordinator will wait for a transaction status update from the producer before proactively aborting the ongoing transaction." +
+ "The transaction status update happens on the first producer send, on adding new partitions to the transaction, and on commit. " +
Review Comment:
I think this configuration is a bit misleading as currently documented. It looks to me like the implementation computes the timeout from the start of the transaction (i.e. the first call to `AddPartitionsToTxn`). We use `txnStartTimestamp` for this purpose here: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala#L134. This value is only updated on the initial transition to the `Ongoing` state: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/coordinator/transaction/TransactionMetadata.scala#L331.
I'd suggest we rephrase this configuration to something like this:
> The maximum amount of time in milliseconds that a transaction will remain open before the coordinator proactively aborts. The start of the transaction is set at the time that the first partition is added to it. If this value is larger than the transaction.max.timeout.ms setting in the broker, the request will fail with a <code>InvalidTxnTimeoutException</code> error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Gerrrr commented on a diff in pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
Gerrrr commented on code in PR #13129:
URL: https://github.com/apache/kafka/pull/13129#discussion_r1081848092
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -319,6 +319,7 @@ public class ProducerConfig extends AbstractConfig {
/** <code> transaction.timeout.ms </code> */
public static final String TRANSACTION_TIMEOUT_CONFIG = "transaction.timeout.ms";
public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in ms that the transaction coordinator will wait for a transaction status update from the producer before proactively aborting the ongoing transaction." +
+ "The transaction status update happens on the first producer send, on adding new partitions to the transaction, and on commit. " +
Review Comment:
Thank you for the review! Updated in [420991c](https://github.com/apache/kafka/pull/13129/commits/420991c39c40a508ec6b7e87c3755597a67441e9).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a diff in pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
hachikuji commented on code in PR #13129:
URL: https://github.com/apache/kafka/pull/13129#discussion_r1081629522
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -319,6 +319,7 @@ public class ProducerConfig extends AbstractConfig {
/** <code> transaction.timeout.ms </code> */
public static final String TRANSACTION_TIMEOUT_CONFIG = "transaction.timeout.ms";
public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in ms that the transaction coordinator will wait for a transaction status update from the producer before proactively aborting the ongoing transaction." +
+ "The transaction status update happens on the first producer send, on adding new partitions to the transaction, and on commit. " +
Review Comment:
I think this configuration is a bit misleading as currently documented. It looks to me like the implementation computes the timeout from the start of the transaction (i.e. the first call to `AddPartitionsToTxn`). We use `txnStartTimestamp` for this purpose here: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala#L134. This value is only updated on the initial transition to the `Ongoing` state: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/coordinator/transaction/TransactionMetadata.scala#L331.
I'd suggest we rephrase this configuration to something like this:
> The maximum amount of time in milliseconds that a transaction will remain open before the coordinator proactively aborts. The start of the transaction is set at the time that the first partition is added to it. If this value is larger than the <code>transaction.max.timeout.ms</code> setting in the broker, the request will fail with a <code>InvalidTxnTimeoutException</code> error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Gerrrr commented on pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
Gerrrr commented on PR #13129:
URL: https://github.com/apache/kafka/pull/13129#issuecomment-1397815859
@hachikuji @jolshan I don't have Kafka committer privileges. Can you please commit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax merged pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
mjsax merged PR #13129:
URL: https://github.com/apache/kafka/pull/13129
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Gerrrr commented on a diff in pull request #13129: KAFKA-14638: Elaborate when transaction.timeout.ms resets
Posted by GitBox <gi...@apache.org>.
Gerrrr commented on code in PR #13129:
URL: https://github.com/apache/kafka/pull/13129#discussion_r1081881848
##########
clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java:
##########
@@ -318,7 +318,8 @@ public class ProducerConfig extends AbstractConfig {
/** <code> transaction.timeout.ms </code> */
public static final String TRANSACTION_TIMEOUT_CONFIG = "transaction.timeout.ms";
- public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in ms that the transaction coordinator will wait for a transaction status update from the producer before proactively aborting the ongoing transaction." +
+ public static final String TRANSACTION_TIMEOUT_DOC = "The maximum amount of time in milliseconds that a transaction will remain open before the coordinator proactively aborts it. " +
+ "The start of the transaction is set at the time that the first partition is added to it. " +
"If this value is larger than the transaction.max.timeout.ms setting in the broker, the request will fail with a <code>InvalidTxnTimeoutException</code> error.";
Review Comment:
Done in [a1119c1](https://github.com/apache/kafka/pull/13129/commits/a1119c16f55555d8c3cedf554d62e8112085b2f6)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org