Bug 335144

Comment 1 Chris Goldthorpe 2011-01-25 16:17:32 EST

I understand the problem, I'm not yet sold on the proposed solution. It seems that the ideal solution would allow you to have more than one version of a documentation plug-in and have a mechanism which ensured that the correct one was loaded based on the version of another plug-in. I don't have enough familiarity with the algorithm used when loading plug-ins to know if this could be achieved by using dependencies with version constraints but it seems that it should work.

There are reasons for having only one index per plug-in, the task to build an index is based on the concept of having a single location where the index is stored.

Comment 2 Holger Voormann 2011-01-26 05:41:43 EST

(In reply to comment #1)
Do you mean that the whole help content plug-in should be duplicated? This would mean that the download time and disc space will also be doubled (our help content plug-ins are hundreds of MBs and these plug-ins are shared (via the Eclipse help feature) between two Eclipse-based applications which may contain different versions of Lucene; in our case the size of index is less than 5% of the content).

The master index is build by merging prebuilt indexes and by adding the index of the plug-ins which do not contain an prebuilt index. I can see no reason why a plug-in should not contain more than one index. It seems that the first index or all indexes which can be used are merged into the master index and if no index can be used then the index is created from scratch. Probably, not an "undefined behaviour" but a specific behaviour is implemented. So why not document it? It seems it solves the problem of providing a help content plug-ins for different targets.

In contrast to the documentation a plug-in with two indexes works on my machine: Eclipse 3.6 uses the 1.9.1 index and Eclipse 3.7 M4 the 2.9.1 index. Would it help if I create an example plug-in with two indexes and attach it to this bug?

Comment 3 Chris Goldthorpe 2011-01-26 13:05:44 EST

Yes, an example would be helpful. I'm still not so sure that Eclipse 3.6 would handle the case of two declared indexes in the desired manner - Eclipse 3.7 can of course be modified if necessary.

Comment 4 Holger Voormann 2011-01-26 18:14:01 EST

Created attachment 187699 [details]
Help content example plug-in which contains both an Lucene 1.9.1 and 2.9.1 index

(In reply to comment #3)
See attachment

Comment 5 Chris Goldthorpe 2011-01-27 18:46:30 EST

I tried out the plug-in you attached and it does indeed load the index correctly and without any warnings in the log. I'm not sure why the warning exists in the documentation.

Comment 6 Holger Voormann 2011-01-27 22:34:10 EST

Maybe it works by mistake:

PluginIndex#equals() is:
    public boolean equals(Object obj) {
        return pluginId.equals(obj);
    }
Pleas note, "pluginId" is compared with the other object instead of the "pluginId" field of the other - maybe PluginIndex - object. If
    PluginIndex x = new PluginIndex("my ID", "my path", null);
then
    x.equals(x)
will return false but must be reflexive.

In PrebuiltIndexes#add() an instance of PluginIndex is added to an HashSet. PrebuiltIndexes#getIndexes() will probably return all indexes (but may also return no or only some indexes because of the broken equals() method in PluginIndex) and not only (randomly) one per plug-in as intended. Probably, this is why the warning in the documentation exists.

I propose to remove the warning in the documentation and the broke equals() method in PluginIndex instead of comparing plug-in IDs: if an plug-in contains more than one index then an index (a) should be not equal to another index (b).

Comment 7 Chris Goldthorpe 2011-01-28 12:59:21 EST

I agree with your proposal, targeting 3.7M6.

Comment 8 Holger Voormann 2011-01-28 23:35:01 EST

1.9.x prebuilt indexes seems to be compatible with a Lucene 2.x runtime. So we can add in SearchIndex#isLuceneCompatible(String) before the last return:

    // Lucene 2.x can read 1.9.x indexes
    if (   version.getMajor() == 2
        && currentVersion.getMajor() == 1
        && currentVersion.getMinor() == 9) return true;

As a result of this Eclipse 3.7 would be able to read indexes built with Eclipse 3.6 (or 3.5, 3.4, 3.3). I tested this with a very large 1.9.1 prebuilt index: the prebuilt indexes were used (very fast indexing) and the search results were correct, too.

Comment 9 Chris Goldthorpe 2011-01-31 19:38:02 EST

That seems like a better solution. I will change isLuceneCompatible() to allow it to read indexes 1.9.1 or greater and add JUnit tests to verify that a 1.9.1 index can be read.

Comment 10 Chris Goldthorpe 2011-02-01 13:40:09 EST

Lucene 2.9.1 has no problem reading indexes built with Lucene 1.9.1 so I have modified SearchIndex#isLuceneCompatible(String) to allow this, see Bug 335997. Thanks for your help in figuring this out.

Is there still a need for more than one prebuilt index per plug-in?

Comment 11 Holger Voormann 2011-02-08 06:12:46 EST

(In reply to comment #10)
> Is there still a need for more than one prebuilt index per plug-in?
>
No, there is no need anymore since bug 335997 solves the real problem much better. Thank you very much to make this work.

I still believe the current implementation of PluginIndex#equals() is wrong and should be removed because it returns false for any instance of PluginIndex.

Comment 12 Chris Goldthorpe 2011-02-08 13:11:06 EST

Created attachment 188539 [details]
Patch to fix the equals() method

Comment 13 Chris Goldthorpe 2011-02-08 13:16:24 EST

I agree about the equals() method, I have fixed it so that PluginId is equal when the path and plugin are both equal. This preserves the undocumented ability to declare multiple indexes.

As for the original problem, closing as WONTFIX