Kafka
  1. Kafka
  2. KAFKA-181

Log errors for unrecognized config options

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0
    • Component/s: core
    • Labels:

      Description

      Currently, unrecognized config options are silently ignored. Notably, if a config has a typo or if a deprecated config is used, then there is no warning issued and defaults are assumed. One can argue that the broker or a consumer or a producer with an unrecognized config option should not even be allowed to start up especially if defaults are silently assumed, but it would be good to at least log an error.

      1. kafka-181_v2.patch
        44 kB
        Jun Rao
      2. kafka_181_v1.patch
        14 kB
        Jun Rao

        Activity

        Hide
        Jay Kreps added a comment -

        Yes, please yes.

        I recommend we create a Config object that wraps java.util.Properties. It should include all the random Utils helpers we have for parsing ints and stuff. Whenever a get() is called for a property string we should record that property in a set. We can add a method that intersects the requested properties with the provided properties to get unused properties.

        This config can be used in KafkaConfig and other configs.

        As a side note, there are many places where we need to be able let the user provide pluggins that implement an interface. Examples are the EventHandler and Serializer interfaces in the producer, and you could imagine us making other things such as offset storage pluggable. One requirement to make this work is that it needs to be possible for the user to set properties for their plugin. For example to create an AvroSerializer you need to be able to pass in a schema.registry.url parameter which needs to get passed through unmolested to the AvroSerializerImpl to use. To enable the config objects like KafkaConfig that parse out their options should retain the original Config instance. The general contract for pluggins should be that they must provide a constructor that takes a Config so that these configs can be passed through.

        Show
        Jay Kreps added a comment - Yes, please yes. I recommend we create a Config object that wraps java.util.Properties. It should include all the random Utils helpers we have for parsing ints and stuff. Whenever a get() is called for a property string we should record that property in a set. We can add a method that intersects the requested properties with the provided properties to get unused properties. This config can be used in KafkaConfig and other configs. As a side note, there are many places where we need to be able let the user provide pluggins that implement an interface. Examples are the EventHandler and Serializer interfaces in the producer, and you could imagine us making other things such as offset storage pluggable. One requirement to make this work is that it needs to be possible for the user to set properties for their plugin. For example to create an AvroSerializer you need to be able to pass in a schema.registry.url parameter which needs to get passed through unmolested to the AvroSerializerImpl to use. To enable the config objects like KafkaConfig that parse out their options should retain the original Config instance. The general contract for pluggins should be that they must provide a constructor that takes a Config so that these configs can be passed through.
        Hide
        Jay Kreps added a comment -

        Here are a few other facilities it would be nice to have if we were adding a generic config helper class:

        1. It would also be nice to have a facility to support aliases for backwards compatibility. Say we introduce a property log.retention.size and later realize that is inconsistent with the other properties which explicitly give the units. It would be nice to change the name to log.retention.size.bytes as the official name but still retain the old name for compatability. Of course this can be done manually, but maybe could be done better in the helper class.
        2. It would be nice to require an official documentation field.
        3. It would be nice to have a way to dump out all configs used, their defaults, and their documentation strings as a way to automatically build the site documentation and keep it in sync.
        Show
        Jay Kreps added a comment - Here are a few other facilities it would be nice to have if we were adding a generic config helper class: It would also be nice to have a facility to support aliases for backwards compatibility. Say we introduce a property log.retention.size and later realize that is inconsistent with the other properties which explicitly give the units. It would be nice to change the name to log.retention.size.bytes as the official name but still retain the old name for compatability. Of course this can be done manually, but maybe could be done better in the helper class. It would be nice to require an official documentation field. It would be nice to have a way to dump out all configs used, their defaults, and their documentation strings as a way to automatically build the site documentation and keep it in sync.
        Hide
        Neha Narkhede added a comment -

        +1, these are good ideas

        Show
        Neha Narkhede added a comment - +1, these are good ideas
        Hide
        Joe Stein added a comment -

        +1

        Show
        Joe Stein added a comment - +1
        Hide
        Jun Rao added a comment -

        That sounds like a good idea. However, how do we know the config initialization has finished so that we can report unused properties?

        Show
        Jun Rao added a comment - That sounds like a good idea. However, how do we know the config initialization has finished so that we can report unused properties?
        Hide
        Jay Kreps added a comment -

        The assumption is that the properties are only used during initialization and are passed into other components as simple constructor args. I notice this isn't universally true, but is generally a good pattern so we could refactor the cases where it isn't true.

        So basically it should be safe to check at the end of kafka server startup and warn about any unused properties.

        Show
        Jay Kreps added a comment - The assumption is that the properties are only used during initialization and are passed into other components as simple constructor args. I notice this isn't universally true, but is generally a good pattern so we could refactor the cases where it isn't true. So basically it should be safe to check at the end of kafka server startup and warn about any unused properties.
        Hide
        Jun Rao added a comment -

        That's reasonable for broker configs. What about configs for clients?

        Show
        Jun Rao added a comment - That's reasonable for broker configs. What about configs for clients?
        Hide
        Jun Rao added a comment -

        Attach patch v1. Added a wrapper class VeririableProperties. Verification is done at the end of the constructor of each public-facing configuration class. It reports overridden property values and non-used property values.

        Show
        Jun Rao added a comment - Attach patch v1. Added a wrapper class VeririableProperties. Verification is done at the end of the constructor of each public-facing configuration class. It reports overridden property values and non-used property values.
        Hide
        Jay Kreps added a comment -

        Can we move the various Utils.get* into VerifiableProperties? This way you just say
        prop.getInt('prop.name')

        The only reason for all those utility functions was because Properties doesn't have them and I didn't know about implicits at that time.

        Show
        Jay Kreps added a comment - Can we move the various Utils.get* into VerifiableProperties? This way you just say prop.getInt('prop.name') The only reason for all those utility functions was because Properties doesn't have them and I didn't know about implicits at that time.
        Hide
        Jun Rao added a comment -

        Thanks for the review. Attach patch v2 that addresses the review comments. Also rebased.

        Show
        Jun Rao added a comment - Thanks for the review. Attach patch v2 that addresses the review comments. Also rebased.
        Hide
        Neha Narkhede added a comment -

        +1

        Show
        Neha Narkhede added a comment - +1
        Hide
        Jay Kreps added a comment -

        +1
        A couple nit picks:
        1. VerifiableProperties is a "generic" utility class, so I don't think it should contain getCompressionCodec() which is pretty application specific.
        2. It would be nice to put it in its own file kafka/utils/VerifiableProperties.scala

        Show
        Jay Kreps added a comment - +1 A couple nit picks: 1. VerifiableProperties is a "generic" utility class, so I don't think it should contain getCompressionCodec() which is pretty application specific. 2. It would be nice to put it in its own file kafka/utils/VerifiableProperties.scala
        Hide
        Jun Rao added a comment -

        Thanks for the review. Addressed the 2 nits. Committed to 0.8.

        Show
        Jun Rao added a comment - Thanks for the review. Addressed the 2 nits. Committed to 0.8.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development