[ https://issues.apache.org/jira/browse/LUCENE-847?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12520293 ]
Steven Parkes commented on LUCENE-847: -------------------------------------- One new small item: you've added a "public void merge()" to IndexWriter so that people can externally kick off a merge request, which is good I think. But, is it really necessary to flush here? It would be better to not flush so that users then have two separate methods (flush() and merge()) to do each function independently. That makes sense. Note that merge() was added not for users (which I have no strong opinion about) but so that, potentially, CMP can check again for merges when a set of merge threads completes, i.e., cascade. I think instead we should leave the methods, not deprecated, as convenience (sugar) methods. Simple things should be simple; complex things should be possible. I think this argues for a LegacyMergePolicy interface again, then? If we change the default merge policy and someone changes their code to use LogDoc for their own purposes, in both cases the getters/setters should work? So cast to the interface and as long as the merge policy supports this, the getters/setters work (unless the merge policy decides to throw within), otherwise the getters/setters throw? Uh, no: when someone calls optimize that means it really must be done, right? So "optimize" is the right name I think. Yeah, but it might do nothing. Just as merge might do nothing. Can you factor this out, eg add a private method "getLogDocMergePolicy(String reason)" Sure. Looks good, thanks. Can you add javadocs (w/ params) for both of these new methods? Sure. Though, this raises the tricky question of index consistency ... Definitely. I'm still trying to understand all the subtleties here. IndexWriter commits the new segments file right after mergePolicy.merge returns ... so for CMP we suddenly have an unusable index (as seen by an IndexReader). How so? I figured that after mergePolicy.merge returns, in the case of CMP with an ongoing merge, segmentInfos won't have changed at all. Is that a problem? I thought the issue would be on the other end, where the concurrent merge finishes and needs to update segmentInfos. Maybe it's too ambitious to allow merges of segments from other directories to run concurrently? Yeah, that might be the case. At least as a default? I would consider it a hard error in IndexWriter if after calling mergePolicy.merge from any of the addIndexes*, there remain segments in other directories. I think we should catch this & throw an exception? It would be easy enough for CMP to block in this case, rather than returning immediately. Wouldn't that be better? And I suppose it's possible to imagine an API on CMP for specifying this behavior? I opened a separate issue LUCENE-982 to track this. I think this is good. I think it's an interesting issue but not directly related to the refactor? Although ... do you think we need need some way for merge policy to state where the new segment should be inserted into SegmentInfos? Right now I assumed it would replace the left most-segment. Since I don't really know the details of what such a merge policy would like, I don't really know what it needs. If you've thought about this more, do you have a suggestion? I suppose we could just add an int. But, then again, I'd do that as a separate function, leaving the original available, so we can do this later, completely compatibly? Hmmm, does CMP block on close while it joins to any running merge threads? Yeah, at least in my sandbox. How can the user close IndexWriter and abort the running merges? I guess CMP would provide a method to abort any running merges, and user would first call that before calling IndexWriter.close? I hadn't really thought about this but I can see that should be made possible. It's always safe to abandon a merge so it should be available, for fast, safe, and clean shutdown. True, LogDoc as it now stands would never exploit concurrency (it will always return the highest level that needs merging). But, we could relax that such that if ever the lowest level has > 2*mergeFactor pending segments to merge then we select the 2nd set. Okay. But it will always return that? Still doesn't sound concurrent? The thing is, the serial merge policy has no concept of concurrent merges, so if the API is always to select the best merge, until a pervious merge finishes, it will always return that as the best merge. Concurrent is going to require, by hook or by crook, that a merge policy be able to generate a set of non-conflicting merges, is it not? I think the LUCENE-845 merge policy does this now, given that CMP gathers up the merge calls. I'm not sure the current LUCENE-847 merge policy does (I'd have to double check) because it sometimes will try to use the result of the current merge in the next merge. The LUCENE-845 merge doesn't try to do this which is a(n) (inconsequential) change? This is another benefit of the simplified API: MergePolicy.maybeMerge would only be called with a lock already acquired (by IndexWriter) on the segmentInfos. Do you really mean a lock on segmentInfos or just the lock on IndexWriter? I'm assuming the latter and I think this is the case for both API models. I don't think it's feasible to have a lock on segmentInfos separately. Only IndexWriter should change segmentInfos and no code should try to look at segmentInfos w/o being called via an IW synch method. This does imply that CMP has to copy any segmentInfos data it plans to use during concurrent merging, since the IW lock is not held during these periods. Then, when the merge is done, segmentInfos is updated in IndexWriter via a synch call to IW#replace. This means IW#segmentInfos can change while a merge is in progress and this has to be accounted for. That's what I'm walking through now. > Factor merge policy out of IndexWriter > -------------------------------------- > > Key: LUCENE-847 > URL: https://issues.apache.org/jira/browse/LUCENE-847 > Project: Lucene - Java > Issue Type: Improvement > Components: Index > Reporter: Steven Parkes > Assignee: Steven Parkes > Attachments: concurrentMerge.patch, LUCENE-847.patch.txt, > LUCENE-847.patch.txt, LUCENE-847.txt > > > If we factor the merge policy out of IndexWriter, we can make it pluggable, > making it possible for apps to choose a custom merge policy and for easier > experimenting with merge policy variants. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online. --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]