|
Sounds good in general, I think.
The test request handler (in tst), was for leveraging existing tests but doing additional tests for every test. We have enough other tests now, most of it is probably redundant. 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: output is: I used "common" rather then "util" since
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. 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. 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) 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) >
>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 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. 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 >> 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. > 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... phew! The changelog frenzy was successful. Thank you eclipse.
The moved files have their revision history: 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... but we should work on that later... 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 This bug was modified as part of a bulk update using the criteria...
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 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Wish
Resolved
Minor
Restructure / Refactor codebase for shared libraries
/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.