----------------------------------------------------------- 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 > >
