You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "edoardocomar (via GitHub)" <gi...@apache.org> on 2024/05/31 09:32:16 UTC
[PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
edoardocomar opened a new pull request, #16151:
URL: https://github.com/apache/kafka/pull/16151
Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers as transaction timeout.
No transaction will be started with this timeout, but ReplicaManager.appendRecords uses this value as its timeout. Use REQUEST_TIMEOUT_MS_CONFIG like a regular producer append to allow for replication to take place.
Co-Authored-By: Adrian Preston <pr...@uk.ibm.com>
### 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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on PR #16151:
URL: https://github.com/apache/kafka/pull/16151#issuecomment-2147128804
Jenkins PR Jdk8 connect failure retested manually
```
./gradlew --no-daemon --no-build-cache cleantest connect:runtime:test --tests "ConnectorRestartApiIntegrationTest.testMultiWorkerRestartBothConnectorAndTasks"
```
`PASSED`
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #16151:
URL: https://github.com/apache/kafka/pull/16151#discussion_r1624740901
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4569,7 +4569,7 @@ public ListTransactionsResult listTransactions(ListTransactionsOptions options)
public FenceProducersResult fenceProducers(Collection<String> transactionalIds, FenceProducersOptions options) {
AdminApiFuture.SimpleAdminApiFuture<CoordinatorKey, ProducerIdAndEpoch> future =
FenceProducersHandler.newFuture(transactionalIds);
- FenceProducersHandler handler = new FenceProducersHandler(logContext);
+ FenceProducersHandler handler = new FenceProducersHandler(logContext, requestTimeoutMs);
Review Comment:
If the AdminClient is no longer waiting for the fence request to complete, does it make sense for the underlying append to continue?
If the AdminClient is willing to wait longer for the fence request to complete, should the underlying append be aborted early?
If someone wants to raise the timeout for this one operation, I don't think that we should require them to increase the client-global request.timeout.ms.
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #16151:
URL: https://github.com/apache/kafka/pull/16151#discussion_r1622743028
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4569,7 +4569,7 @@ public ListTransactionsResult listTransactions(ListTransactionsOptions options)
public FenceProducersResult fenceProducers(Collection<String> transactionalIds, FenceProducersOptions options) {
AdminApiFuture.SimpleAdminApiFuture<CoordinatorKey, ProducerIdAndEpoch> future =
FenceProducersHandler.newFuture(transactionalIds);
- FenceProducersHandler handler = new FenceProducersHandler(logContext);
+ FenceProducersHandler handler = new FenceProducersHandler(logContext, requestTimeoutMs);
Review Comment:
There's a requestTimeoutMs in options, can you use that if specified?
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #16151:
URL: https://github.com/apache/kafka/pull/16151#discussion_r1624103535
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4569,7 +4569,7 @@ public ListTransactionsResult listTransactions(ListTransactionsOptions options)
public FenceProducersResult fenceProducers(Collection<String> transactionalIds, FenceProducersOptions options) {
AdminApiFuture.SimpleAdminApiFuture<CoordinatorKey, ProducerIdAndEpoch> future =
FenceProducersHandler.newFuture(transactionalIds);
- FenceProducersHandler handler = new FenceProducersHandler(logContext);
+ FenceProducersHandler handler = new FenceProducersHandler(logContext, requestTimeoutMs);
Review Comment:
Hi @gharris1727 I'd appreciate if you can take a 2nd look or approve as IMHO without this small fix, exactly once MM2/connect is unusable in production
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on PR #16151:
URL: https://github.com/apache/kafka/pull/16151#issuecomment-2146353225
> If someone wants to raise the timeout for this one operation, I don't think that we should require them to increase the client-global request.timeout.ms.
I agree to that. Hopefully the commit addresses your concerns.
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #16151:
URL: https://github.com/apache/kafka/pull/16151#discussion_r1623013225
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4569,7 +4569,7 @@ public ListTransactionsResult listTransactions(ListTransactionsOptions options)
public FenceProducersResult fenceProducers(Collection<String> transactionalIds, FenceProducersOptions options) {
AdminApiFuture.SimpleAdminApiFuture<CoordinatorKey, ProducerIdAndEpoch> future =
FenceProducersHandler.newFuture(transactionalIds);
- FenceProducersHandler handler = new FenceProducersHandler(logContext);
+ FenceProducersHandler handler = new FenceProducersHandler(logContext, requestTimeoutMs);
Review Comment:
https://issues.apache.org/jira/browse/KAFKA-16047?focusedCommentId=17851019
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #16151:
URL: https://github.com/apache/kafka/pull/16151#discussion_r1624103535
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4569,7 +4569,7 @@ public ListTransactionsResult listTransactions(ListTransactionsOptions options)
public FenceProducersResult fenceProducers(Collection<String> transactionalIds, FenceProducersOptions options) {
AdminApiFuture.SimpleAdminApiFuture<CoordinatorKey, ProducerIdAndEpoch> future =
FenceProducersHandler.newFuture(transactionalIds);
- FenceProducersHandler handler = new FenceProducersHandler(logContext);
+ FenceProducersHandler handler = new FenceProducersHandler(logContext, requestTimeoutMs);
Review Comment:
Hi @gharris1727 I'd appreciate if you can take a 2nd look or approve as IMHO without this small fix, exactly once MM2/connect is unusable in production, so I'd like to have this in 3.8.0 and 3.7.1
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on PR #16151:
URL: https://github.com/apache/kafka/pull/16151#issuecomment-2147247109
merged in trunk, cherry-picked to 3.7.1 and 3.8.0. thanks @gharris1727
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar merged PR #16151:
URL: https://github.com/apache/kafka/pull/16151
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #16151:
URL: https://github.com/apache/kafka/pull/16151#discussion_r1623011495
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4569,7 +4569,7 @@ public ListTransactionsResult listTransactions(ListTransactionsOptions options)
public FenceProducersResult fenceProducers(Collection<String> transactionalIds, FenceProducersOptions options) {
AdminApiFuture.SimpleAdminApiFuture<CoordinatorKey, ProducerIdAndEpoch> future =
FenceProducersHandler.newFuture(transactionalIds);
- FenceProducersHandler handler = new FenceProducersHandler(logContext);
+ FenceProducersHandler handler = new FenceProducersHandler(logContext, requestTimeoutMs);
Review Comment:
I looked at it, and the option timeoutMs is used already as an API timeout - ie to calc a deadline when retrying.
The requestTimeoutMs is instead used for a single append, so I'd actually prefer not to mix the two and leave the PR as is.
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #16151:
URL: https://github.com/apache/kafka/pull/16151#discussion_r1622769486
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4569,7 +4569,7 @@ public ListTransactionsResult listTransactions(ListTransactionsOptions options)
public FenceProducersResult fenceProducers(Collection<String> transactionalIds, FenceProducersOptions options) {
AdminApiFuture.SimpleAdminApiFuture<CoordinatorKey, ProducerIdAndEpoch> future =
FenceProducersHandler.newFuture(transactionalIds);
- FenceProducersHandler handler = new FenceProducersHandler(logContext);
+ FenceProducersHandler handler = new FenceProducersHandler(logContext, requestTimeoutMs);
Review Comment:
as an override to the AdminClient REQUEST_TIMEOUT_MS_CONFIG ?
The options doc says it's on override on API timeout
but yes we could use that too
--
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
Re: [PR] KAFKA-16047: Use REQUEST_TIMEOUT_MS_CONFIG in AdminClient.fenceProducers [kafka]
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on PR #16151:
URL: https://github.com/apache/kafka/pull/16151#issuecomment-2147303894
I think this also fixes https://issues.apache.org/jira/browse/KAFKA-16488
--
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