[GitHub] [kafka] guozhangwang commented on pull request #12041: MINOR: ignore unused configuration when ConsumerCoordinator is not constructed
guozhangwang commented on PR #12041: URL: https://github.com/apache/kafka/pull/12041#issuecomment-1126350779 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. -- 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
[GitHub] [kafka] guozhangwang commented on pull request #12041: MINOR: ignore unused configuration when ConsumerCoordinator is not constructed
guozhangwang commented on PR #12041: URL: https://github.com/apache/kafka/pull/12041#issuecomment-1117635812 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? -- 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
[GitHub] [kafka] guozhangwang commented on pull request #12041: MINOR: ignore unused configuration when ConsumerCoordinator is not constructed
guozhangwang commented on PR #12041: URL: https://github.com/apache/kafka/pull/12041#issuecomment-1108911987 @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. -- 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
[GitHub] [kafka] guozhangwang commented on pull request #12041: MINOR: ignore unused configuration when ConsumerCoordinator is not constructed
guozhangwang commented on PR #12041: URL: https://github.com/apache/kafka/pull/12041#issuecomment-1104256988 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. -- 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
[GitHub] [kafka] guozhangwang commented on pull request #12041: MINOR: ignore unused configuration when ConsumerCoordinator is not constructed
guozhangwang commented on PR #12041: URL: https://github.com/apache/kafka/pull/12041#issuecomment-1099741042 LGTM. Merged to trunk. -- 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
