----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29091/#review79812 -----------------------------------------------------------
I think this looks more reasonable. I am pretty confused by the code in roll(), have you tried that? Maybe I am misreading it? Let's do this: 1. Push the preallocation as far down in log as possible. I.e. ideally FileMessageSet should do the preallocation and a trim() method which would be invoked on Log.role() and during FileMessageSet.close(). 2. Get rid of the Os.isWindows option and make this a proper configuration as you suggested. I think there is nothing Windows specific, right? 3. Couple of other comments I left. core/src/main/scala/kafka/log/Log.scala <https://reviews.apache.org/r/29091/#comment129394> This should not be exposed as it doesn't make sense as a public operation. The fact that we preallocate and shrink the data file is internal. I think we can just have this happen as part of close(), right? core/src/main/scala/kafka/log/Log.scala <https://reviews.apache.org/r/29091/#comment129395> I'm confused by this. It looks like you are running log recovery--i.e. iterating over all messages in the segment and checksumming them--in the middle of rolling the log? That seems wrong, isn't it? I mean this means that if you had a 1GB log you would spend several minutes blocking during rolling the new segment, right? I think you do need to truncate off any preallocated bytes in the active segment but that should go through the same code path as close() right? core/src/main/scala/kafka/utils/Utils.scala <https://reviews.apache.org/r/29091/#comment129393> This utility is a bit unusual now since it is something like openChannelAndPreallocate. Let's do the following: 1. Remove the debug logging. We try to make the logging intelligible to non-programmer operators reading the log without reference to the code. And that won't make any sense. 2. Combine the two openChannel methods into one with an optional preallocateToSize option. 3. Move that method into FileMessageSet since that is the only place it is used and now it is a bit more idiosyncratic. - Jay Kreps On March 13, 2015, 3:12 a.m., Qianlin Xia wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29091/ > ----------------------------------------------------------- > > (Updated March 13, 2015, 3:12 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1646 > https://issues.apache.org/jira/browse/KAFKA-1646 > > > Repository: kafka > > > Description > ------- > > Improve 1646 fix by reduce check if Os.IsWindows > > > Diffs > ----- > > core/src/main/scala/kafka/log/FileMessageSet.scala > e1f8b979c3e6f62ea235bd47bc1587a1291443f9 > core/src/main/scala/kafka/log/Log.scala > 46df8d99d977a3b010a9b9f4698187fa9bfb2498 > core/src/main/scala/kafka/log/LogManager.scala > 7cee5435b23fcd0d76f531004911a2ca499df4f8 > core/src/main/scala/kafka/log/LogSegment.scala > 0d6926ea105a99c9ff2cfc9ea6440f2f2d37bde8 > core/src/main/scala/kafka/utils/Utils.scala > a89b0463685e6224d263bc9177075e1bb6b93d04 > > Diff: https://reviews.apache.org/r/29091/diff/ > > > Testing > ------- > > > Thanks, > > Qianlin Xia > >
