C0urante commented on code in PR #14313: URL: https://github.com/apache/kafka/pull/14313#discussion_r1312323499
########## connect/runtime/src/test/java/org/apache/kafka/connect/util/TopicAdminTest.java: ########## @@ -532,19 +532,31 @@ public void retryEndOffsetsShouldWrapNonRetriableExceptionsWithConnectException( Set<TopicPartition> tps = Collections.singleton(tp1); Long offset = 1000L; Cluster cluster = createCluster(1, "myTopic", 1); + String bootstrapServers = "localhost:8121"; - try (final AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(new MockTime(10), cluster)) { + try (final AdminClientUnitTestEnv env = new AdminClientUnitTestEnv(new MockTime(), cluster)) { Map<TopicPartition, Long> offsetMap = new HashMap<>(); offsetMap.put(tp1, offset); env.kafkaClient().setNodeApiVersions(NodeApiVersions.create()); - env.kafkaClient().prepareResponse(prepareMetadataResponse(cluster, Errors.UNKNOWN_TOPIC_OR_PARTITION, Errors.NONE)); - Map<String, Object> adminConfig = new HashMap<>(); - adminConfig.put(AdminClientConfig.RETRY_BACKOFF_MS_CONFIG, "0"); - TopicAdmin admin = new TopicAdmin(adminConfig, env.adminClient()); - assertThrows(ConnectException.class, () -> { - admin.retryEndOffsets(tps, Duration.ofMillis(100), 1); - }); + // This error should be treated as non-retriable and cause TopicAdmin::retryEndOffsets to fail + env.kafkaClient().prepareResponse(prepareMetadataResponse(cluster, Errors.TOPIC_AUTHORIZATION_FAILED, Errors.NONE)); + // But, in case there's a bug in our logic, prepare a valid response afterward so that TopicAdmin::retryEndOffsets + // will return successfully if we retry (which should in turn cause this test to fail) + env.kafkaClient().prepareResponse(prepareMetadataResponse(cluster, Errors.NONE)); + env.kafkaClient().prepareResponse(listOffsetsResult(tp1, offset)); + + TopicAdmin admin = new TopicAdmin(bootstrapServers, env.adminClient()); Review Comment: Good point! If memory serves, I originally went with the `Object` parameter in the `TopicAdmin` constructor for ergonomic reasons: the configuration accepted by `Admin::create` is a `Map<String, Object>`, so it would be a simple matter of passing something like `adminConfig.get("bootstrap.servers")` as the first argument instead of having to do pesky null-sensitive `toString` operations. But I agree that this isn't especially elegant or, more importantly, safe: there are even cases (thankfully only in tests) where we passed the entire config object as the `bootstrapServers` argument instead of just a string. I've pushed a commit that refactors the `TopicAdmin` constructors and all applicable call sites to be more type-safe. If this is easy enough to review, LMK and we can include it in this PR. Otherwise, I'm happy to revert the commit, merge this PR, and then file a follow-up to clean up the `TopicAdmin` constructors. -- 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