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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
