Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.3
    • Component/s: None
    • Labels:

      Description

      It would be nice to have Kafka brokers auto-assign node ids rather than having that be a configuration. Having a configuration is irritating because (1) you have to generate a custom config for each broker and (2) even though it is in configuration, changing the node id can cause all kinds of bad things to happen.

      1. KAFKA-1070_2014-07-19_16:06:13.patch
        18 kB
        Sriharsha Chintalapani
      2. KAFKA-1070_2014-07-22_11:34:18.patch
        35 kB
        Sriharsha Chintalapani
      3. KAFKA-1070_2014-07-24_20:58:17.patch
        33 kB
        Sriharsha Chintalapani
      4. KAFKA-1070_2014-07-24_21:05:33.patch
        22 kB
        Sriharsha Chintalapani
      5. KAFKA-1070_2014-08-21_10:26:20.patch
        47 kB
        Sriharsha Chintalapani
      6. KAFKA-1070_2014-11-20_10:50:04.patch
        35 kB
        Sriharsha Chintalapani
      7. KAFKA-1070_2014-11-25_20:29:37.patch
        43 kB
        Sriharsha Chintalapani
      8. KAFKA-1070_2015-01-01_17:39:30.patch
        28 kB
        Sriharsha Chintalapani
      9. KAFKA-1070_2015-01-12_10:46:54.patch
        28 kB
        Sriharsha Chintalapani
      10. KAFKA-1070_2015-01-12_18:30:17.patch
        29 kB
        Sriharsha Chintalapani
      11. KAFKA-1070.patch
        205 kB
        Sriharsha Chintalapani

        Activity

        Hide
        Jay Kreps added a comment -

        I think the right way to do this is to have a sequence in zookeeper that atomically increments and use this for id generation. On startup a node that has no id can generate one for itself and store it.

        One tricky bit is that this node id needs to be stored with the data, but we actually partition data up over multiple disks now and hope to be able to survive the destruction of any of them. Which disk should we store the node id on? I would recommend we store it on all of them--if it is missing on some we will add it there, if the id is inconsistent between disks we will error out (this should never happen).

        I would recommend adding a properties file named "meta" in every data directory containing the "id=x" value, we can extend this later with more perminant values. For example, I think it would be nice to add a data format version to help with in-place data upgrades.

        On startup the broker would check this value for consistency across directories. If it is not present in any directory it would auto-generate a node id and persist that for future use.

        For compatibility we would retain the current id configuration value--if it is present we will use it and ensure the id sequence is larger than this value.

        Show
        Jay Kreps added a comment - I think the right way to do this is to have a sequence in zookeeper that atomically increments and use this for id generation. On startup a node that has no id can generate one for itself and store it. One tricky bit is that this node id needs to be stored with the data, but we actually partition data up over multiple disks now and hope to be able to survive the destruction of any of them. Which disk should we store the node id on? I would recommend we store it on all of them--if it is missing on some we will add it there, if the id is inconsistent between disks we will error out (this should never happen). I would recommend adding a properties file named "meta" in every data directory containing the "id=x" value, we can extend this later with more perminant values. For example, I think it would be nice to add a data format version to help with in-place data upgrades. On startup the broker would check this value for consistency across directories. If it is not present in any directory it would auto-generate a node id and persist that for future use. For compatibility we would retain the current id configuration value--if it is present we will use it and ensure the id sequence is larger than this value.
        Hide
        Ancoron Luciferis added a comment -

        For me, a solution that generates a unique ID by itself would also be fine.

        E.g. as a workaround, I currently generate an integer based on the MAC address from the interface that clients are configured to see. However, this is not reliably unique, because a MAC address is 48-bit and the broker-ID is only a signed 32-bit integer, a long would be better fit here.

        Going further on standalone generation of unique IDs, within applications I am used to rely on something that does provide me some guarantees. A UUID of Type 1 (MAC/timestamp-based) is exactly that. However, here the problem is even worse, because a UUID is a 128-bit number.

        My question here would be (and I am not yet very much into the Kafka code base - so please excuse if this is a very dumb question): what is the brokerId actually being used for? In other words: which requirements does it have to fulfill and what functionality relies on it being unique inside a cluster?

        Another question is: why does it have to be a non-negative integer?

        Show
        Ancoron Luciferis added a comment - For me, a solution that generates a unique ID by itself would also be fine. E.g. as a workaround, I currently generate an integer based on the MAC address from the interface that clients are configured to see. However, this is not reliably unique, because a MAC address is 48-bit and the broker-ID is only a signed 32-bit integer, a long would be better fit here. Going further on standalone generation of unique IDs, within applications I am used to rely on something that does provide me some guarantees. A UUID of Type 1 (MAC/timestamp-based) is exactly that. However, here the problem is even worse, because a UUID is a 128-bit number. My question here would be (and I am not yet very much into the Kafka code base - so please excuse if this is a very dumb question): what is the brokerId actually being used for? In other words: which requirements does it have to fulfill and what functionality relies on it being unique inside a cluster? Another question is: why does it have to be a non-negative integer?
        Hide
        Sriharsha Chintalapani added a comment -

        Jay Kreps Neha Narkhede
        I am working on this JIRA. Following the proposed design in the comments. I see there might be a issue where
        one of the broker configs have broker.id defined as 0 and another one doesn't have broker.id. If the second broker started first we fetch a global sequence id from zookeeper that starts with 0 and the next one has already have a defined config we will use that in this case we have two brokers with same id. Should we consider this case in which a kafka cluster's broker might have a inconsistent config interms of broker.id.
        Instead of using zookeeper for generating a global sequence number why shouldn't we be using UUID and make brokerId Long type.
        Thanks.

        Show
        Sriharsha Chintalapani added a comment - Jay Kreps Neha Narkhede I am working on this JIRA. Following the proposed design in the comments. I see there might be a issue where one of the broker configs have broker.id defined as 0 and another one doesn't have broker.id. If the second broker started first we fetch a global sequence id from zookeeper that starts with 0 and the next one has already have a defined config we will use that in this case we have two brokers with same id. Should we consider this case in which a kafka cluster's broker might have a inconsistent config interms of broker.id. Instead of using zookeeper for generating a global sequence number why shouldn't we be using UUID and make brokerId Long type. Thanks.
        Hide
        Jay Kreps added a comment -

        Hey sriharsha chintalapani, that is a great point.

        I'd like to avoid changing the type of the id to a long or UUID so as to not have to bump up the protocol format for the metadata request which hands these out to clients (we would need a way to handle compatibility with older clients that don't expect the longer types).

        I think we can get around the problem you point out by just defaulting the node id sequence to 1000. This could theoretically conflict but most people number from 0 or 1 and we can discuss this in the release notes. Our plan will be to release with support for both configured node ids and assigned node ids for compatibility. After a couple of releases we will remove the config.

        So the behavior would be this:
        If there is a node id in the config we will validate it against the node id in the data directory
        If it matches that good, we'll use that.
        If it doesn't match that is bad, we'll crash with an error.
        If there is a node id in the data directory but none in the config, we'll use whatever is in the data directory.
        If there is no node id in the data directory yet but there is one in the config we'll write that to the data directory and use it.
        If there is neither a node id in the data directory nor in the config we'll allocate a node id and write it to the data directory.

        Show
        Jay Kreps added a comment - Hey sriharsha chintalapani , that is a great point. I'd like to avoid changing the type of the id to a long or UUID so as to not have to bump up the protocol format for the metadata request which hands these out to clients (we would need a way to handle compatibility with older clients that don't expect the longer types). I think we can get around the problem you point out by just defaulting the node id sequence to 1000. This could theoretically conflict but most people number from 0 or 1 and we can discuss this in the release notes. Our plan will be to release with support for both configured node ids and assigned node ids for compatibility. After a couple of releases we will remove the config. So the behavior would be this: If there is a node id in the config we will validate it against the node id in the data directory If it matches that good, we'll use that. If it doesn't match that is bad, we'll crash with an error. If there is a node id in the data directory but none in the config, we'll use whatever is in the data directory. If there is no node id in the data directory yet but there is one in the config we'll write that to the data directory and use it. If there is neither a node id in the data directory nor in the config we'll allocate a node id and write it to the data directory.
        Hide
        Sriharsha Chintalapani added a comment -

        Created reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Created reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Thanks Jay Kreps

        Show
        Sriharsha Chintalapani added a comment - Thanks Jay Kreps
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Clark Haskins added a comment -

        We currently have node IDs in the 0-10000 range. I think some better logic for identifying used node IDs is necessary rather than just starting from 1000.

        Show
        Clark Haskins added a comment - We currently have node IDs in the 0-10000 range. I think some better logic for identifying used node IDs is necessary rather than just starting from 1000.
        Hide
        Jon Bringhurst added a comment - - edited

        Clark Haskins, Sriharsha Chintalapani, having ReservedBrokerIdMaxValue as a configurable option (perhaps with a default of 1000) would probably solve clark's use case.

        Also, if I'm reading this right, it will ignore the broker ID if it's set in the config but out of the reserved range (set to -1). It will then create a generated broker ID to overwrite the one in the config. It might be nice to have the broker error out on startup if a broker.id is set in the config, but is out of range. A new broker ID should only be generated if an id isn't specified in the config AND the meta.properties doesn't exist (otherwise the config file wouldn't match the actual broker ID). Edit: nevermind, my Scala reading skills aren't up to par. This isn't the case here.

        Show
        Jon Bringhurst added a comment - - edited Clark Haskins , Sriharsha Chintalapani , having ReservedBrokerIdMaxValue as a configurable option (perhaps with a default of 1000) would probably solve clark's use case. Also, if I'm reading this right, it will ignore the broker ID if it's set in the config but out of the reserved range (set to -1). It will then create a generated broker ID to overwrite the one in the config. It might be nice to have the broker error out on startup if a broker.id is set in the config, but is out of range. A new broker ID should only be generated if an id isn't specified in the config AND the meta.properties doesn't exist (otherwise the config file wouldn't match the actual broker ID). Edit: nevermind, my Scala reading skills aren't up to par. This isn't the case here.
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Joe Stein added a comment - - edited

        This sounds like a very cool and VERY useful feature. Excited to use it myself often.

        I know of a few (>10) different clusters that not only use varying sized numbers for their broker.id but do so in what is a seemingly random (but not really when you think about it) way.

        so in a cluster there may be broker.id that is 1721632 and another 172164875 and another 172162240 . Making your brokers by replacing "." in chef/puppet/salt/ansemble/etc type scripts and sometimes folks get more fancy just doing 2288, 2388, 17, 177 (where just the last two octets get used and "." is replaced).

        I am not saying I agree with this approach and I actively advocate away from doing this but in some/many scenarios it is the best/only way to automate their deploys for how things are setup. It is also what seems to make sense when folks are building their automation scripts since they have no other option without doing more than they should be expected to-do (and the IP replace "." is so obvious to them, and it is).

        So, for folks in these cases they would just pick the upper bound to be, lets say 17216255256 and then it would auto assign from there?

        Is there some better way to go about this where you might have a start increment and and some exclusion list? or a way to see broker.id already had that and not use it? I think a lot of folks would like to get having broker id be more continious and be easier to communicate but the desire to automate everything will outweigh that. We could give them some sanity back with brokers 1,2,3,4,5,6,7,8,9,10,11,12 for a 12 node cluster.

        not crucial and you may have already accounted for the scenario I brought up, but wanted to bring it up as a real use case for how people automate things.

        it might be better for folks to manually migrate their scripts but not sure they would do it and if they did would have to decommission brokers which in a prod environment could take a few weeks/months. If we let them start at 1 and exclude what they have then they can do it one at a time. After taking down the first broker and bring it back up (empty) it is broker.id=1, and so on (and if they have a 5 they don't have to take it down), etc.

        For new clusters this is a slam dunk and wouldn't want to hold up the feature for existing users that have already decided a work around as I don't know what the intent of this was or not. Some folks might not change knowing broker.id=17216520 sometimes is nice you just login to that box but talking about broker 17216520 over and over again is a pita.

        Show
        Joe Stein added a comment - - edited This sounds like a very cool and VERY useful feature. Excited to use it myself often. I know of a few (>10) different clusters that not only use varying sized numbers for their broker.id but do so in what is a seemingly random (but not really when you think about it) way. so in a cluster there may be broker.id that is 1721632 and another 172164875 and another 172162240 . Making your brokers by replacing "." in chef/puppet/salt/ansemble/etc type scripts and sometimes folks get more fancy just doing 2288, 2388, 17, 177 (where just the last two octets get used and "." is replaced). I am not saying I agree with this approach and I actively advocate away from doing this but in some/many scenarios it is the best/only way to automate their deploys for how things are setup. It is also what seems to make sense when folks are building their automation scripts since they have no other option without doing more than they should be expected to-do (and the IP replace "." is so obvious to them, and it is). So, for folks in these cases they would just pick the upper bound to be, lets say 17216255256 and then it would auto assign from there? Is there some better way to go about this where you might have a start increment and and some exclusion list? or a way to see broker.id already had that and not use it? I think a lot of folks would like to get having broker id be more continious and be easier to communicate but the desire to automate everything will outweigh that. We could give them some sanity back with brokers 1,2,3,4,5,6,7,8,9,10,11,12 for a 12 node cluster. not crucial and you may have already accounted for the scenario I brought up, but wanted to bring it up as a real use case for how people automate things. it might be better for folks to manually migrate their scripts but not sure they would do it and if they did would have to decommission brokers which in a prod environment could take a few weeks/months. If we let them start at 1 and exclude what they have then they can do it one at a time. After taking down the first broker and bring it back up (empty) it is broker.id=1, and so on (and if they have a 5 they don't have to take it down), etc. For new clusters this is a slam dunk and wouldn't want to hold up the feature for existing users that have already decided a work around as I don't know what the intent of this was or not. Some folks might not change knowing broker.id=17216520 sometimes is nice you just login to that box but talking about broker 17216520 over and over again is a pita.
        Hide
        Sriharsha Chintalapani added a comment -

        Joe Stein Thanks for the comments. Currently the approach allows users to specify MaxBrokerId and kafka starts generating a sequence from MaxBrokerId + 1. If user specifies brokerId , kafka uses that instead of generating one from zookeeper. In the current implementation we expect user configured broker.id to be less than MaxBrokerId.
        The tricky case here is if the user have a cluster where some of the brokers have broker.id present in their config and others don't. In this case we use MaxBrokerId (defaults to 1000) and start generating a sequenceId from there.
        The issue with brokerId exception list is we do not know from which point to generate a sequenceId.
        In your example " in a cluster there may be broker.id that is 1721632 and another 172164875 and another 172162240 "
        and user added another broker to the cluster without broker.id in config. If the user specifies that MaxReserveBrokerId is 172164875 than we can assign +1 to that new broker. If we take the approach of looking at brokerIds that exists in the cluster and generate a new one that doesn't exist or taken by a broker already(exclusion list as you suggested) than the problem would be how do we know what are all the broker.ids existed in the cluster one way to look at is zookeeper but a broker registers at zk after it starts.
        For ex: if a broker1 starts with 1721632 and broker2 starts with no broker.id than we generate 1721633 from zk and there is no guarantee that another broker has this same id in their config. Of course we can have a exclusion list of brokerids in a config file but that feels cumbersome for the user to list out all the brokerids.

        Show
        Sriharsha Chintalapani added a comment - Joe Stein Thanks for the comments. Currently the approach allows users to specify MaxBrokerId and kafka starts generating a sequence from MaxBrokerId + 1. If user specifies brokerId , kafka uses that instead of generating one from zookeeper. In the current implementation we expect user configured broker.id to be less than MaxBrokerId. The tricky case here is if the user have a cluster where some of the brokers have broker.id present in their config and others don't. In this case we use MaxBrokerId (defaults to 1000) and start generating a sequenceId from there. The issue with brokerId exception list is we do not know from which point to generate a sequenceId. In your example " in a cluster there may be broker.id that is 1721632 and another 172164875 and another 172162240 " and user added another broker to the cluster without broker.id in config. If the user specifies that MaxReserveBrokerId is 172164875 than we can assign +1 to that new broker. If we take the approach of looking at brokerIds that exists in the cluster and generate a new one that doesn't exist or taken by a broker already(exclusion list as you suggested) than the problem would be how do we know what are all the broker.ids existed in the cluster one way to look at is zookeeper but a broker registers at zk after it starts. For ex: if a broker1 starts with 1721632 and broker2 starts with no broker.id than we generate 1721633 from zk and there is no guarantee that another broker has this same id in their config. Of course we can have a exclusion list of brokerids in a config file but that feels cumbersome for the user to list out all the brokerids.
        Hide
        Joe Stein added a comment - - edited

        Sriharsha Chintalapani I understand. What I am struggling with is this for new users or existing users? For existing users what I have seen them do (at least a dozen times now) is to avoid the conflict of broker.id in the properties file (like you are saying) is to take the IP address, strip out the "." and use that (since IP is unique that is what they go with). They do this in their automation scripts. I have to imagine it is larger than I have seen as they all did this independently of each other as an intuitive way to make unique broker.id.

        Show
        Joe Stein added a comment - - edited Sriharsha Chintalapani I understand. What I am struggling with is this for new users or existing users? For existing users what I have seen them do (at least a dozen times now) is to avoid the conflict of broker.id in the properties file (like you are saying) is to take the IP address, strip out the "." and use that (since IP is unique that is what they go with). They do this in their automation scripts. I have to imagine it is larger than I have seen as they all did this independently of each other as an intuitive way to make unique broker.id.
        Hide
        Sriharsha Chintalapani added a comment -

        Jay Kreps Jun Rao Neha Narkhede Can you please review the last patch I sent. Thanks.

        Show
        Sriharsha Chintalapani added a comment - Jay Kreps Jun Rao Neha Narkhede Can you please review the last patch I sent. Thanks.
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Jay Kreps Jun Rao Neha Narkhede Can you please review the latest patch. Thanks.

        Show
        Sriharsha Chintalapani added a comment - Jay Kreps Jun Rao Neha Narkhede Can you please review the latest patch. Thanks.
        Hide
        Neha Narkhede added a comment -

        In an attempt to keep up with reviews, assigning to Jay Kreps since he has most context on the patch. Feel free to reassign

        Show
        Neha Narkhede added a comment - In an attempt to keep up with reviews, assigning to Jay Kreps since he has most context on the patch. Feel free to reassign
        Hide
        Sriharsha Chintalapani added a comment - - edited

        Neha Narkhede Jun Rao Jay Kreps Guozhang Wang Hi All, I am trying to get this patch reviewed. Please let me know if there is anything that need to be fixed with this patch. Thank you.

        Show
        Sriharsha Chintalapani added a comment - - edited Neha Narkhede Jun Rao Jay Kreps Guozhang Wang Hi All, I am trying to get this patch reviewed. Please let me know if there is anything that need to be fixed with this patch. Thank you.
        Hide
        Otis Gospodnetic added a comment -

        sriharsha chintalapani - it looks like a number of people would really like to use IPs for broker.id. There is a lot of interest in having that. Please see this thread: http://search-hadoop.com/m/4TaT4dTPKi1
        Do you think this is something you could add to this patch, maybe as another broker ID assignment scheme/policy?

        Show
        Otis Gospodnetic added a comment - sriharsha chintalapani - it looks like a number of people would really like to use IPs for broker.id. There is a lot of interest in having that. Please see this thread: http://search-hadoop.com/m/4TaT4dTPKi1 Do you think this is something you could add to this patch, maybe as another broker ID assignment scheme/policy?
        Hide
        Sriharsha Chintalapani added a comment -

        Otis Gospodnetic this patch pending the 0.8.2 release. Since the we have 0.8.2 branched out I'll upmerge this patch and add a configurable option to server.properties where if user prefers IP based broker Id we will generate that or use the seq id that I currently have as a default.
        Jay Kreps Neha Narkhede Jun Rao please let me know if the above approach looks good to you. Thanks.

        Show
        Sriharsha Chintalapani added a comment - Otis Gospodnetic this patch pending the 0.8.2 release. Since the we have 0.8.2 branched out I'll upmerge this patch and add a configurable option to server.properties where if user prefers IP based broker Id we will generate that or use the seq id that I currently have as a default. Jay Kreps Neha Narkhede Jun Rao please let me know if the above approach looks good to you. Thanks.
        Hide
        Otis Gospodnetic added a comment -

        this patch pending the 0.8.2 release

        Note this issue doesn't have Fix Version set to 0.8.2. Maybe that's what you want? +1 from me – it looks like a number of people would like to see this issue resolved!

        Show
        Otis Gospodnetic added a comment - this patch pending the 0.8.2 release Note this issue doesn't have Fix Version set to 0.8.2. Maybe that's what you want? +1 from me – it looks like a number of people would like to see this issue resolved!
        Hide
        Sriharsha Chintalapani added a comment -

        Otis Gospodnetic Sorry I meant pending review after 0.8.2 release. Not planned for 0.8.2 release. I'll update the patch with ip as brokerid option.

        Show
        Sriharsha Chintalapani added a comment - Otis Gospodnetic Sorry I meant pending review after 0.8.2 release. Not planned for 0.8.2 release. I'll update the patch with ip as brokerid option.
        Hide
        Neha Narkhede added a comment -

        Sorry for the late review. Expect to get to this shortly.

        Show
        Neha Narkhede added a comment - Sorry for the late review. Expect to get to this shortly.
        Hide
        Neha Narkhede added a comment -

        Sriharsha Chintalapani Would you mind rebasing your patch? Can help you review it in the next few days.

        Show
        Neha Narkhede added a comment - Sriharsha Chintalapani Would you mind rebasing your patch? Can help you review it in the next few days.
        Hide
        Sriharsha Chintalapani added a comment -

        Neha Narkhede will implement the requested ip based broker id as an option and send an updated patch. Try to get it done this week.

        Show
        Sriharsha Chintalapani added a comment - Neha Narkhede will implement the requested ip based broker id as an option and send an updated patch. Try to get it done this week.
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Neha Narkhede Joe Stein Otis Gospodnetic
        Please take a look at the latest patch. I added broker.id.policy as a configurable option with "ip" and "sequence" .
        I've few questions regarding "ip" based broker.id.
        currently the patch expects /etc/hosts to be properly configured with an ip associated with the hostname.
        1) should we ignore loopback ip
        i) if we don't consider loopback ip than on a single node machine with /etc/hosts is not being configured properly it will thrown an error
        ii) if we consider this than there is a same broker id being generate on multiple hosts or multiple brokers on a single host ( this could be the case in non loopback ips too)
        2) broker.id is Int and there is a possibility of throwing a NumberFormatException if the ip doesn't fit in Int.MaxValue
        3) If a host contains multiple network interfaces which ip should we pick up. Current implementation takes whatever configured for the hostname in /etc/hosts which seems ok to me .
        Please let me know your thoughts on the above. Thanks.

        Show
        Sriharsha Chintalapani added a comment - Neha Narkhede Joe Stein Otis Gospodnetic Please take a look at the latest patch. I added broker.id.policy as a configurable option with "ip" and "sequence" . I've few questions regarding "ip" based broker.id. currently the patch expects /etc/hosts to be properly configured with an ip associated with the hostname. 1) should we ignore loopback ip i) if we don't consider loopback ip than on a single node machine with /etc/hosts is not being configured properly it will thrown an error ii) if we consider this than there is a same broker id being generate on multiple hosts or multiple brokers on a single host ( this could be the case in non loopback ips too) 2) broker.id is Int and there is a possibility of throwing a NumberFormatException if the ip doesn't fit in Int.MaxValue 3) If a host contains multiple network interfaces which ip should we pick up. Current implementation takes whatever configured for the hostname in /etc/hosts which seems ok to me . Please let me know your thoughts on the above. Thanks.
        Hide
        Neha Narkhede added a comment -

        Sriharsha Chintalapani Reviewed your latest patch. Posted review comments on the rb.

        Show
        Neha Narkhede added a comment - Sriharsha Chintalapani Reviewed your latest patch. Posted review comments on the rb.
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Neha Narkhede Thanks for the review. Updated the patch please take a look .

        Show
        Sriharsha Chintalapani added a comment - Neha Narkhede Thanks for the review. Updated the patch please take a look .
        Hide
        Sriharsha Chintalapani added a comment -

        Neha Narkhede Can you please take a look at the patch and also reply to your comments. Thanks.

        Show
        Sriharsha Chintalapani added a comment - Neha Narkhede Can you please take a look at the patch and also reply to your comments. Thanks.
        Hide
        Neha Narkhede added a comment -

        Thanks for the updated patch Sriharsha Chintalapani. Almost there. Left a few more cleanup comments.

        Show
        Neha Narkhede added a comment - Thanks for the updated patch Sriharsha Chintalapani . Almost there. Left a few more cleanup comments.
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Thanks for the review Neha Narkhede. I updated the patch , please take a look when you get a chance . Thanks.

        Show
        Sriharsha Chintalapani added a comment - Thanks for the review Neha Narkhede . I updated the patch , please take a look when you get a chance . Thanks.
        Hide
        Neha Narkhede added a comment -

        Sriharsha Chintalapani There are still questions around the broker metadata file serialization. I've reviewed the latest patch and left my comments on the rb.

        Show
        Neha Narkhede added a comment - Sriharsha Chintalapani There are still questions around the broker metadata file serialization. I've reviewed the latest patch and left my comments on the rb.
        Hide
        Sriharsha Chintalapani added a comment -

        Thanks Neha Narkhede . I'll send an updated patch.

        Show
        Sriharsha Chintalapani added a comment - Thanks Neha Narkhede . I'll send an updated patch.
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Neha Narkhede updated patch addresses 2 of your comments and I've replied to your other comments. Please check and let me know. Thank for the review.

        Show
        Sriharsha Chintalapani added a comment - Neha Narkhede updated patch addresses 2 of your comments and I've replied to your other comments. Please check and let me know. Thank for the review.
        Hide
        Neha Narkhede added a comment -

        Sriharsha Chintalapani Thanks for your efforts in getting this patch in shape. Pushed to trunk

        Show
        Neha Narkhede added a comment - Sriharsha Chintalapani Thanks for your efforts in getting this patch in shape. Pushed to trunk
        Hide
        Sriharsha Chintalapani added a comment -

        Updated reviewboard https://reviews.apache.org/r/23702/diff/
        against branch origin/trunk

        Show
        Sriharsha Chintalapani added a comment - Updated reviewboard https://reviews.apache.org/r/23702/diff/ against branch origin/trunk

          People

          • Assignee:
            Sriharsha Chintalapani
            Reporter:
            Jay Kreps
            Reviewer:
            Neha Narkhede
          • Votes:
            3 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development