[
https://issues.apache.org/jira/browse/LUCENE-7976?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16463704#comment-16463704
]
Simon Willnauer edited comment on LUCENE-7976 at 5/4/18 11:31 AM:
------------------------------------------------------------------
Eric thanks for tackling this big issue here!
here are a couple comments:
* please remove the commented part that refers to // TODO: See LUCENE-8263
* Can we find a better name for _InfoInfo_ maybe _SegmentSizeAndDocs_
* can you make _SegmentSizeAndDocs_ static and maybe a simple struct ie. no
getters and don't pass IW to it
* can we assert that _int totalMaxDocs_ is always positive. I know we don't
allow that many documents in an index but I think it would be good to have an
extra check.
* can we name _maxMergeAtOnceThisMerge_ _currentMaxMergeAtOnce_ or maybe just
_maxMergeAtOnce_
I got down this quite a bit and I am starting to question if we should really
try to change the algorithm that we have today or if this class needs cleanup
and refactorings first. I am sorry to come in late here but this is a very very
complex piece of code and adding more complexity to it will rather do harm.
That said, I wonder if we can generalize the algorithm here into a single
method because in the end they all do the same thing. We can for instance make
the selection alg pluggable with a func we pass in and that way differentiate
between findMerges and findForceMerge etc. At the end of the day we want them
all to work in the same way. I am not saying we should go down all that way but
maybe we can extract a common code path that we can share between the places
were we filter out the segments that are not eligible.
This is just a suggestion, I am happy to help here btw. One thing that concerns
me and is in-fact a showstopper IMO is that the patch doesn't have a single
test that ensures it's correct. I mean we significantly change the behavior I
think it warrants tests no?
was (Author: simonw):
Eric thanks for tackling this big issue here!
here are a couple comments:
* please remove the commented part that refers to // TODO: See LUCENE-8263
* Can we find a better name for _InfoInfo_ maybe _SegmentSizeAndDocs_
* can you make _SegmentSizeAndDocs_ static and maybe a simple struct ie. no
getters and don't pass IW to it
* can we assert that _int totalMaxDocs_ is always positive. I know we don't
allow that many documents in an index but I think it would be good to have an
extra check.
* can we name _ maxMergeAtOnceThisMerge_ _ currentMaxMergeAtOnce_ or maybe just
_ maxMergeAtOnce_
I got down this quite a bit and I am starting to question if we should really
try to change the algorithm that we have today or if this class needs cleanup
and refactorings first. I am sorry to come in late here but this is a very very
complex piece of code and adding more complexity to it will rather do harm.
That said, I wonder if we can generalize the algorithm here into a single
method because in the end they all do the same thing. We can for instance make
the selection alg pluggable with a func we pass in and that way differentiate
between findMerges and findForceMerge etc. At the end of the day we want them
all to work in the same way. I am not saying we should go down all that way but
maybe we can extract a common code path that we can share between the places
were we filter out the segments that are not eligible.
This is just a suggestion, I am happy to help here btw. One thing that concerns
me and is in-fact a showstopper IMO is that the patch doesn't have a single
test that ensures it's correct. I mean we significantly change the behavior I
think it warrants tests no?
> Make TieredMergePolicy respect maxSegmentSizeMB and allow singleton merges of
> very large segments
> -------------------------------------------------------------------------------------------------
>
> Key: LUCENE-7976
> URL: https://issues.apache.org/jira/browse/LUCENE-7976
> Project: Lucene - Core
> Issue Type: Improvement
> Reporter: Erick Erickson
> Assignee: Erick Erickson
> Priority: Major
> Attachments: LUCENE-7976.patch, LUCENE-7976.patch, LUCENE-7976.patch,
> LUCENE-7976.patch, LUCENE-7976.patch, LUCENE-7976.patch, LUCENE-7976.patch
>
>
> We're seeing situations "in the wild" where there are very large indexes (on
> disk) handled quite easily in a single Lucene index. This is particularly
> true as features like docValues move data into MMapDirectory space. The
> current TMP algorithm allows on the order of 50% deleted documents as per a
> dev list conversation with Mike McCandless (and his blog here:
> https://www.elastic.co/blog/lucenes-handling-of-deleted-documents).
> Especially in the current era of very large indexes in aggregate, (think many
> TB) solutions like "you need to distribute your collection over more shards"
> become very costly. Additionally, the tempting "optimize" button exacerbates
> the issue since once you form, say, a 100G segment (by
> optimizing/forceMerging) it is not eligible for merging until 97.5G of the
> docs in it are deleted (current default 5G max segment size).
> The proposal here would be to add a new parameter to TMP, something like
> <maxAllowedPctDeletedInBigSegments> (no, that's not serious name, suggestions
> welcome) which would default to 100 (or the same behavior we have now).
> So if I set this parameter to, say, 20%, and the max segment size stays at
> 5G, the following would happen when segments were selected for merging:
> > any segment with > 20% deleted documents would be merged or rewritten NO
> > MATTER HOW LARGE. There are two cases,
> >> the segment has < 5G "live" docs. In that case it would be merged with
> >> smaller segments to bring the resulting segment up to 5G. If no smaller
> >> segments exist, it would just be rewritten
> >> The segment has > 5G "live" docs (the result of a forceMerge or optimize).
> >> It would be rewritten into a single segment removing all deleted docs no
> >> matter how big it is to start. The 100G example above would be rewritten
> >> to an 80G segment for instance.
> Of course this would lead to potentially much more I/O which is why the
> default would be the same behavior we see now. As it stands now, though,
> there's no way to recover from an optimize/forceMerge except to re-index from
> scratch. We routinely see 200G-300G Lucene indexes at this point "in the
> wild" with 10s of shards replicated 3 or more times. And that doesn't even
> include having these over HDFS.
> Alternatives welcome! Something like the above seems minimally invasive. A
> new merge policy is certainly an alternative.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
