You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Sriharsha Chintalapani <ha...@hortonworks.com> on 2015/01/02 02:39:34 UTC

Re: Review Request 23702: Patch for KAFKA-1070

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/
-----------------------------------------------------------

(Updated Jan. 2, 2015, 1:39 a.m.)


Review request for kafka.


Bugs: KAFKA-1070
    https://issues.apache.org/jira/browse/KAFKA-1070


Repository: kafka


Description
-------

KAFKA-1070. Auto-assign node id.


Diffs (updated)
-----

  core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
  core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
  core/src/main/scala/kafka/server/KafkaServer.scala 1bf7d10cef23a77e716666eb16bf6d0e68bc4ebe 
  core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
  core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 94d0028d8c4907e747aa8a74a13d719b974c97bf 

Diff: https://reviews.apache.org/r/23702/diff/


Testing
-------


Thanks,

Sriharsha Chintalapani


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> On Jan. 9, 2015, 11:39 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala, line 48
> > <https://reviews.apache.org/r/23702/diff/8-9/?file=776402#file776402line48>
> >
> >     Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions.
> >     
> >     Also, if you serialize the file this way, is it still human readable? 
> >     
> >     I'd recommend you look at other checkpoint file and follow the same or change those to use the new strategy you pick.

brokerMetaProps.store(fileOutputStream,"") stores the data in human-readable form . For ex:
"#Mon Jan 12 10:10:46 PST 2015
version=0
broker.id=1001"
In BrokerMetadataCheckpoint.read() methods checks the version and does case operation similar to OffsetCheckpoint.

In storing data  OffsetCheckpoint is different, OffsetCheckpoint just stores the values like
0
topic=offset

I like the current version for broker metadata as the file will be self-explanatory if users needed to change or look at the current values.
I don't think OffsetCheckpoint and BrokerMetadataCheckpoint needs to follow same format to store data. If you disagree I can change it to just store data like in Offsetcheckpoint.


- Sriharsha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67551
-----------------------------------------------------------


On Jan. 12, 2015, 6:46 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 6:46 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> On Jan. 9, 2015, 11:39 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala, line 30
> > <https://reviews.apache.org/r/23702/diff/9/?file=805362#file805362line30>
> >
> >     I think the way you modelled this checkpoint file, which makes it different from all other checkpoints we have, is that the metadata checkpoints handles the multiple log directories itself. So it is not a single broker metadata checkpoint. My only concern with this is that now only this checkpoint is very different from other checkpoints. I'd prefer we maintain some consistency in all our checkpoints. So modeling log directories is fine but then it's desirable if all our checkpoints behaved that way. 
> >     
> >     If you'd prefer changing this checkpoint file to match others, I'd suggest-
> >     
> >     1. Change the name to BrokerMetadataCheckpoint. 
> >     2. Make it take the File that identifies the metadata checkpoint file in the constructor
> >     3. Change write to not accept the log directories. Just include the BrokerMetadata object. 
> >     4. Similarly include no parameters in the read() API

Followed your suggesstion.


- Sriharsha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67551
-----------------------------------------------------------


On Jan. 12, 2015, 6:46 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 6:46 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> On Jan. 9, 2015, 11:39 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala, line 48
> > <https://reviews.apache.org/r/23702/diff/8-9/?file=776402#file776402line48>
> >
> >     Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions.
> >     
> >     Also, if you serialize the file this way, is it still human readable? 
> >     
> >     I'd recommend you look at other checkpoint file and follow the same or change those to use the new strategy you pick.
> 
> Sriharsha Chintalapani wrote:
>     brokerMetaProps.store(fileOutputStream,"") stores the data in human-readable form . For ex:
>     "#Mon Jan 12 10:10:46 PST 2015
>     version=0
>     broker.id=1001"
>     In BrokerMetadataCheckpoint.read() methods checks the version and does case operation similar to OffsetCheckpoint.
>     
>     In storing data  OffsetCheckpoint is different, OffsetCheckpoint just stores the values like
>     0
>     topic=offset
>     
>     I like the current version for broker metadata as the file will be self-explanatory if users needed to change or look at the current values.
>     I don't think OffsetCheckpoint and BrokerMetadataCheckpoint needs to follow same format to store data. If you disagree I can change it to just store data like in Offsetcheckpoint.
> 
> Neha Narkhede wrote:
>     Okay. What do you think of the following -
>     "Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions."

I am not sure if I understand correctly. BrokerMetadataCheckpoint.read() does this.
        val brokerMetaProps = new VerifiableProperties(Utils.loadProps(file.getAbsolutePath()))
        val version = brokerMetaProps.getIntInRange("version", (0, Int.MaxValue))
        version match {
          case 0 => 
          .
It does load the entire contents into brokerMetaProps and we can pick the version from it. 
If the intention is to just read the single line to get the version its not possible with the current approach.
Any specific reason that we need just the version. In either case we will be able to support multiple version files at the same.
Since the file contents are like key=value pairs.


- Sriharsha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67551
-----------------------------------------------------------


On Jan. 12, 2015, 6:46 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 6:46 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Neha Narkhede <ne...@gmail.com>.

> On Jan. 9, 2015, 11:39 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala, line 48
> > <https://reviews.apache.org/r/23702/diff/8-9/?file=776402#file776402line48>
> >
> >     Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions.
> >     
> >     Also, if you serialize the file this way, is it still human readable? 
> >     
> >     I'd recommend you look at other checkpoint file and follow the same or change those to use the new strategy you pick.
> 
> Sriharsha Chintalapani wrote:
>     brokerMetaProps.store(fileOutputStream,"") stores the data in human-readable form . For ex:
>     "#Mon Jan 12 10:10:46 PST 2015
>     version=0
>     broker.id=1001"
>     In BrokerMetadataCheckpoint.read() methods checks the version and does case operation similar to OffsetCheckpoint.
>     
>     In storing data  OffsetCheckpoint is different, OffsetCheckpoint just stores the values like
>     0
>     topic=offset
>     
>     I like the current version for broker metadata as the file will be self-explanatory if users needed to change or look at the current values.
>     I don't think OffsetCheckpoint and BrokerMetadataCheckpoint needs to follow same format to store data. If you disagree I can change it to just store data like in Offsetcheckpoint.
> 
> Neha Narkhede wrote:
>     Okay. What do you think of the following -
>     "Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions."
> 
> Sriharsha Chintalapani wrote:
>     I am not sure if I understand correctly. BrokerMetadataCheckpoint.read() does this.
>             val brokerMetaProps = new VerifiableProperties(Utils.loadProps(file.getAbsolutePath()))
>             val version = brokerMetaProps.getIntInRange("version", (0, Int.MaxValue))
>             version match {
>               case 0 => 
>               .
>     It does load the entire contents into brokerMetaProps and we can pick the version from it. 
>     If the intention is to just read the single line to get the version its not possible with the current approach.
>     Any specific reason that we need just the version. In either case we will be able to support multiple version files at the same.
>     Since the file contents are like key=value pairs.

> It does load the entire contents into brokerMetaProps and we can pick the version from it.

You are right. This should be sufficient. The broker can be smart about interpreting the rest of the fields based on the read version.


- Neha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67551
-----------------------------------------------------------


On Jan. 12, 2015, 6:46 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 6:46 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Neha Narkhede <ne...@gmail.com>.

> On Jan. 9, 2015, 11:39 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala, line 48
> > <https://reviews.apache.org/r/23702/diff/8-9/?file=776402#file776402line48>
> >
> >     Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions.
> >     
> >     Also, if you serialize the file this way, is it still human readable? 
> >     
> >     I'd recommend you look at other checkpoint file and follow the same or change those to use the new strategy you pick.
> 
> Sriharsha Chintalapani wrote:
>     brokerMetaProps.store(fileOutputStream,"") stores the data in human-readable form . For ex:
>     "#Mon Jan 12 10:10:46 PST 2015
>     version=0
>     broker.id=1001"
>     In BrokerMetadataCheckpoint.read() methods checks the version and does case operation similar to OffsetCheckpoint.
>     
>     In storing data  OffsetCheckpoint is different, OffsetCheckpoint just stores the values like
>     0
>     topic=offset
>     
>     I like the current version for broker metadata as the file will be self-explanatory if users needed to change or look at the current values.
>     I don't think OffsetCheckpoint and BrokerMetadataCheckpoint needs to follow same format to store data. If you disagree I can change it to just store data like in Offsetcheckpoint.

Okay. What do you think of the following -
"Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions."


- Neha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67551
-----------------------------------------------------------


On Jan. 12, 2015, 6:46 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 6:46 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67551
-----------------------------------------------------------



core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala
<https://reviews.apache.org/r/23702/#comment111593>

    This may not be required since you are writing to the file. It will create it if it doesn't already exist.



core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala
<https://reviews.apache.org/r/23702/#comment111594>

    Does java serialization for the Properties object allow you to read just the version field? Basically, if we want to upgrade the version of this file, for some time the server would have to support 2 versions and read the file differently based on the different versions.
    
    Also, if you serialize the file this way, is it still human readable? 
    
    I'd recommend you look at other checkpoint file and follow the same or change those to use the new strategy you pick.



core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala
<https://reviews.apache.org/r/23702/#comment111591>

    I think the way you modelled this checkpoint file, which makes it different from all other checkpoints we have, is that the metadata checkpoints handles the multiple log directories itself. So it is not a single broker metadata checkpoint. My only concern with this is that now only this checkpoint is very different from other checkpoints. I'd prefer we maintain some consistency in all our checkpoints. So modeling log directories is fine but then it's desirable if all our checkpoints behaved that way. 
    
    If you'd prefer changing this checkpoint file to match others, I'd suggest-
    
    1. Change the name to BrokerMetadataCheckpoint. 
    2. Make it take the File that identifies the metadata checkpoint file in the constructor
    3. Change write to not accept the log directories. Just include the BrokerMetadata object. 
    4. Similarly include no parameters in the read() API


- Neha Narkhede


On Jan. 2, 2015, 1:39 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2015, 1:39 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataFileHandler.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1bf7d10cef23a77e716666eb16bf6d0e68bc4ebe 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 94d0028d8c4907e747aa8a74a13d719b974c97bf 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/
-----------------------------------------------------------

(Updated Jan. 13, 2015, 2:30 a.m.)


Review request for kafka.


Bugs: KAFKA-1070
    https://issues.apache.org/jira/browse/KAFKA-1070


Repository: kafka


Description
-------

KAFKA-1070. Auto-assign node id.


Diffs (updated)
-----

  core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
  core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
  core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
  core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
  core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 

Diff: https://reviews.apache.org/r/23702/diff/


Testing
-------


Thanks,

Sriharsha Chintalapani


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> On Jan. 12, 2015, 10:08 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala, line 55
> > <https://reviews.apache.org/r/23702/diff/10/?file=816767#file816767line55>
> >
> >     Minor nit which I can address during checkin: According to our coding convention, there should be a space after the , here.

updated the patch with the fix.


- Sriharsha


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67715
-----------------------------------------------------------


On Jan. 13, 2015, 2:30 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 2:30 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/#review67715
-----------------------------------------------------------

Ship it!


Ship It!


core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala
<https://reviews.apache.org/r/23702/#comment111797>

    Minor nit which I can address during checkin: According to our coding convention, there should be a space after the , here.


- Neha Narkhede


On Jan. 12, 2015, 6:46 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23702/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 6:46 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1070
>     https://issues.apache.org/jira/browse/KAFKA-1070
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1070. Auto-assign node id.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
>   core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
>   core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
>   core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 
> 
> Diff: https://reviews.apache.org/r/23702/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>


Re: Review Request 23702: Patch for KAFKA-1070

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23702/
-----------------------------------------------------------

(Updated Jan. 12, 2015, 6:46 p.m.)


Review request for kafka.


Bugs: KAFKA-1070
    https://issues.apache.org/jira/browse/KAFKA-1070


Repository: kafka


Description
-------

KAFKA-1070. Auto-assign node id.


Diffs (updated)
-----

  core/src/main/scala/kafka/common/GenerateBrokerIdException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/InconsistentBrokerIdException.scala PRE-CREATION 
  core/src/main/scala/kafka/server/BrokerMetadataCheckpoint.scala PRE-CREATION 
  core/src/main/scala/kafka/server/KafkaConfig.scala 6e26c5436feb4629d17f199011f3ebb674aa767f 
  core/src/main/scala/kafka/server/KafkaServer.scala 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b 
  core/src/main/scala/kafka/utils/ZkUtils.scala 56e3e88e0cc6d917b0ffd1254e173295c1c4aabd 
  core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala PRE-CREATION 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c9e8ba257b77f46c5c9b62b451470348b6e58889 

Diff: https://reviews.apache.org/r/23702/diff/


Testing
-------


Thanks,

Sriharsha Chintalapani