Solr
  1. Solr
  2. SOLR-135

Restructure / Refactor codebase for shared libraries

    Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3
    • Component/s: None
    • Labels:
      None

      Description

      For SOLR-20 and other java projects, it would be nice to have common code share a codebase that does not require lucene or junit to compile.

        Issue Links

          Activity

          Hide
          Ryan McKinley added a comment -

          A first start for this would be to move:
          /solr/src/java/org/apache/solr/util/
          to:
          /solr/src/common/org/apache/solr/util/

          (or some other name, "shared"?)

          It would also be nice to move:
          AbstractSolrTestCase.java, TestHarness.java, and o.a.s.util.test to:
          \solr\src\test\org\apache\solr\util

          As a first pass, SolrParams.java and ContentStream.java should be moved to:
          /solr/src/common/org/apache/solr/request/

          Longer term, the logic and constants should be seperated from SolrParams.java

          I think a quick version of this could be done that would not affect any existing code, just the build.xml file.

          • - - - - -

          should o.a.s.tst be moved to \solr\src\test\org\apache\solr\tst?

          I can't quite figure out what "tst" does.

          Show
          Ryan McKinley added a comment - A first start for this would be to move: /solr/src/java/org/apache/solr/util/ to: /solr/src/common/org/apache/solr/util/ (or some other name, "shared"?) It would also be nice to move: AbstractSolrTestCase.java, TestHarness.java, and o.a.s.util.test to: \solr\src\test\org\apache\solr\util As a first pass, SolrParams.java and ContentStream.java should be moved to: /solr/src/common/org/apache/solr/request/ Longer term, the logic and constants should be seperated from SolrParams.java I think a quick version of this could be done that would not affect any existing code, just the build.xml file. - - - - - should o.a.s.tst be moved to \solr\src\test\org\apache\solr\tst? I can't quite figure out what "tst" does.
          Hide
          Yonik Seeley added a comment -

          Sounds good in general, I think.

          The test request handler (in tst), was for leveraging existing tests but doing additional tests for every test.
          Things like testing that if one requests a DocList and a DocSet of the same query, all the documents are identical, tested filtering by a query (before we had things like fq, etc), tested the doc iteration interface, intersectionSize, etc.

          We have enough other tests now, most of it is probably redundant.
          I think the entire "tst" directory can be removed.

          Show
          Yonik Seeley added a comment - Sounds good in general, I think. The test request handler (in tst), was for leveraging existing tests but doing additional tests for every test. Things like testing that if one requests a DocList and a DocSet of the same query, all the documents are identical, tested filtering by a query (before we had things like fq, etc), tested the doc iteration interface, intersectionSize, etc. We have enough other tests now, most of it is probably redundant. I think the entire "tst" directory can be removed.
          Hide
          Ryan McKinley added a comment -

          This patch moves a single file (XML.java) into a new source directory and modifies build.xml to generate the proper .jar files.

          The structure is:
          /src/common/
          /src/java/

          output is:
          /dist/apache-solr-1.3-dev.jar
          /dist/apache-solr-1.3-dev-common.jar

          I used "common" rather then "util" since SOLR-20 needs many files that are not in util and without a lucene dependency, it should not have them – if anyone has better names, let me know!

          • - - -

          Someone more familiar with ant "best practices" needs to look over these changes!

          • - - -

          I have been unable to make a patch that successfully moves XML.java from /src/java to /src/common, when you test this, just copy the file over – no changes.

          Show
          Ryan McKinley added a comment - This patch moves a single file (XML.java) into a new source directory and modifies build.xml to generate the proper .jar files. The structure is: /src/common/ /src/java/ output is: /dist/apache-solr-1.3-dev.jar /dist/apache-solr-1.3-dev-common.jar I used "common" rather then "util" since SOLR-20 needs many files that are not in util and without a lucene dependency, it should not have them – if anyone has better names, let me know! - - - Someone more familiar with ant "best practices" needs to look over these changes! - - - I have been unable to make a patch that successfully moves XML.java from /src/java to /src/common, when you test this, just copy the file over – no changes.
          Hide
          Hoss Man added a comment -

          one the topic of adding a src/common directory ... i think in the long run we'll be happier if there is no overlap in the java package names that live in this directory and the ones that live in src/java (much the way the only java packages in src/webapp are o.a.s.servlet) ... so using src/common/org/apache/solr/common/XML.java may be a better way to go (even though it means we would need to leave a deprecated src/java/org/apache/solr/util/XML.java subclassing it in src/java) I could probably be convinced that this isn't that important, but i've definitely found it confusing for people that some of the lucene-java contribs reuse the same package names as the core classes in some cases)

          on the subject of the build.xml ... now that we've got three instances of <javac> and two of <jar> we probably want to make some macros for them to reduce redundency.

          Gimme 30 minutes to see if i can whip up a derivitive patch ... if i dont' attach one it means i got sidetracked with something else.

          Show
          Hoss Man added a comment - one the topic of adding a src/common directory ... i think in the long run we'll be happier if there is no overlap in the java package names that live in this directory and the ones that live in src/java (much the way the only java packages in src/webapp are o.a.s.servlet) ... so using src/common/org/apache/solr/common/XML.java may be a better way to go (even though it means we would need to leave a deprecated src/java/org/apache/solr/util/XML.java subclassing it in src/java) I could probably be convinced that this isn't that important, but i've definitely found it confusing for people that some of the lucene-java contribs reuse the same package names as the core classes in some cases) on the subject of the build.xml ... now that we've got three instances of <javac> and two of <jar> we probably want to make some macros for them to reduce redundency. Gimme 30 minutes to see if i can whip up a derivitive patch ... if i dont' attach one it means i got sidetracked with something else.
          Hide
          Hoss Man added a comment -

          here's what i've got so far ...it occured to me that if we use seperate package names, we don't actually need to separate the code out, we can do it all with exclude/include directives.

          this has one small glitch at the moment, post.jar isnt' getting it's main-class set properly ... might be a mistake i made, or it might be a defect in the manifest merging ant is suppose to do ... i'll check it out later

          (this isn't a big deal though, post.jar has never really had a good manifest file, i was just trying to clean that up when i added the macro)

          Show
          Hoss Man added a comment - here's what i've got so far ...it occured to me that if we use seperate package names, we don't actually need to separate the code out, we can do it all with exclude/include directives. this has one small glitch at the moment, post.jar isnt' getting it's main-class set properly ... might be a mistake i made, or it might be a defect in the manifest merging ant is suppose to do ... i'll check it out later (this isn't a big deal though, post.jar has never really had a good manifest file, i was just trying to clean that up when i added the macro)
          Hide
          Hoss Man added a comment -

          as far as i can tell the manifest merging that the ant docs describe for the <jar> task just flat out don't work, so we just wont use the new macro for hte post.jar

          NOTE: with this patch, the intent is to svn copy XML.java to the new common dir, then patch the existing file to purge it's body and add the deprecated messages.

          as i mentioned before, this appraoch doesn't use src/common at all ... it assumes a new "org.apache.solr.common" package in src/java and uses include/exclude rules to make sure things in that package live in the common jar (and compile first)

          it would be easy to move this package to live in src/common if people think there is a need, my main concern is just that we shouldn't have "org.apache.solr.util" living in two places (src/java and src/common)

          Show
          Hoss Man added a comment - as far as i can tell the manifest merging that the ant docs describe for the <jar> task just flat out don't work, so we just wont use the new macro for hte post.jar NOTE: with this patch, the intent is to svn copy XML.java to the new common dir, then patch the existing file to purge it's body and add the deprecated messages. as i mentioned before, this appraoch doesn't use src/common at all ... it assumes a new "org.apache.solr.common" package in src/java and uses include/exclude rules to make sure things in that package live in the common jar (and compile first) it would be easy to move this package to live in src/common if people think there is a need, my main concern is just that we shouldn't have "org.apache.solr.util" living in two places (src/java and src/common)
          Hide
          Ryan McKinley added a comment -

          >
          >it would be easy to move this package to live in src/common if people think there is a need, my main concern is just that we shouldn't have "org.apache.solr.util" living in two places (src/java and src/common)
          >

          yes, I like separate package names better.... but i'm worried about the impact on dependent code.

          The classes needed for SOLR-20 are:
          http://solrstuff.org/svn/solrj/src/org/apache/solr/util/
          http://solrstuff.org/svn/solrj/src/org/apache/solr/request/
          http://solrstuff.org/svn/solrj/src/org/apache/solr/core/

          Are you suggesting its ok to move XML.java and SolrException.java to o.a.s.common? That seems kinda extreme for anyone using the classes...

          If it is ok, i'm all for it... if not, I think we should make the 'common' package and put anything new in there, adding comments to the classes that should be moved in the future.

          Show
          Ryan McKinley added a comment - > >it would be easy to move this package to live in src/common if people think there is a need, my main concern is just that we shouldn't have "org.apache.solr.util" living in two places (src/java and src/common) > yes, I like separate package names better.... but i'm worried about the impact on dependent code. The classes needed for SOLR-20 are: http://solrstuff.org/svn/solrj/src/org/apache/solr/util/ http://solrstuff.org/svn/solrj/src/org/apache/solr/request/ http://solrstuff.org/svn/solrj/src/org/apache/solr/core/ Are you suggesting its ok to move XML.java and SolrException.java to o.a.s.common? That seems kinda extreme for anyone using the classes... If it is ok, i'm all for it... if not, I think we should make the 'common' package and put anything new in there, adding comments to the classes that should be moved in the future.
          Hide
          Ryan McKinley added a comment -

          As a note to anyone not looking at the patch... this would not break API compatibility, but it would add a lot of empty classes that look like:

          @Deprecated
          public class XML extends org.apache.solr.common.XML

          { // don't use this class! }
          Show
          Ryan McKinley added a comment - As a note to anyone not looking at the patch... this would not break API compatibility, but it would add a lot of empty classes that look like: @Deprecated public class XML extends org.apache.solr.common.XML { // don't use this class! }
          Hide
          Hoss Man added a comment -

          >> yes, I like separate package names better.... but i'm worried about the impact on dependent code.
          >> ...
          >> Are you suggesting its ok to move XML.java and SolrException.java to o.a.s.common? That seems kinda extreme
          >> for anyone using the classes...

          I'm not sure. I think we've been talking for a long time about refactoring some of the classes into different packages, which really only affects their organization when developers look at them – if we are now also looking at reorganizing them into jars, and ensuring that certain subsets can be compiled into their own jar with no dependencie on files not in that jar – then i think we might as well do both at once. I said, I could probably be convinced that this isn't that important, and we should continue using the same package names in a new src/common directory – so perhaps a better question to ask is: do we want to rework the packages too?"

          Most of the classes you listed seem like perfect candidates for new "common" package (or at the very least o.a.s.common.util, o.a.s.common.params), but i have to admit i hadn't really considered SolrException ... on one hand it's used so pervasively it should be considered "common" (not including it would mean changing a lot of APIs of things we want to be able to include in the common jar) on the other hand it does have very HTTP specific error codes in it.

          Just spit balling here... what if o.a.s.common.SolrException was a base class with no error codes (it looks like all of the "Common" classes just use "BAD_REQUEST" at this point so refactoring it out would be clean, and the http codes don't make sense in a 'common' context anyway) and o.a.s.util.SolrException a real (non deprecated) subclass that adds the ErrorCodes ... anyone catching util.SolrException is golden, anyone catching common.SolrException can either infer an ErrorCode from context, or assume BAD_REQUEST (a static utility in util.SolrException could make this easy by wrapping the common.SolrException in a util.SolrException.

          ugh.

          Show
          Hoss Man added a comment - >> yes, I like separate package names better.... but i'm worried about the impact on dependent code. >> ... >> Are you suggesting its ok to move XML.java and SolrException.java to o.a.s.common? That seems kinda extreme >> for anyone using the classes... I'm not sure. I think we've been talking for a long time about refactoring some of the classes into different packages, which really only affects their organization when developers look at them – if we are now also looking at reorganizing them into jars, and ensuring that certain subsets can be compiled into their own jar with no dependencie on files not in that jar – then i think we might as well do both at once. I said, I could probably be convinced that this isn't that important, and we should continue using the same package names in a new src/common directory – so perhaps a better question to ask is: do we want to rework the packages too?" Most of the classes you listed seem like perfect candidates for new "common" package (or at the very least o.a.s.common.util, o.a.s.common.params), but i have to admit i hadn't really considered SolrException ... on one hand it's used so pervasively it should be considered "common" (not including it would mean changing a lot of APIs of things we want to be able to include in the common jar) on the other hand it does have very HTTP specific error codes in it. Just spit balling here... what if o.a.s.common.SolrException was a base class with no error codes (it looks like all of the "Common" classes just use "BAD_REQUEST" at this point so refactoring it out would be clean, and the http codes don't make sense in a 'common' context anyway) and o.a.s.util.SolrException a real (non deprecated) subclass that adds the ErrorCodes ... anyone catching util.SolrException is golden, anyone catching common.SolrException can either infer an ErrorCode from context, or assume BAD_REQUEST (a static utility in util.SolrException could make this easy by wrapping the common.SolrException in a util.SolrException. ugh.
          Hide
          Ryan McKinley added a comment -

          > do we want to rework the packages too?

          I think now is as good of a time as any. I agree that having independent jar files with classes in the same package is less then great.

          > on the other hand it does have very HTTP specific error codes in it.
          >

          I don't see any problem with HTTP status codes in the base class. It seems standard enough. I think we may want to have multiple error types that overlap with the same HTTP status code (500)

          (Not now, but...) I would like to see typed exceptions so they could be handled differently – BadRequestException() etc...

          Show
          Ryan McKinley added a comment - > do we want to rework the packages too? I think now is as good of a time as any. I agree that having independent jar files with classes in the same package is less then great. > on the other hand it does have very HTTP specific error codes in it. > I don't see any problem with HTTP status codes in the base class. It seems standard enough. I think we may want to have multiple error types that overlap with the same HTTP status code (500) (Not now, but...) I would like to see typed exceptions so they could be handled differently – BadRequestException() etc...
          Hide
          Ryan McKinley added a comment -

          phew! The changelog frenzy was successful. Thank you eclipse.

          The moved files have their revision history:
          http://svn.apache.org/viewvc/lucene/solr/trunk/src/java/org/apache/solr/common/params/SolrParams.java?view=log

          and stub classs are all in place.

          Since we have moved these files to a new location, I think we should try to clean up the depricated classes like CommonParams. There are essentially two types of "Param" classes

          1. public static final String PARAM_NAME lists...
          2. SolrParams helpers

          but we should work on that later...

          Show
          Ryan McKinley added a comment - phew! The changelog frenzy was successful. Thank you eclipse. The moved files have their revision history: http://svn.apache.org/viewvc/lucene/solr/trunk/src/java/org/apache/solr/common/params/SolrParams.java?view=log and stub classs are all in place. Since we have moved these files to a new location, I think we should try to clean up the depricated classes like CommonParams. There are essentially two types of "Param" classes 1. public static final String PARAM_NAME lists... 2. SolrParams helpers but we should work on that later...
          Hide
          Hoss Man added a comment -

          Whoops ... while banging my head against my desk trying ot figure out why i couldn't see my changes, i discovered that the last build.xml patch i attached to this issue had a really bad bug in that it was compiling all of hte classes into both jars (core and common)

          i have fixed this locally but it turns out it was masking another problem: CommonParams and DisMaxParams have a compilation dependency on SolrCore (and possible other things i haven't found yet because javac gave up when it hit that error) to log errors.

          I'm going to change these classes to have their own loggers ... if that's hte only problem i'll commit, if there are still more problems i'll udpate this bug with a patch for discussion

          Show
          Hoss Man added a comment - Whoops ... while banging my head against my desk trying ot figure out why i couldn't see my changes, i discovered that the last build.xml patch i attached to this issue had a really bad bug in that it was compiling all of hte classes into both jars (core and common) i have fixed this locally but it turns out it was masking another problem: CommonParams and DisMaxParams have a compilation dependency on SolrCore (and possible other things i haven't found yet because javac gave up when it hit that error) to log errors. I'm going to change these classes to have their own loggers ... if that's hte only problem i'll commit, if there are still more problems i'll udpate this bug with a patch for discussion
          Hide
          Hoss Man added a comment -

          Committed revision 547455.

          Show
          Hoss Man added a comment - Committed revision 547455.
          Hide
          Hoss Man added a comment -

          This bug was modified as part of a bulk update using the criteria...

          • Marked "Resolved" and "Fixed"
          • Had no "Fix Version" versions
          • Was listed in the CHANGES.txt for 1.3 as of today 2008-03-15

          The Fix Version for all 29 issues found was set to 1.3, email notification was suppressed to prevent excessive email.

          For a list of all the issues modified, search jira comments for this (hopefully) unique string: batch20070315hossman1

          Show
          Hoss Man added a comment - This bug was modified as part of a bulk update using the criteria... Marked "Resolved" and "Fixed" Had no "Fix Version" versions Was listed in the CHANGES.txt for 1.3 as of today 2008-03-15 The Fix Version for all 29 issues found was set to 1.3, email notification was suppressed to prevent excessive email. For a list of all the issues modified, search jira comments for this (hopefully) unique string: batch20070315hossman1

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Ryan McKinley
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development