> On Oct. 6, 2014, 5:24 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/log/Log.scala, line 502 > > <https://reviews.apache.org/r/26346/diff/3/?file=714142#file714142line502> > > > > We probably should enforce that a segment is never larger than > > config.segmentSize. So, if messageSize is larger than config.segmentSize, > > we should just throw an exception. Once we do that, it seems that we can > > just use the following check to cover both conditions. > > > > if (segment.size + messageSize > (long) config.segmentSize > > > > We likely need to adjust the comment above accordingly. > > Sriharsha Chintalapani wrote: > Sorry I am not able follow . can you please elaborate on this " So, if > messageSize is larger than config.segmentSize,". > Here the issue is not the messageSize is larger than the > config.segmentSize. > Currently we only roll when segment.size is greater than the > config.segmentSize and the edge case here is > if the config.segmentSize is Int.MaxValue and the current segment.size is > Int.MaxValue - 1 we still wouldn't roll the segment > and append the current batch to the same segment and next time we check > segment.size is overflown will be negative and still fail to pass the check > segment.size > config.segmentSize and we keep appending to the same > LogSegment. > > if (segment.size + messageSize > (long) config.segmentSize > This condition wouldn't work since segment.size is Int and if its value > is anywhere closer to Int.MaxValue adding the current messages size will > cause it overflown. > > we can change the above condition to > if (segment.size.toLong + messageSize > config.segmentSize) > and changing the comment to > LogSegment will be rolled if segment.size + messagesBatch.size is > greater than config.segmentSize. > Please let me know if these changes looks good. Thanks.
Yes, my point is that the current implementation of config.segmentSize is a bit misleading. It actually allows a log segment to be larger than config.segmentSize. It's easier to understand if we disallow that. Also, if we never allow a segment to go beyond config.segmentSize, it will never overflow since config.segmentSize is capped at 2G. The following check that you suggested will suffice. if (segment.size.toLong + messageSize > config.segmentSize) Then the issue is what happens when the appended message set itself is larger than config.segmentSize. If you just have the above check, you will still allow a segment to have a size larger than config.segmentSize. Another issue is that if the active segment is empty, we will end up rolling a new segment with the same name, which will confuse the broker. In this case, it will be easier to reject the message set by throwing an exception. - Jun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26346/#review55526 ----------------------------------------------------------- On Oct. 6, 2014, 4:48 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26346/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2014, 4:48 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1670 > https://issues.apache.org/jira/browse/KAFKA-1670 > > > Repository: kafka > > > Description > ------- > > KAFKA-1670. Corrupt log files for segment.bytes values close to Int.MaxInt. > > > Diffs > ----- > > core/src/main/scala/kafka/log/Log.scala > 0ddf97bd30311b6039e19abade41d2fbbad2f59b > > Diff: https://reviews.apache.org/r/26346/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
