Thanks for the review Ning!
> 1 In the two rollbacks in mergeSegments (where inTransaction is
> false), the segmentInfos' generation is not always rolled back. So
> something like this could happen: two consecutive successful commits
> write segments_3 and segments_5, respectively. Nothing is broken,
> but it'd be nice to roll back completely (even for the IndexWriter
> instance) when a commit fails.
This is actually intentional: I don't want to write to the same
segments_N filename, ever, on the possibility that a reader may be
reading it. Admittedly, this should be quite rare (filling up disk
and then experiencing contention, only on Windows), but still I wanted
to keep "write once" even in this case.
> 2 Code serving two purposes are (and has been) mixed in
> mergeSegments: one to merge segments and create compound file if
> necessary, the other to commit or roll back when inTransaction is
> false. It'd be nice if the two could be separated: optimize and
> maybeMergeSegments call mergeSegmentsAndCommit, which creates a
> transaction, calls mergeSegments and commits or rolls back;
> mergeSegments doesn't deal with commit or rollback. However,
> currently the non-CFS version is committed first even if
> useCompoundFile is true. Until that's changed, mergeSegments
> probably has to continue serving both purposes.
I agree, mergeSegments is doing two different things now: merging
segments, and, committing this change (in 2 steps, for the CFS case)
into the index (if not in a transaction).
I had in fact tried putting a transaction up at the
optimize/maybeMergeSegments level, and it worked, but there was one
severe drawback: the max temp free space required would be [up to] 2X
the starting size of the segments-to-be-merged because the original
segments would be there (1X), the newly merged separate segment files
would be there (another 1X) and the just-created CFS segment file
would also be there (another 1X).
Whereas the max temp space required now is 1X the starting size of
So I think this (doing 2 commits with the current source before my
patch) is intentional, to keep the temp free space required at 1X.
It's also [relatively] OK to commit that first time, at least
functionally: the index is consistent and has all docs. The only
downside is that if you hit disk full or other exception when creating
the CFS file then your index has one segment not in compound format (I
will call this out in the javadocs).
OK I've added a unit-test that explicitly tests max temp space
required by just optimize and asserts that it's at most 1X starting
index size. Will attach a patch shortly!