Kafka
  1. Kafka
  2. KAFKA-103

Make whitelist/blacklist mirror configs more consistent

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: 0.7
    • Component/s: None
    • Labels:
      None

      Description

      The blacklist config for kafka mirrors is a comma separated list of topics. However, the whitelist config is a comma-separated list of "topics:numthreads" pairs, which allows for a multi-threaded consumer in the mirror. It will be good to keep the two configs consistent in format. So, we can make the whitelist config a comma-separated list of topics and provide a config (say, kafka.mirror.consumer.threads) that will specify the number of threads to use for all topics in the whitelist (if present).

      1. kafka-103-patch.v1
        4 kB
        Joel Koshy
      2. kafka-103-incremental-v2.patch
        3 kB
        Joel Koshy

        Activity

        Hide
        Joel Koshy added a comment -

        Hmm.. looks like the mirror topics patch actually ignored the numthreads in the whitelist. In any event, this patch allows a global numthreads for the embedded consumer.

        Show
        Joel Koshy added a comment - Hmm.. looks like the mirror topics patch actually ignored the numthreads in the whitelist. In any event, this patch allows a global numthreads for the embedded consumer.
        Hide
        Joel Koshy added a comment -

        Sorry about deleting that.. I forgot to update the property in system test.

        Show
        Joel Koshy added a comment - Sorry about deleting that.. I forgot to update the property in system test.
        Hide
        Jun Rao added a comment -

        Thanks, Joel. Just committed this.

        Show
        Jun Rao added a comment - Thanks, Joel. Just committed this.
        Hide
        Neha Narkhede added a comment -

        The behavior of the mirror.topics.whitelist is backwards incompatible. If some users currently have a whitelist setup with the old format (topic1:numPartitions1, topic2:numPartitions2), it is ideal to just rename "embedded.consumer.topics" to "mirror.topics.whitelist". Instead, this change forces those users to change their white-listed topics list to strip off the number of partitions. It would be ideal if the number of partitions is just ignored, with a little warning message. That way the change is backwards compatible.

        If a user just renames "embedded.consumer.topics" to "mirror.topics.whitelist" and leaves the number of partitions, currently the corp replica behavior is to not mirror anything at all. That is unintuitive.

        Show
        Neha Narkhede added a comment - The behavior of the mirror.topics.whitelist is backwards incompatible. If some users currently have a whitelist setup with the old format (topic1:numPartitions1, topic2:numPartitions2), it is ideal to just rename "embedded.consumer.topics" to "mirror.topics.whitelist". Instead, this change forces those users to change their white-listed topics list to strip off the number of partitions. It would be ideal if the number of partitions is just ignored, with a little warning message. That way the change is backwards compatible. If a user just renames "embedded.consumer.topics" to "mirror.topics.whitelist" and leaves the number of partitions, currently the corp replica behavior is to not mirror anything at all. That is unintuitive.
        Hide
        Joel Koshy added a comment -

        Note that this is an incremental patch.

        That is a good point - i.e., we have told clients that we are changing the old whitelist config to mirror.topics.whitelist. If they simply do a rename then nothing is going to get mirrored, so I agree that backward-compatibility is important.

        btw, I think a better fix (that would also help KAFKA-104) is to disallow invalid topics, but it would be better to think about that more carefully.

        Show
        Joel Koshy added a comment - Note that this is an incremental patch. That is a good point - i.e., we have told clients that we are changing the old whitelist config to mirror.topics.whitelist. If they simply do a rename then nothing is going to get mirrored, so I agree that backward-compatibility is important. btw, I think a better fix (that would also help KAFKA-104 ) is to disallow invalid topics, but it would be better to think about that more carefully.
        Hide
        Jun Rao added a comment -

        I actually don't think that we need to make this backward compatible because the # of threads specified in the old format will be ignored in the new config. If we support the old format, users will assume that the number of consumer threads is in effect. It's better to make this incompatible so the users can realize there is a problem.

        Show
        Jun Rao added a comment - I actually don't think that we need to make this backward compatible because the # of threads specified in the old format will be ignored in the new config. If we support the old format, users will assume that the number of consumer threads is in effect. It's better to make this incompatible so the users can realize there is a problem.
        Hide
        Joel Koshy added a comment -

        At least per kafka-103-patch.v1 it is not totally ignored. So if the whitelist config contains: SomeTopic:1 then SomeTopic will not get mirrored because it will fail the isTopicAllowed filter in KafkaServerStartable. i.e., no topics will get mirrored.

        Show
        Joel Koshy added a comment - At least per kafka-103-patch.v1 it is not totally ignored. So if the whitelist config contains: SomeTopic:1 then SomeTopic will not get mirrored because it will fail the isTopicAllowed filter in KafkaServerStartable. i.e., no topics will get mirrored.
        Hide
        Neha Narkhede added a comment -

        Jun, even if we don't want to support the older format, the behavior of silently not mirroring any data is also not acceptable. What would be helpful is to either log it as a warning or throw an InvalidConfigException.

        Show
        Neha Narkhede added a comment - Jun, even if we don't want to support the older format, the behavior of silently not mirroring any data is also not acceptable. What would be helpful is to either log it as a warning or throw an InvalidConfigException.
        Hide
        Jun Rao added a comment -

        However, we haven't officially released anything that exposes whitelist in the config. We are just iterating in trunk.

        Show
        Jun Rao added a comment - However, we haven't officially released anything that exposes whitelist in the config. We are just iterating in trunk.
        Hide
        Jun Rao added a comment -

        In summary, I think it's simpler if we enforce a single format for any property value. In this case, to help users identify problems, we can probably log the whitelisted and blacklisted topics, if any.

        Show
        Jun Rao added a comment - In summary, I think it's simpler if we enforce a single format for any property value. In this case, to help users identify problems, we can probably log the whitelisted and blacklisted topics, if any.
        Hide
        Jay Kreps added a comment -

        Is this complete?

        Show
        Jay Kreps added a comment - Is this complete?
        Hide
        Jun Rao added a comment -

        I think the concerns raised here can be addressed with clear documentation on how mirroring is done.

        Show
        Jun Rao added a comment - I think the concerns raised here can be addressed with clear documentation on how mirroring is done.

          People

          • Assignee:
            Jun Rao
            Reporter:
            Joel Koshy
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development