Kafka
  1. Kafka
  2. KAFKA-648

Use uniform convention for naming properties keys

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0, 0.8.1
    • Component/s: None
    • Labels:
      None

      Description

      Currently, the convention that we seem to use to get a property value in *Config is as follows:

      val configVal = property.getType("config.val", ...) // dot is used to separate two words in the key and the first letter of second word is capitalized in configVal.

      We should use similar convention for groupId, consumerId, clientId, correlationId.

      This change will probably be backward non-compatible.

      1. configchanges-1.patch
        47 kB
        Sriram Subramanian
      2. configchanges-v2.patch
        108 kB
        Sriram Subramanian
      3. configchanges-v3.patch
        137 kB
        Sriram Subramanian
      4. configchanges-v4.patch
        138 kB
        Sriram Subramanian
      5. configchanges-v5.patch
        261 kB
        Sriram Subramanian
      6. configchanges-v6.patch
        283 kB
        Sriram Subramanian
      7. configchanges-v6-rebased.patch
        282 kB
        Sriram Subramanian

        Activity

        Jun Rao made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Jun Rao added a comment -

        Merged to trunk and resolved conflicts.

        Show
        Jun Rao added a comment - Merged to trunk and resolved conflicts.
        Jun Rao made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Jun Rao added a comment -

        Thanks for the last patch. +1. Committed to 0.8 after fixing clienid in ProducerPerformance.

        Show
        Jun Rao added a comment - Thanks for the last patch. +1. Committed to 0.8 after fixing clienid in ProducerPerformance.
        Hide
        Swapnil Ghike added a comment -

        Will it make sense to create a Utils function to get the value of logDirs and move the require(logDirs.size > 0) inside this function? Currently, that's the only require() call in KafkaConfig. This new function will probably be too specific to logDirs though.

        Show
        Swapnil Ghike added a comment - Will it make sense to create a Utils function to get the value of logDirs and move the require(logDirs.size > 0) inside this function? Currently, that's the only require() call in KafkaConfig. This new function will probably be too specific to logDirs though.
        Sriram Subramanian made changes -
        Attachment configchanges-v6-rebased.patch [ 12564516 ]
        Hide
        Sriram Subramanian added a comment -

        rebased

        Show
        Sriram Subramanian added a comment - rebased
        Sriram Subramanian made changes -
        Attachment configchanges-v6.patch [ 12564480 ]
        Hide
        Sriram Subramanian added a comment -
        • Updated the comments.
        • Did not remove log.dir as there are systems tests depending on it. Just used a default path instead of "".
        • Removed messageMaxSize from producer as it is no longer used.
        Show
        Sriram Subramanian added a comment - Updated the comments. Did not remove log.dir as there are systems tests depending on it. Just used a default path instead of "". Removed messageMaxSize from producer as it is no longer used.
        Hide
        Jun Rao added a comment -

        Thanks for patch v5. Some of the comments in those config files are outdated or are missing. Made another pass.

        50. SyncProducerConfig: max.message.size => max.message.bytes

        51. KafkaConfig:
        51.1 auto.create.topics => auto.create.topics.enable
        51.2 We probably should remove the support of "log.dir" and default "log.dirs" to /tmp/kafka-logs (defaulting to "" is confusing).

        52. ConsumerConfig: Fix the following grammar.
        "to immediate satisfy min.fetch.bytes" => "to immediately satisfy min.fetch.bytes"

        53. KafkaConfig:
        53.1 replicaLagTimeMaxMs: add the following comments
        //If a follower hasn't sent any fetch requests during this time, the leader will remove the follower from isr.
        53.2 replica.lag.max.bytes: The name is out dated since offsets are now logical, instead of physical. So we need to change this property to replica.lag.max.messages and add the following comment.
        //If the lag in messages between a leader and a follower exceeds this number, the leader will remove the follower from isr.
        We also need to change the input parameter name in maybeShrinkIsr() and getOutOfSyncReplicas() in Partition from bytes to messages too.
        53.3 auto.create.topics => auto.create.topics.enable
        "highwater mark" => "high watermark"

        54. ProducerConfig:
        54.1 messageSendMaxRetries: The comment is outdated. Let's change it to the following:
        //The leader may be unavailable transiently, which can fail the sending of a message. This property specifies the number of retries when such failures occur.
        54.2 retryBackoffMs: Let's add the following comment.
        //Before each retry, the producer refreshes the metadata of relevant topics. Since leader election takes a bit of time, this property specifies the amount of time that the producer waits before refreshing the metadata.

        Show
        Jun Rao added a comment - Thanks for patch v5. Some of the comments in those config files are outdated or are missing. Made another pass. 50. SyncProducerConfig: max.message.size => max.message.bytes 51. KafkaConfig: 51.1 auto.create.topics => auto.create.topics.enable 51.2 We probably should remove the support of "log.dir" and default "log.dirs" to /tmp/kafka-logs (defaulting to "" is confusing). 52. ConsumerConfig: Fix the following grammar. "to immediate satisfy min.fetch.bytes" => "to immediately satisfy min.fetch.bytes" 53. KafkaConfig: 53.1 replicaLagTimeMaxMs: add the following comments //If a follower hasn't sent any fetch requests during this time, the leader will remove the follower from isr. 53.2 replica.lag.max.bytes: The name is out dated since offsets are now logical, instead of physical. So we need to change this property to replica.lag.max.messages and add the following comment. //If the lag in messages between a leader and a follower exceeds this number, the leader will remove the follower from isr. We also need to change the input parameter name in maybeShrinkIsr() and getOutOfSyncReplicas() in Partition from bytes to messages too. 53.3 auto.create.topics => auto.create.topics.enable "highwater mark" => "high watermark" 54. ProducerConfig: 54.1 messageSendMaxRetries: The comment is outdated. Let's change it to the following: //The leader may be unavailable transiently, which can fail the sending of a message. This property specifies the number of retries when such failures occur. 54.2 retryBackoffMs: Let's add the following comment. //Before each retry, the producer refreshes the metadata of relevant topics. Since leader election takes a bit of time, this property specifies the amount of time that the producer waits before refreshing the metadata.
        Sriram Subramanian made changes -
        Attachment configchanges-v5.patch [ 12564296 ]
        Hide
        Sriram Subramanian added a comment -

        40 / 42/ 43
        Accepted the suggestions but handled them differently. Specifying max and min at the beginning will cause configs related to the same feature to not look similar. For Example,

        max.log.Index.size and log.roll.hours are both configs related to logs but end up looking different.

        Instead, the configs use the following format -

        ConfigName => ComponentName AnyString [Max/Min] [Unit]

        FeatureName => Name of the component/feature this config is used for. Example - log, replica, etc.

        AnyString => A string that represents what this config is used for

        Max/Min => Optional. Used if the config represents a max or min value. For example, replicaLagTimeMaxMs

        Unit => Optional. The unit of the value the config represents. For example, replicaLagMaxBytes for value specified in bytes.

        41 Removed the producer prefix in producer configs.

        John you may have to fix the json files once more to work with the new changes.

        Show
        Sriram Subramanian added a comment - 40 / 42/ 43 Accepted the suggestions but handled them differently. Specifying max and min at the beginning will cause configs related to the same feature to not look similar. For Example, max.log.Index.size and log.roll.hours are both configs related to logs but end up looking different. Instead, the configs use the following format - ConfigName => ComponentName AnyString [Max/Min] [Unit] FeatureName => Name of the component/feature this config is used for. Example - log, replica, etc. AnyString => A string that represents what this config is used for Max/Min => Optional. Used if the config represents a max or min value. For example, replicaLagTimeMaxMs Unit => Optional. The unit of the value the config represents. For example, replicaLagMaxBytes for value specified in bytes. 41 Removed the producer prefix in producer configs. John you may have to fix the json files once more to work with the new changes.
        Hide
        Jun Rao added a comment -

        Thanks for patch v4. Made another pass. Should we make the following changes?

        40. KafkaConfig:
        max.message.size => max.message.bytes
        socket.send.buffer => socket.send.buffer.bytes
        socket.receive.buffer => socket.receive.buffer.bytes
        log.segment.size => log.segment.bytes
        log.retention.size => log.retention.bytes
        log.flush.interval => log.flush.interval.messages
        log.default.flush.interval.ms => log.flush.interval.ms
        log.flush.intervals.ms.per.topic => log.flush.interval.ms.per.topic
        replica.socket.buffersize => replica.socket.receive.buffer.bytes
        replica.fetch.size => replica.max.fetch.bytes
        fetch.request.purgatory.purge.interval => fetch.purgatory.purge.interval.requests
        producer.request.purgatory.purge.interval => producer.purgatory.purge.interval.requests
        replica.max.lag.time.ms => max.replica.lag.time.ms
        replica.max.lag.bytes => max.replica.lag.bytes
        replica.fetch.max.wait.ms => max.replica.fetch.wait.ms
        replica.fetch.min.bytes => min.replica.fetch.bytes

        41. ProducerConfig: producer.retry.count. In other configs we have num.network.threads. To be consistent, shouldn't we use num.producer.retries?

        42. AsyncProducerConfig:
        queue.time => queue.time.ms
        buffer.size => socket.send.buffer.bytes

        43. ConsumerConfig:
        socket.buffer.size => socket.receive.buffer.bytes
        fetch.size => max.fetch.bytes

        Show
        Jun Rao added a comment - Thanks for patch v4. Made another pass. Should we make the following changes? 40. KafkaConfig: max.message.size => max.message.bytes socket.send.buffer => socket.send.buffer.bytes socket.receive.buffer => socket.receive.buffer.bytes log.segment.size => log.segment.bytes log.retention.size => log.retention.bytes log.flush.interval => log.flush.interval.messages log.default.flush.interval.ms => log.flush.interval.ms log.flush.intervals.ms.per.topic => log.flush.interval.ms.per.topic replica.socket.buffersize => replica.socket.receive.buffer.bytes replica.fetch.size => replica.max.fetch.bytes fetch.request.purgatory.purge.interval => fetch.purgatory.purge.interval.requests producer.request.purgatory.purge.interval => producer.purgatory.purge.interval.requests replica.max.lag.time.ms => max.replica.lag.time.ms replica.max.lag.bytes => max.replica.lag.bytes replica.fetch.max.wait.ms => max.replica.fetch.wait.ms replica.fetch.min.bytes => min.replica.fetch.bytes 41. ProducerConfig: producer.retry.count. In other configs we have num.network.threads. To be consistent, shouldn't we use num.producer.retries? 42. AsyncProducerConfig: queue.time => queue.time.ms buffer.size => socket.send.buffer.bytes 43. ConsumerConfig: socket.buffer.size => socket.receive.buffer.bytes fetch.size => max.fetch.bytes
        Hide
        John Fung added a comment -

        Tested v4 patch with KAFKA-688-v1.patch with the latest patch and it works fine.

        Show
        John Fung added a comment - Tested v4 patch with KAFKA-688 -v1.patch with the latest patch and it works fine.
        Sriram Subramanian made changes -
        Attachment configchanges-v4.patch [ 12564016 ]
        Hide
        Sriram Subramanian added a comment -

        Reverted the change from migrationtool/consumer.properties

        Show
        Sriram Subramanian added a comment - Reverted the change from migrationtool/consumer.properties
        Hide
        John Fung added a comment -

        Hi Sriram,

        The change in system_test/migration_tool_testsuite/config/migration_consumer.properties is not needed because the migration tool consumer is using 0.7 library and would not be aware of this naming convention change.

        Other than that, changes in the System Test part looks good.

        Show
        John Fung added a comment - Hi Sriram, The change in system_test/migration_tool_testsuite/config/migration_consumer.properties is not needed because the migration tool consumer is using 0.7 library and would not be aware of this naming convention change. Other than that, changes in the System Test part looks good.
        Sriram Subramanian made changes -
        Attachment configchanges-v3.patch [ 12563847 ]
        Hide
        Sriram Subramanian added a comment -
        • made a scan through ZKConfigs
        • There are many other properties that seem to be longer than these. I have fixed the two mentioned.
        • Removed the unused configs
        Show
        Sriram Subramanian added a comment - made a scan through ZKConfigs There are many other properties that seem to be longer than these. I have fixed the two mentioned. Removed the unused configs
        Hide
        John Fung added a comment - - edited

        The testcase_xxxx_properties.json files in System Test will need to be updated for the following changes:

        brokerid => broker.id
        log.file.size => log.segment.size
        groupid => group.id

        A separate JIRA KAFKA-688 is created for this task.

        Show
        John Fung added a comment - - edited The testcase_xxxx_properties.json files in System Test will need to be updated for the following changes: brokerid => broker.id log.file.size => log.segment.size groupid => group.id A separate JIRA KAFKA-688 is created for this task.
        Hide
        Jun Rao added a comment -

        Thanks for patch v2. I missed a few items in the previous review. Sorry about that. The following are some more comments.

        20. We need to standardize properties in ZKConfig too.

        21. KafkaConfig: The following names are pretty long.
        replica.fetch.max.wait.time.ms
        replica.fetch.min.expected.bytes
        Would it be better to change them to
        replica.fetch.max.wait.ms
        replica.fetch.min.bytes

        22. config/producer.properties: The following properties no longer exist.

        1. the callback handler for one or multiple events
          #callback.handler=
        1. properties required to initialize the callback handler
          #callback.handler.props=
        1. the handler for events
          #event.handler=
        1. properties required to initialize the event handler
          #event.handler.props=

        23. config/server.properties: The following property no longer exists.

        1. Overrides for for the default given by num.partitions on a per-topic basis
          #topic.partition.count.map=topic1:3, topic2:4
        Show
        Jun Rao added a comment - Thanks for patch v2. I missed a few items in the previous review. Sorry about that. The following are some more comments. 20. We need to standardize properties in ZKConfig too. 21. KafkaConfig: The following names are pretty long. replica.fetch.max.wait.time.ms replica.fetch.min.expected.bytes Would it be better to change them to replica.fetch.max.wait.ms replica.fetch.min.bytes 22. config/producer.properties: The following properties no longer exist. the callback handler for one or multiple events #callback.handler= properties required to initialize the callback handler #callback.handler.props= the handler for events #event.handler= properties required to initialize the event handler #event.handler.props= 23. config/server.properties: The following property no longer exists. Overrides for for the default given by num.partitions on a per-topic basis #topic.partition.count.map=topic1:3, topic2:4
        Sriram Subramanian made changes -
        Attachment configchanges-v2.patch [ 12563666 ]
        Hide
        Sriram Subramanian added a comment -

        1. Standardized usage of enable
        2. Fixed config names in SyncPRoducerConfigShared and AsyncProducerConfig
        3. Made the property changes in system_tests, perf

        Show
        Sriram Subramanian added a comment - 1. Standardized usage of enable 2. Fixed config names in SyncPRoducerConfigShared and AsyncProducerConfig 3. Made the property changes in system_tests, perf
        Hide
        Jun Rao added a comment -

        Thanks for the patch. Some comments:

        1. ConsumerConfig: We have both enable.shallow.iterator and auto.commit.enable. Need to standardize.

        2. We need to make a pass of the names in SyncProducerConfigShared and AsyncProducerConfig too.

        3. Could you change the affected property names in other directories (config/, system_tests/, examples/, perf/, contrib/) too?

        Show
        Jun Rao added a comment - Thanks for the patch. Some comments: 1. ConsumerConfig: We have both enable.shallow.iterator and auto.commit.enable. Need to standardize. 2. We need to make a pass of the names in SyncProducerConfigShared and AsyncProducerConfig too. 3. Could you change the affected property names in other directories (config/, system_tests/, examples/, perf/, contrib/) too?
        Sriram Subramanian made changes -
        Attachment configchanges-1.patch [ 12563174 ]
        Sriram Subramanian made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Fix Version/s 0.8 [ 12317244 ]
        Hide
        Sriram Subramanian added a comment -

        1. Ensured that all config names have uniform naming format
        2. Changed some config names to better reflect their usage

        I did not modify any time formats as part of this change. Let me know which configs need more fine grained time specifications.

        Show
        Sriram Subramanian added a comment - 1. Ensured that all config names have uniform naming format 2. Changed some config names to better reflect their usage I did not modify any time formats as part of this change. Let me know which configs need more fine grained time specifications.
        Sriram Subramanian made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Neha Narkhede made changes -
        Assignee Sriram Subramanian [ sriramsub ]
        Swapnil Ghike made changes -
        Summary Use uniform convention for naming properties keys for clientId, groupId, consumerId, correlationId Use uniform convention for naming properties keys
        Jun Rao made changes -
        Priority Major [ 3 ] Blocker [ 1 ]
        Neha Narkhede made changes -
        Field Original Value New Value
        Fix Version/s 0.8.1 [ 12322960 ]
        Hide
        Jay Kreps added a comment -

        I actually recommend that we do a full config review and just change not only things that are poorly capitalized but also things which have inconsistent naming conventions or that are just confusing. For example 100% of properties related to segment retention are called log.retention.x EXCEPT for log.cleanup.interval.mins--why???? Also some times are given in mins or hours when a more granular time interval might be more useful. Or another: log.file.size should really be log.segment.size to make it clear that it is the size of the log segments not the total size of the log. I recommend we just make a full list of all configs and review them all. It will be boring but worth it.

        Show
        Jay Kreps added a comment - I actually recommend that we do a full config review and just change not only things that are poorly capitalized but also things which have inconsistent naming conventions or that are just confusing. For example 100% of properties related to segment retention are called log.retention.x EXCEPT for log.cleanup.interval.mins--why???? Also some times are given in mins or hours when a more granular time interval might be more useful. Or another: log.file.size should really be log.segment.size to make it clear that it is the size of the log segments not the total size of the log. I recommend we just make a full list of all configs and review them all. It will be boring but worth it.
        Hide
        Jay Kreps added a comment -

        A bunch of the config values are non standard. For example KafkaConfig.brokerId is given by the property "brokerid" which should be "broker.id". What I take Swapnil to be saying is that all properties should be in the form
        a.config.name
        and the scala variable should always be the same thing, but in camel case,
        aConfigName
        Would be great to do this in 0.8 with the other compatibility changes.

        Show
        Jay Kreps added a comment - A bunch of the config values are non standard. For example KafkaConfig.brokerId is given by the property "brokerid" which should be "broker.id". What I take Swapnil to be saying is that all properties should be in the form a.config.name and the scala variable should always be the same thing, but in camel case, aConfigName Would be great to do this in 0.8 with the other compatibility changes.
        Hide
        Neha Narkhede added a comment -

        Not sure I got the part about config.val, where first letter of second word is capitalized. Mind giving an example ?

        Show
        Neha Narkhede added a comment - Not sure I got the part about config.val, where first letter of second word is capitalized. Mind giving an example ?
        Hide
        Jay Kreps added a comment -

        word.

        Show
        Jay Kreps added a comment - word.
        Swapnil Ghike created issue -

          People

          • Assignee:
            Sriram Subramanian
            Reporter:
            Swapnil Ghike
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development