MINOR: ignore unused configuration when ConsumerCoordinator is not constructed

[RivenSun2]

Following PR #11940, ignore unused config when ConsumerCoordinator is not constructed

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[RivenSun2]

Hi @guozhangwang @showuon
please help to review PR when available.
Thanks.

[guozhangwang]

Why the THROW_ON_FETCH_STABLE_OFFSET_UNSUPPORTED config is related to groupID?

[RivenSun2]

Because the value of groupId will affect whether to construct the consumerCoordinator,
Constructing consumerCoordinator will retrieve THROW_ON_FETCH_STABLE_OFFSET_UNSUPPORTED config.

[guozhangwang]

LGTM. Merged to trunk.

[C0urante]

@RivenSun2 Isn't this a correct message to log? These properties are unused if a group ID is supplied, aren't they? That lines up pretty well with the new log message wording of "supplied but are not used yet".

BTW, thanks for cleaning that stuff up into a single message--much nicer to read now!

[RivenSun2]

Hi @C0urante Thank you for your reply. I see what you mean.
What we now expect is that a configuration is supplied but not used due to special behavior inside KafkaClient, and we want to be able to ignore this configuration.
For example, the serialization and deserialization configuration is treated the same as the initialization of KafkaConsumer and KafkaProducer.
Thanks.

[RivenSun2]

In other words, for a configuration (including unknownConfig), if neither Kafka nor the user retrieves this configuration when the KafkaClient completes initialization, the log prompt will only be printed.

Kafka is responsible for retrieving all knownConfigs, just like the transactional.id configuration, it will be used whether the user has passed in transactional.id or not.

            String transactionalId = config.getString(ProducerConfig.TRANSACTIONAL_ID_CONFIG);

            this.clientId = config.getString(ProducerConfig.CLIENT_ID_CONFIG);

[C0urante]

If Kafka doesn't retrieve a config because it has no effect due to other configs, isn't it still valid to warn the user?

I agree that if there's special internal logic that causes Kafka not to call get for a config even though it's applicable for the given client/scenario, then we shouldn't log a warning. The case of (de)serializer properties in the consumer and producer fall into this category.

But if we're not calling get because of publicly-documented behavior, a warning could be useful in that case to clear up potential misunderstandings by the user of what the configs they passed in actually do. The case of configs related to offset commits like the auto.commit.interval.ms fall into this category when no group.id is specified.

Not sure which category that the internal internal.throw.on.fetch.stable.offset.unsupported property falls into to be honest, but since it appears to be intended for use exclusively by Kafka Streams and isn't publicly documented, we probably should keep hiding warning messages that mention it since it probably originates from Kafka Streams anyways and directly not from users.

[C0urante]

Also worth noting that if we really do want to disable all unused config warnings for all (public) configs that Kafka defines, we could probably do this automatically instead of on a case-by-case basis.

For example, in KafkaConsumer, we could modify the constructor:

public class KafkaConsumer<K, V> implements Consumer<K, V> {
    @SuppressWarnings("unchecked")
    KafkaConsumer(ConsumerConfig config, Deserializer<K> keyDeserializer, Deserializer<V> valueDeserializer) {
        try {
            // We don't use some properties in some cases; no need to log a warning to the user for them
            ConsumerConfig.configNames().forEach(config::ignore);
            // ...
        } catch (Throwable t) {
            // ...
        }
}

This would be easier to maintain and reduce noise in the code base related to config wrangling.

[RivenSun2]

Hi @C0urante Thanks for your reply, I understand what you mean.
I also agree to ignore all ignoreConfig together.

But take the two parameters of KafkaProducer as an example (there are many similar situations in KafkaConsumer):
enable.idempotence transactional.id

Regardless of whether the user has passed in these two parameters, they will always be used because of the internal behavior of kafka to retrieve these two configurations to do some logic.

Our current intention is to call the ignore method uniformly if the configuration is not retrieved due to the special behavior of Kafka.

The logUnused method may work better when users use custom plugins and configurations.

WDYT? @guozhangwang

Thanks.

[guozhangwang]

I agree with @RivenSun2 's rationale here. More specifically I think 1) we are not really trying to just ignore all configs that are not used; instead we just want to 2) not print the unnecessary warn log entry for those configs that are rightfully ignored by the library.

[C0urante]

@guozhangwang thanks, but it seems like the disagreement here is on what actually constitutes an unnecessary warning message. If someone believes their consumer is auto-committing when it's not, that seems worthy of a warning. Of course, this could be issued elsewhere and with more specificity, but not logging at all is probably a regression in this scenario, not an improvement.

[guozhangwang]

@C0urante Yes I agree this is a fair question to debate on, and I personally think that it could be very subjective --- as you can see in https://issues.apache.org/jira/browse/KAFKA-13689, some people may prefer "for those not picked configs since they are not needed, do not bother to log it" while others would say "please let me know". In addition, I think this is not a regression to not log an ignored config, e.g. in KafkaConsumer we have been ignoring other configs (like KEY_DESERIALIZER_CLASS_CONFIG, when the actual deserializer object is provided) without logging, so it is more seems to me to be aligned with the existing behavior.

All that being said, if we feel that the general rule of thumb should be "always let me know when some configs are ignored", then we can have this discussion and if outcome is yes we should change the behavior universally.

[C0urante]

Thanks Guozhang. I think the cost of logging warnings in cases like this is fairly low as users can and should adjust their configurations to not use nonsensical properties, and the benefit can be high in the event that a user is confused about client behavior. I do sympathize with concerns that the warning for an unused property may make it seem like the property is unconditionally unrecognized (i.e., not defined by a client at all) instead of conditionally unrecognized (i.e., not used because of other properties).

One alternative could be to use the newly-introduced ConnectUtils::ensureProperty or something similar to it (possibly one that logs a warning if any value for a specific property is given, regardless of whether it matches the default). This way, we could continue logging warnings for cases like these, but make it clear exactly why the property should not be included in the config.

Either way, I think the piecemeal logic introduced in this PR is suboptimal. Dedicating one line for every to-be-ignored property is unnecessary if we want to remove these warnings for all properties defined by a client; in that case, we can use the approach I described earlier, which will be easier to maintain and take up less space.

[guozhangwang]

Thanks @C0urante for your thoughts. I'd like to clarify one thing that, today users can pass in both defined and unknown config values, where the latter may be used in some plugin modules (e.g. Kafka Streams's partition assignor). Since the config.logUnused() is called at the end of the the client constructor, at that time the latter category of configs may not be retrieved yet, and that does not mean that they will never be retrieved later after the constructor. So the logging message: "... were supplied but are not used yet." is reasonable, as "by that time" we do not know if they are never used or not, and we cannot just call ignore on all these configs in order to not log them.

Now for the former case, generally we expect that by the time config.logUnused() is called, all defined configs should be retrieved. If the client does not retrieve them yet, AND users have specified values for those configs, it's debatable that we should let users know as a reminder that they can consider removing those overrides; whereas for those configs which are not overridden by users, we would not bother to let them know at all.

If we want to do that, I'd suggest we do it universally: i.e. for all cases, including the previous ignored cases like KEY_DESERIALIZER_CLASS_CONFIG. Maybe you can send out a discussion email in the community to ask for a consensus?

[C0urante]

@guozhangwang to be clear, nobody is advocating that we call ignore on everything. I was proposing that we call ignore on everything that's already defined, which is pretty clear if you read the code example I gave. Does that clear things up?

[guozhangwang]

AH yes, that's clear.

My concern was that it's assuming the defined properties should be all retrieved in the constructor (since the logUnused is called at the end of it). I think it's true for most clients --- at least I think in producer and consumer, but it may not be the case for streams.