mjsax commented on code in PR #23364: URL: https://github.com/apache/kafka/pull/23364#discussion_r3952459088
########## docs/getting-started/upgrade.md: ########## @@ -54,7 +54,7 @@ type: docs * Kafka Connect distributed workers now support the `internal.topics.automatic.creation.enable` configuration (default: `true`). When set to `false`, Connect will not automatically create internal topics (offset, config, status, and connector-specific offset topics) and will instead fail at startup if any of these topics are missing. A new `connect-internal-topics.sh` tool is also available for manually creating these topics. For further details, please refer to [KIP-1209](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1209:+Add+configuration+to+control+internal+topic+creation+in+Kafka+Connect). * Streams groups now support broker-side custom task assignors, registered via the new broker configuration `group.streams.assignors` and selected per group with the new group configuration `streams.assignor.name`. For further details, please refer to [KIP-1357](https://cwiki.apache.org/confluence/x/NoSnGQ). * Controllers can now be unregistered from the cluster metadata. A new `kafka-cluster.sh unregister-controller` command and a `--unregister` flag on `kafka-metadata-quorum.sh remove-controller` are provided, backed by the new `Admin#unregisterController` API and the new `UnregisterController` RPC. This introduces the error code `CONTROLLER_ID_NOT_REGISTERED` (136) and requires metadata version `4.4-IV2` (`IBP_4_4_IV2`). For further details, please refer to [KIP-1312](https://cwiki.apache.org/confluence/spaces/KAFKA/pages/406623954/KIP-1312+Support+unregistering+controllers). - * A new client configuration, `bootstrap.resolve.timeout.ms`, controls how DNS resolution of `bootstrap.servers` is performed. The default value of `0` preserves the historical synchronous behavior. Setting a positive value opts into asynchronous bootstrap resolution: the client constructor returns immediately, `NetworkClient` retries DNS resolution up to the configured timeout, and failures are surfaced later as `BootstrapResolutionExceptions` from subsequent API calls. This failure is unrecoverable: once raised, the exception is re-thrown by every API call until the client is closed and re-created after fixing the underlying DNS or `bootstrap.servers` configuration. Both asynchronous bootstrap resolution (enabled by setting `bootstrap.resolve.timeout.ms` to a positive value) and `BootstrapResolutionException` are evolving features and may undergo compatibility-breaking changes in a minor release. For further details, please refer to [KIP-909](https://cwiki.apache.org/confluence/ x/MJtbDg). + * A new client configuration, `bootstrap.resolve.timeout.ms`, controls how DNS resolution of `bootstrap.servers` is performed. The default value of `0` preserves the historical synchronous behavior. Setting a positive value opts into asynchronous bootstrap resolution: the client constructor returns immediately, `NetworkClient` retries DNS resolution up to the configured timeout, and failures are surfaced later as `BootstrapResolutionExceptions` from subsequent API calls. This failure is unrecoverable: once raised, the exception is re-thrown by every API call until the client is closed and re-created after fixing the underlying DNS or `bootstrap.servers` configuration. Both asynchronous bootstrap resolution (enabled by setting `bootstrap.resolve.timeout.ms` to a positive value) and `BootstrapResolutionException` are evolving features and may undergo compatibility-breaking changes in a minor release. For further details, please refer to [KIP-909](https://cwiki.apache.org/confluence/ x/MJtbDg). Note that Kafka Streams and Kafka Connect do not support asynchronous bootstrap resolution: for every client they create internally, `bootstrap.resolve.timeout.ms` is forced to `0` and a user-supplied positive value is ignored with a warning. Review Comment: ```suggestion * A new client configuration, `bootstrap.resolve.timeout.ms`, controls how DNS resolution of `bootstrap.servers` is performed. The default value of `0` preserves the historical synchronous behavior. Setting a positive value opts into asynchronous bootstrap resolution: the client constructor returns immediately, `NetworkClient` retries DNS resolution up to the configured timeout, and failures are surfaced later as `BootstrapResolutionExceptions` from subsequent API calls. This failure is unrecoverable: once raised, the exception is re-thrown by every API call until the client is closed and re-created after fixing the underlying DNS or `bootstrap.servers` configuration. Both asynchronous bootstrap resolution (enabled by setting `bootstrap.resolve.timeout.ms` to a positive value) and `BootstrapResolutionException` are evolving features and may undergo compatibility-breaking changes in a minor release. For further details, please refer to [KIP-909](https://cwiki.apache.org/confluenc e/x/MJtbDg). Note that Kafka Streams and Kafka Connect do not support asynchronous bootstrap resolution yet: for every client they create internally, `bootstrap.resolve.timeout.ms` is forced to `0` and a user-supplied positive value is ignored with a warning. ``` ########## streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java: ########## @@ -1926,6 +1939,7 @@ public Map<String, Object> getMainConsumerConfigs(final String groupId, final St final Map<String, Object> mainConsumerProps = originalsWithPrefix(MAIN_CONSUMER_PREFIX); checkIfUnexpectedUserSpecifiedClientConfig(mainConsumerProps, NON_CONFIGURABLE_CONSUMER_DEFAULT_CONFIGS); consumerProps.putAll(mainConsumerProps); + enforceSynchronousBootstrapResolution(consumerProps, "consumer"); Review Comment: Why do we make this call here? Should it not get folded into `checkIfUnexpectedUserSpecifiedClientConfig` from above? And the new config should be added to `NON_CONFIGURABLE_CONSUMER_DEFAULT_CONFIGS` (and similar sibling variables)? ########## docs/streams/upgrade-guide.md: ########## @@ -67,6 +67,8 @@ Since 2.6.0 release, Kafka Streams depends on a RocksDB version that requires Ma ## Streams API changes in 4.5.0 +Kafka Streams does not support the asynchronous bootstrap DNS resolution mode introduced by [KIP-909](https://cwiki.apache.org/confluence/x/MJtbDg). A user-supplied positive value, through any prefix form, is ignored with a warning. Review Comment: This should go into 4.4 section, not 4.5 section ########## streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java: ########## @@ -292,6 +292,33 @@ public void testGetMainConsumerConfigsWithMainConsumerOverriddenPrefix() { assertEquals("50", returnedProps.get(ConsumerConfig.MAX_POLL_RECORDS_CONFIG)); } + @Test + public void shouldEnforceSynchronousBootstrapResolutionForAllClientsByDefault() { + assertEquals(0L, streamsConfig.getMainConsumerConfigs(groupId, clientId, threadIdx).get(CommonClientConfigs.BOOTSTRAP_RESOLVE_TIMEOUT_MS_CONFIG)); + assertEquals(0L, streamsConfig.getRestoreConsumerConfigs(clientId).get(CommonClientConfigs.BOOTSTRAP_RESOLVE_TIMEOUT_MS_CONFIG)); + assertEquals(0L, streamsConfig.getGlobalConsumerConfigs(clientId).get(CommonClientConfigs.BOOTSTRAP_RESOLVE_TIMEOUT_MS_CONFIG)); + assertEquals(0L, streamsConfig.getProducerConfigs(clientId).get(CommonClientConfigs.BOOTSTRAP_RESOLVE_TIMEOUT_MS_CONFIG)); + assertEquals(0L, streamsConfig.getAdminConfigs(clientId).get(CommonClientConfigs.BOOTSTRAP_RESOLVE_TIMEOUT_MS_CONFIG)); + } + + @Test + public void shouldIgnoreUserSpecifiedBootstrapResolveTimeout() { Review Comment: Should we add a LogCapture verification, 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
