gharris1727 commented on code in PR #15078:
URL: https://github.com/apache/kafka/pull/15078#discussion_r1438351308
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -7031,6 +7031,7 @@ public void testFenceProducers() throws Exception {
try (AdminClientUnitTestEnv env = mockClientEnv()) {
String transactionalId = "copyCat";
Node transactionCoordinator =
env.cluster().nodes().iterator().next();
+ final FenceProducersOptions options = new
FenceProducersOptions().timeoutMs(10000);
Review Comment:
This doesn't test the null-default case which is going to be more common,
and doesn't seem necessary for the correctness of the test either.
I think instead, we should change the `request -> request instanceof
InitProducerIdRequest` to assert that the increased timeout is used, instead of
the 1ms timeout. And maybe refactor/parameterize the test to try a
default-options and options with an explicit timeout, if we want to test both
branches.
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -4394,7 +4394,10 @@ 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);
+ if (options.timeoutMs() == null) {
+ options.timeoutMs(defaultApiTimeoutMs);
Review Comment:
I think it's best if we don't mutate the options if the user passed it in.
If someone were to log the options somewhere else, they would see our injected
default instead of the fact that no value was provided.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
