Lucene - Core
  1. Lucene - Core
  2. LUCENE-1407

Refactor Searchable to not have RMI Remote dependency

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      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

      1. back_compat_20090607b_with_contrib.patch
        77 kB
        Simon Willnauer
      2. back_compat_20090607b_without_contrib.patch
        33 kB
        Simon Willnauer
      3. LUCENE-1407_with_back_compat.patch
        82 kB
        Simon Willnauer
      4. LUCENE-1407.patch
        82 kB
        Simon Willnauer

        Activity

        Hide
        Mark Miller added a comment -

        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.

        Show
        Mark Miller added a comment - 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.
        Hide
        Simon Willnauer added a comment -

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

        simon

        Show
        Simon Willnauer added a comment - This bugs me for a while already. Just started on refactoring it - Would be happy to get it into 2.9. simon
        Hide
        Michael McCandless added a comment -

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

        Show
        Michael McCandless added a comment - I'd also like to see this in 2.9 as well...
        Hide
        Shai Erera added a comment -

        In LUCENE-1630 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?

        Show
        Shai Erera added a comment - In LUCENE-1630 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?
        Hide
        Mark Miller added a comment -

        Looks like we flip back for now in either case

        Show
        Mark Miller added a comment - Looks like we flip back for now in either case
        Hide
        Shai Erera added a comment -

        Hmm ... maybe that won't be that simple. As part of LUCENE-1630, 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?

        Show
        Shai Erera added a comment - Hmm ... maybe that won't be that simple. As part of LUCENE-1630 , 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?
        Hide
        Simon Willnauer added a comment -

        Shai, I have looked at LUCENE-1630 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

        Show
        Simon Willnauer added a comment - Shai, I have looked at LUCENE-1630 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
        Hide
        Shai Erera added a comment -

        Shai, I have looked at LUCENE-1630 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.

        Show
        Shai Erera added a comment - Shai, I have looked at LUCENE-1630 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.
        Hide
        Shai Erera added a comment -

        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

        Show
        Shai Erera added a comment - 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
        Hide
        Shai Erera added a comment -

        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 LUCENE-1630 as-is in that area.

        Show
        Shai Erera added a comment - 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 LUCENE-1630 as-is in that area.
        Hide
        Simon Willnauer added a comment -

        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

        Show
        Simon Willnauer added a comment - 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
        Hide
        Simon Willnauer added a comment -

        Moved

        • org.apache.lucene.search.RemoteSearchable.java
        • org.apache.lucene.search.RemoteCachingWrapperFilter.java
          and depending test-cases to contrib/remote

        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.

        Show
        Simon Willnauer added a comment - Moved org.apache.lucene.search.RemoteSearchable.java org.apache.lucene.search.RemoteCachingWrapperFilter.java and depending test-cases to contrib/remote 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.
        Hide
        Michael McCandless added a comment -

        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.

        Show
        Michael McCandless added a comment - 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.
        Hide
        Simon Willnauer added a comment -

        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

        Show
        Simon Willnauer added a comment - 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
        Hide
        Michael McCandless added a comment -

        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!

        Show
        Michael McCandless added a comment - 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!
        Hide
        Shai Erera added a comment -

        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 LUCENE-1630, I'll make the necessary updates to RemoteSearchable - implementing the new methods that will be added.

        Show
        Shai Erera added a comment - 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 LUCENE-1630 , I'll make the necessary updates to RemoteSearchable - implementing the new methods that will be added.
        Hide
        Simon Willnauer added a comment -

        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

        Show
        Simon Willnauer added a comment - 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
        Hide
        Michael McCandless added a comment -

        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.

        Show
        Michael McCandless added a comment - 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.
        Hide
        Michael McCandless added a comment -

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

        Show
        Michael McCandless added a comment - Patch looks good Simon! Thanks. I plan to commit in a day or two...
        Hide
        Michael McCandless added a comment -

        Thanks Simon!

        Show
        Michael McCandless added a comment - Thanks Simon!

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Grant Ingersoll
          • Votes:
            3 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development