search

apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.39k stars 949 forks source link

Refactor Searchable to not have RMI Remote dependency [LUCENE-1407] #2481

Closed asfimport closed 14 years ago

asfimport commented 15 years ago

Per http://lucene.markmail.org/message/fu34tuomnqejchfj?q=RemoteSearchable

We should refactor Searchable slightly so that it doesn't extend the java.rmi.Remote marker interface. I believe the same could be achieved by just marking the RemoteSearchable and refactoring the RMI implementation out of core and into a contrib.

If we do this, we should deprecate/denote it for 2.9 and then move it for 3.0

Migrated from LUCENE-1407 by Grant Ingersoll (@gsingers), 3 votes, resolved Jun 14 2009 Attachments: back_compat_20090607b_with_contrib.patch, back_compat_20090607b_without_contrib.patch, LUCENE-1407_with_back_compat.patch, LUCENE-1407.patch

asfimport commented 14 years ago

Mark Miller (@markrmiller) (migrated from JIRA)

No work yet on this issue right? I'm going to pull off the 2.9 and leave the 3.0. Feel free to flip back if you plan on doing it.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

This bugs me for a while already. Just started on refactoring it - Would be happy to get it into 2.9.

simon

asfimport commented 14 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

I'd also like to see this in 2.9 as well...

asfimport commented 14 years ago

Shai Erera (@shaie) (migrated from JIRA)

In #2704 I'm going to do just that, as part of deprecating Weight in favor of a new QueryWeight. I'm going to deprecate Searchable entirely. So perhaps we should wait with this until 1630 is resolved, or fold that issue under 1630?

asfimport commented 14 years ago

Mark Miller (@markrmiller) (migrated from JIRA)

Looks like we flip back for now in either case

asfimport commented 14 years ago

Shai Erera (@shaie) (migrated from JIRA)

Hmm ... maybe that won't be that simple. As part of #2704, I'm deprecating Searchable, and the plan is to stick with just Searcher. Now, RemoteSearchable today implements Searchalbe (and by inheritance java.rmi.Remote) as well as extend UnicastRemoteObject.

After refactoring, it will need to extend both Searcher and UnicastRemoteObject. Unless we make Searcher extend UnicastRemoteObject, but that will bring the RMI stuff back into core.

Anybody got an idea how we can work around that besides keeping Searchable, or have Searcher extend UnicastRemoteObject?

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Shai, I have looked at #2704 but haven't seen any patch available. I looked briefly at the comments and what you are doing and I feel removing Searchable in 2.9 is a heavy change and will most likely break back compat. The RMI related classes this issue is about will just be moved to contrib but the problem which are you talking about will stay the same. With or without a patch for this issue. Removing the RMI dependency would be a great step forward and will not keep you from working on your patch.

Simon

asfimport commented 14 years ago

Shai Erera (@shaie) (migrated from JIRA)

Shai, I have looked at #2704 but haven't seen any patch available

It's in the oven :)

I feel removing Searchable in 2.9 is a heavy change and will most likely break back compat.

Back-compat is already broken in 2.9 due to the Collector work. So people who extend Searcher will need to implement the new search(***, Collector) methods. As I understood it, Searcher was really a replacement for Searchable, and I suspect it was left there not deprecated just because of RMI.

We preferred to add new methods to Searcher as abstract, rather than to Searchable, b/c we want to move away from interfaces. Therefore I don't think that delaying that work will do any good - we'll have more methods to maintain on the interface as well.

But to be honest I don't know what's better. I can have Searcher extend the RMI class, but that will leave RMI in core. Or I can have Searchable implement new methods, but that won't take us in the ultimate direction we want - getting rid of interfaces.

asfimport commented 14 years ago

Shai Erera (@shaie) (migrated from JIRA)

I did the following: removed the "extends Remote" from Searchable and move it to RemoteSearchable. But TestSort.testRemoteSort fails to lookup the searchable. The object it returns from lookup is of type Remote, while if Searchable extends Remote, the object returned from lookup is Searchable. I'm still debugging this, but if someone knows what's going on, please don't be shy :)

asfimport commented 14 years ago

Shai Erera (@shaie) (migrated from JIRA)

From UnicastRemoteObject's documentation:

If the remote object is exported using the exportObject(Remote) UnicastRemoteObject.exportObject(Remote) method, a stub class (typically pregenerated from the remote object's class using the rmic tool) is loaded and an instance of that stub class is constructed as follows.

  • A "root class" is determined as follows: if the remote object's class directly implements an interface that extends Remote, then the remote object's class is the root class; otherwise, the root class is the most derived superclass of the remote object's class that directly implements an interface that extends Remote.

Naming.lookup only succeeds when Searchable extends Remote, and RemoteSearchable implements Searchable. In fact, the type returned by lookup() is Searchable, not even RemoteSearchable.

I guess we can solve it by having RemoteSearchable implement a Remoteable interface which extends Remote .. but that's just spooky. For now I left the code in #2704 as-is in that area.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Shai, you miss one little tricky thing with RMI. RMI uses a DynamicProxy (java.lang.reflect.Proxy) together with a java.lang.reflect.InvocationHandler to proxy the remote call. A proxy can only be created using an interface. All calls to the interface are passed to the InvocationHandler instance and subsequently to the remote server. You can only cast an object returned by lookup() into an interface implemented by the remote object. If you would implement Remotable in you remote object you can only cast it into Remotable (while I have never seen that interface). This will only offer you the methods of this interface but not the methods of the remote object, in our case RemoteSeachable. Eventually we need an interface that defines all methods needed to use RemoteSearchable, without a base interface this is not gonna work together with RMI.

simon

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Moved

Removed ant#rmci call from build.xml Added build.xml to contrib/remote Added pom.xml.template to contrib/remote Added package.html to contrib/remote/src/java/org/apache/lucene/search/

Ideally, the move should be done using svn copy which does not appear in a diff afaik.

I did run test-cases with success. After finishing the patch I removed the Java standard library and compiled against android.jar version 1.1 / 1.5 and did not get any compile errors. I did not run the build in an emulator neither did I run any test-cases in it I guess that is little out of scope but I will do it one in the near future just in case I can catch another problem.

This change has one downside :(, the ant target "test-tag" will fail which is correct in my eyes as this patch breaks the compatibility in a certain way. I do have some ideas to make is succeed but I did not include them in the patch.

asfimport commented 14 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

the ant target "test-tag" will fail which is correct in my eyes as this patch breaks the compatibility in a certain way.

This is actually fine, assuming we're OK with making an exception (to break back-compat), here (which I believe we are – if anyone objects, speak up!).

If you can work out the patch to src/tags/... source code, and attach it, then when we commit this we'll commit the fixes to the back-compat branch, too.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

What do you mean by: If you can work out the patch to src/tags/... source code, and attach it, then when we commit this we'll commit the fixes to the back-compat branch, too.

Are you talking about http://svn.apache.org/repos/asf/lucene/java/branches/lucene_2_4_back_compat_tests/? please gimme some more details on that.

thanks, simon

asfimport commented 14 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

Woops, sorry... "ant test-tag" check out that branch into your local area, eg under tags/lucene_2_4_back_compat_tests_2009060.

Under there is src/test/... which you can go ahead and fix up with "accepted" changes to back-compat, until it passes "ant test-tag".

Then, from within there do "svn diff" and attach that patch here.

Thanks!

asfimport commented 14 years ago

Shai Erera (@shaie) (migrated from JIRA)

If you would implement Remotable in you remote object you can only cast it into Remotable (while I have never seen that interface).

By Remotable I meant exactly what you came up eventually with RMIRemoteSearchable - a mediator class between Searchable and RemoteSearchable which also extends Remote.

If this issue will be committed before #2704, I'll make the necessary updates to RemoteSearchable - implementing the new methods that will be added.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Attached patches for back compat tag lucene_2_4_back_compat_tests_20090607b The patch /back_compat_20090607b_without_contrib.patch contains only the changes to build.xml, /src/java and /src/test while back_compat_20090607b_with_contrib.patch has also the created remote contrib project in it.

The third patch (LUCENE-1407_with_back_compat.patch) is an update to the original LUCENE-1407.patch where I addtionally removed the rmic call in the test-tag target. This call is obsolete and would fail as the RemoteSearchable.java is not present in src/java.

I did run ant test and ant test-tag on trunk with the local modified compat tag directory set to tags-dir. The test do all pass on trunk. I did also run ant test on the compat tag where a lot of unrelated tests do fail but I guess that is due to modifications on accepted changes, correct?!

simon

asfimport commented 14 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

I did also run ant test on the compat tag where a lot of unrelated tests do fail but I guess that is due to modifications on accepted changes, correct?!

Yeah, this won't work (and yeah, it's confusing). You can't checkout the back compat tag dir and run its tests because many changes we've made to src/test/* on that branch rely on trunk. Ie we only really use src/test/* on the back compat branch.

asfimport commented 14 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

Patch looks good Simon! Thanks. I plan to commit in a day or two...

asfimport commented 14 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

Thanks Simon!