KAFKA-6979: Add default.api.timeout.ms to KafkaConsumer (KIP-266) #5122
Conversation
| @@ -91,6 +91,9 @@ | |||
| + "elapses the client will resend the request if necessary or fail the request if " | |||
| + "retries are exhausted."; | |||
|
|
|||
| public static final String MAX_BLOCK_MS_CONFIG = "max.block.ms"; | |||
| public static final String MAX_BLOCK_MS_DOC = "The configuration controls the maximum time that the consumer will block for"; | |||
There was a problem hiding this comment.
This is CommonClientConfigs, so we should avoid consumer-specific documentation. The producer also has a max.block.ms config, so if we could come up with a generic way to document the behavior of both, it would make sense to write it here. Otherwise, I'd suggest documenting separately.
|
retest this please |
1 similar comment
|
retest this please |
There was a problem hiding this comment.
Thanks @dhruvilshah3 for the PR.
Current javadoc of the updated methods in this PR need to be modified for timeout behavior like it is done in other methods in this class.
- Change the description to cover timeout expiration along with success/error scenarios.
- Add
throwsjavadoc forTimeoutException
|
Also, can you update the upgrade notes? We mention KIP-266, but not the new configuration. |
dhruvilshah3
force-pushed
the
consumer-max-block
branch
from
dbe5803
to
5abbfb2
Compare
| @@ -218,6 +218,10 @@ | |||
| public static final String REQUEST_TIMEOUT_MS_CONFIG = CommonClientConfigs.REQUEST_TIMEOUT_MS_CONFIG; | |||
| private static final String REQUEST_TIMEOUT_MS_DOC = CommonClientConfigs.REQUEST_TIMEOUT_MS_DOC; | |||
|
|
|||
| /** <code>max.block.ms</code> */ | |||
| public static final String MAX_BLOCK_MS_CONFIG = "max.block.ms"; | |||
| public static final String MAX_BLOCK_MS_DOC = "The configuration controls the maximum time that the consumer will block for"; | |||
There was a problem hiding this comment.
The config name was chosen for consistency with the producer, but it is not an ideal name when considering the consumer in isolation. The consumer can block longer than this if a longer timeout is passed to any blocking API that accepts a timeout. Can we explain that this config is only used as the default timeout for operations which do not have an explicit timeout option? Similarly for the changes in the upgrade docs.
|
Made a pass over the PR, the change LGTM as well besides the config name. I'm still currently in favor of adding a new config than leveraging on |
| @@ -218,6 +218,10 @@ | |||
| public static final String REQUEST_TIMEOUT_MS_CONFIG = CommonClientConfigs.REQUEST_TIMEOUT_MS_CONFIG; | |||
| private static final String REQUEST_TIMEOUT_MS_DOC = CommonClientConfigs.REQUEST_TIMEOUT_MS_DOC; | |||
|
|
|||
| /** <code>max.block.ms</code> */ | |||
| public static final String DEFAULT_API_TIMEOUT_MS_CONFIG = "default.api.timeout.ms"; | |||
| public static final String DEFAULT_API_TIMEOUT_MS_DOC = "Specifies the timeout (in milliseconds) for consumer APIs that could block. This configuration is used as the default timeout for all consumer operations that do not explicitly accept a <code>timeout</code> option."; | |||
There was a problem hiding this comment.
nit: instead of timeout "option," how about "parameter" or "argument"?
dhruvilshah3
changed the title
max.block.ms to KafkaConsumer
dhruvilshah3
force-pushed
the
consumer-max-block
branch
from
02538ca
to
9b46267
Compare
…5122) Adds a configuration that specifies the default timeout for KafkaConsumer APIs that could block. This was introduced in KIP-266. Reviewers: Satish Duggana <satish.duggana@gmail.com>, Jason Gustafson <jason@confluent.io>
…pache#5122) Adds a configuration that specifies the default timeout for KafkaConsumer APIs that could block. This was introduced in KIP-266. Reviewers: Satish Duggana <satish.duggana@gmail.com>, Jason Gustafson <jason@confluent.io>
Adds a configuration that specifies the default timeout for KafkaConsumer APIs that could block.