Re: Review Request 29091: Improve 1646 fix by reduce check if Os.IsWindows

[Jay Kreps <bo...@gmail.com>]

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