Solr
  1. Solr
  2. SOLR-2588

Make Velocity an optional dependency in SolrCore

    Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      In 1.4. it was fine to run Solr without Velocity on the classpath. However, in 3.2. SolrCore won't load because of a hard reference to the Velocity response writer in a static initializer.

      ... ERROR org.apache.solr.core.CoreContainer - java.lang.NoClassDefFoundError: org/apache/velocity/context/Context
      	at org.apache.solr.core.SolrCore.<clinit>(SolrCore.java:1447)
      	at org.apache.solr.core.CoreContainer.create(CoreContainer.java:463)
      	at org.apache.solr.core.CoreContainer.load(CoreContainer.java:316)
      	at org.apache.solr.core.CoreContainer.load(CoreContainer.java:207)
      
      1. SOLR-2588_Don_t_fail_if_velocity_libs_not_present_.patch
        2 kB
        David Smiley
      2. SOLR-2588.patch
        26 kB
        Steve Rowe
      3. SOLR-2588.patch
        12 kB
        Steve Rowe
      4. SOLR-2588.patch
        68 kB
        Erik Hatcher
      5. SOLR-2588.patch
        68 kB
        Erik Hatcher
      6. SOLR-2588.patch
        47 kB
        Steve Rowe
      7. SOLR-2588.patch
        46 kB
        Steve Rowe
      8. SOLR-2588.patch
        40 kB
        Erik Hatcher
      9. SOLR-2588.patch
        41 kB
        Steve Rowe
      10. SOLR-2588.patch
        50 kB
        Erik Hatcher
      11. SOLR-2588-maven.patch
        6 kB
        Steve Rowe

        Issue Links

          Activity

          Gunnar Wagenknecht created issue -
          Hide
          Mark Miller added a comment -

          I'm assuming that 1.4 did not offer the Velocity ResponseWriter? Or was that part of the velocity contrib and it was move to core?

          Would prefer if these libs where not required unless you used the functionality myself as well - have not look to see how easy that is to do. Not that I have anything against velocity, I think it's an awesome templating system...

          Show
          Mark Miller added a comment - I'm assuming that 1.4 did not offer the Velocity ResponseWriter? Or was that part of the velocity contrib and it was move to core? Would prefer if these libs where not required unless you used the functionality myself as well - have not look to see how easy that is to do. Not that I have anything against velocity, I think it's an awesome templating system...
          Mark Miller made changes -
          Field Original Value New Value
          Fix Version/s 3.3 [ 12316471 ]
          Hide
          Hoss Man added a comment -

          I don't understand this bug?

          in SOLR-1957 the velocity response writer was promoted from being a contrib to being part of the solr core so that the jars are all included in the solr.war and the velocity writer would be one of the writers provided by deault.

          nothing special should be needed on the classpath.

          Show
          Hoss Man added a comment - I don't understand this bug? in SOLR-1957 the velocity response writer was promoted from being a contrib to being part of the solr core so that the jars are all included in the solr.war and the velocity writer would be one of the writers provided by deault. nothing special should be needed on the classpath.
          Hide
          Mark Miller added a comment -

          Perhaps he is not using the webapp?

          Show
          Mark Miller added a comment - Perhaps he is not using the webapp?
          Hide
          Uwe Schindler added a comment -

          I generally also do not use the webapp directly, so thats not uncommon!

          Show
          Uwe Schindler added a comment - I generally also do not use the webapp directly, so thats not uncommon!
          Hide
          Ryan McKinley added a comment -

          "bug" is a strech... I think what this is getting at is that velocity is now required for solr to work at all. With some small changes, Velocity could be optional.

          I think somethign as easy as:

          Index: solr/src/java/org/apache/solr/core/SolrCore.java
          ===================================================================
          --- solr/src/java/org/apache/solr/core/SolrCore.java    (revision 1134331)
          +++ solr/src/java/org/apache/solr/core/SolrCore.java    (working copy)
          @@ -1381,7 +1381,12 @@
               m.put("ruby", new RubyResponseWriter());
               m.put("raw", new RawResponseWriter());
               m.put("javabin", new BinaryResponseWriter());
          -    m.put("velocity", new VelocityResponseWriter());
          +    try {
          +      m.put("velocity", new VelocityResponseWriter());
          +    }
          +    catch( Throwable t ) {
          +      log.warn("Error initalizing VelocityResponseWriter", t );
          +    }
               m.put("csv", new CSVResponseWriter());
               DEFAULT_RESPONSE_WRITERS = Collections.unmodifiableMap(m);
             }
          

          Is all he is talking about... but I'm not sure how/if we want to deal with the error being gobbled...

          perhaps something smarter to see if Velocity can be created before trying?

          Show
          Ryan McKinley added a comment - "bug" is a strech... I think what this is getting at is that velocity is now required for solr to work at all. With some small changes, Velocity could be optional. I think somethign as easy as: Index: solr/src/java/org/apache/solr/core/SolrCore.java =================================================================== --- solr/src/java/org/apache/solr/core/SolrCore.java (revision 1134331) +++ solr/src/java/org/apache/solr/core/SolrCore.java (working copy) @@ -1381,7 +1381,12 @@ m.put( "ruby" , new RubyResponseWriter()); m.put( "raw" , new RawResponseWriter()); m.put( "javabin" , new BinaryResponseWriter()); - m.put( "velocity" , new VelocityResponseWriter()); + try { + m.put( "velocity" , new VelocityResponseWriter()); + } + catch ( Throwable t ) { + log.warn( "Error initalizing VelocityResponseWriter" , t ); + } m.put( "csv" , new CSVResponseWriter()); DEFAULT_RESPONSE_WRITERS = Collections.unmodifiableMap(m); } Is all he is talking about... but I'm not sure how/if we want to deal with the error being gobbled... perhaps something smarter to see if Velocity can be created before trying?
          Hide
          Gunnar Wagenknecht added a comment -

          Thanks Ryan, that's exactly what I was thinking of.

          FWIW, it's fine to have support for Velocity or XYZ in core. But this shouldn't require everybody to inherit the dependency into their apps. For me it's an additional dependency that I have to get through an IP/due diligence process and that I have to keep track of security updates, etc. even if I don't use it in my app.

          Show
          Gunnar Wagenknecht added a comment - Thanks Ryan, that's exactly what I was thinking of. FWIW, it's fine to have support for Velocity or XYZ in core. But this shouldn't require everybody to inherit the dependency into their apps. For me it's an additional dependency that I have to get through an IP/due diligence process and that I have to keep track of security updates, etc. even if I don't use it in my app.
          Hide
          Hoss Man added a comment -

          With some small changes, Velocity could be optional.

          Velocity (and the velocitywriter) were optional before, and a conscious and deliberate choice was made to promote it into a core dependency so that admin code (and users) could start expecting it to reliable always work.

          if we want to re-consider i'm fine with having that discussion, but it shouldn't be an "optional core" feature ... either it's a core feature and dependency, or it's an optional contrib.

          It's not a bug that code in solr has a direct dependency on jars in the lib dir.

          Show
          Hoss Man added a comment - With some small changes, Velocity could be optional. Velocity (and the velocitywriter) were optional before, and a conscious and deliberate choice was made to promote it into a core dependency so that admin code (and users) could start expecting it to reliable always work. if we want to re-consider i'm fine with having that discussion, but it shouldn't be an "optional core" feature ... either it's a core feature and dependency, or it's an optional contrib. It's not a bug that code in solr has a direct dependency on jars in the lib dir.
          Hide
          Ryan McKinley added a comment -

          updating the JIRA description... since this is not a 'bug'

          Show
          Ryan McKinley added a comment - updating the JIRA description... since this is not a 'bug'
          Ryan McKinley made changes -
          Summary Solr doesn't work without Velocity on classpath Make Velocity an optional dependency in SolrCore
          Issue Type Bug [ 1 ] Wish [ 5 ]
          Priority Major [ 3 ] Minor [ 4 ]
          Robert Muir made changes -
          Fix Version/s 3.4 [ 12316683 ]
          Fix Version/s 4.0 [ 12314992 ]
          Fix Version/s 3.3 [ 12316471 ]
          Hide
          David Smiley added a comment -

          I'm surprised velocity became a core dependency, but nonetheless I think it should be possible to use Solr in an embedded fashion without pulling in extraneous dependencies like velocity and others. What if these response writers were initialized on-demand? This would increase startup time & decrease memory usage just a little since most people aren't actually going to use all response writers that Solr supports. I'm willing to put together a patch.

          Show
          David Smiley added a comment - I'm surprised velocity became a core dependency, but nonetheless I think it should be possible to use Solr in an embedded fashion without pulling in extraneous dependencies like velocity and others. What if these response writers were initialized on-demand? This would increase startup time & decrease memory usage just a little since most people aren't actually going to use all response writers that Solr supports. I'm willing to put together a patch.
          Hide
          Erik Hatcher added a comment -

          Sorry - I missed this when it first got posted, and David's comment bumped it... it was intentional to make Velocity a core component as the idea being that we'd use it for built-in admin UI. So far we're only using it for the /browse interface though.

          I get the argument that Velocity ideally shouldn't be required to "embed" Solr though. I'm ok with the Velocity writer creation either being in the try/catch as Ryan posted, or pulling it out of the default writers and having it be explicitly configured in solrconfig.xml for our example app.

          Show
          Erik Hatcher added a comment - Sorry - I missed this when it first got posted, and David's comment bumped it... it was intentional to make Velocity a core component as the idea being that we'd use it for built-in admin UI. So far we're only using it for the /browse interface though. I get the argument that Velocity ideally shouldn't be required to "embed" Solr though. I'm ok with the Velocity writer creation either being in the try/catch as Ryan posted, or pulling it out of the default writers and having it be explicitly configured in solrconfig.xml for our example app.
          David Smiley made changes -
          Assignee David Smiley [ dsmiley ]
          Hide
          David Smiley added a comment -

          The attached patch catches exceptions during attempts to instantiate the default query response writers, and logs a warning.

          I tested that this had the desired effect manually by removing the velocity libs from the example app and going to the /browse UI. Oddly, the default behavior when the "wt" param fails to resolve is to use the default (XML) instead of throwing an error.

          FYI I contemplated a lazy-instantiation strategy but in the end ditched it because it was more complicated than needed.

          Show
          David Smiley added a comment - The attached patch catches exceptions during attempts to instantiate the default query response writers, and logs a warning. I tested that this had the desired effect manually by removing the velocity libs from the example app and going to the /browse UI. Oddly, the default behavior when the "wt" param fails to resolve is to use the default (XML) instead of throwing an error. FYI I contemplated a lazy-instantiation strategy but in the end ditched it because it was more complicated than needed.
          David Smiley made changes -
          Hide
          Hoss Man added a comment -

          -1

          I re-iterate...

          either [velocity is] a core feature and dependency, or it's an optional contrib.

          we should not claim it is a core feature, but then silently succeed even if it's not found at runtime. That will only lead to confusion for users/plugin devs who see that it works by default in (w/o any special <lib/> loading/config) in their solr instance, and then in other instances it's just not there w/o any warning/error on startup.

          if it's a core feature then it's a core feature that people should be able to rely on always working with any solr instance (war or otherwise) and we should fail hard and fast if it's not found.

          if there is going to any ambiguity or about whether it is/isn't available then it must be rolled back to a contrib and people who want it should explicitly say "i want this" using a <lib/> declaration.

          Show
          Hoss Man added a comment - -1 I re-iterate... either [velocity is] a core feature and dependency, or it's an optional contrib. we should not claim it is a core feature, but then silently succeed even if it's not found at runtime. That will only lead to confusion for users/plugin devs who see that it works by default in (w/o any special <lib/> loading/config) in their solr instance, and then in other instances it's just not there w/o any warning/error on startup. if it's a core feature then it's a core feature that people should be able to rely on always working with any solr instance (war or otherwise) and we should fail hard and fast if it's not found. if there is going to any ambiguity or about whether it is/isn't available then it must be rolled back to a contrib and people who want it should explicitly say "i want this" using a <lib/> declaration.
          Hide
          David Smiley added a comment -

          Hoss, you apparently have a black or white view of things – something is needed or not without conditions. I don't advocate removing Velocity from Solr's maven pom or the WAR file that comes with Solr. However I do think that if the Solr user/packager realized that Velocity is not used in their setup (perhaps using Solr in an embedded fashion) and if Solr can gracefully work without it for the rest of Solr that doesn't need it, then it should run without it. There are many parts of Solr that are very loosely tied into the framework (a good thing) like request handlers, query parsers, response writers, text analysis etc. Unless one of these are explicitly registered, I think Solr should not fail to start if a dependency isn't present.

          it's just not there w/o any warning/error on startup

          This is a false claim. My patch logs a warning.

          Show
          David Smiley added a comment - Hoss, you apparently have a black or white view of things – something is needed or not without conditions. I don't advocate removing Velocity from Solr's maven pom or the WAR file that comes with Solr. However I do think that if the Solr user/packager realized that Velocity is not used in their setup (perhaps using Solr in an embedded fashion) and if Solr can gracefully work without it for the rest of Solr that doesn't need it, then it should run without it. There are many parts of Solr that are very loosely tied into the framework (a good thing) like request handlers, query parsers, response writers, text analysis etc. Unless one of these are explicitly registered, I think Solr should not fail to start if a dependency isn't present. it's just not there w/o any warning/error on startup This is a false claim. My patch logs a warning.
          Hide
          Hoss Man added a comment -

          This is a false claim. My patch logs a warning.

          my apologies, i misread the patch and thought it would only warn on first usage. It doesn't change the fundemental issue however...

          Hoss, you apparently have a black or white view of things

          I absolutely consider this issue to be black/white.

          However I do think that if the Solr user/packager realized that Velocity is not used in their setup (perhaps using Solr in an embedded fashion) and if Solr can gracefully work without it for the rest of Solr that doesn't need it, then it should run without it.

          the problem with that philosophy is that it completely breaks the "contract" we make with our users – novices and plugin writers – about what they can/can't expect to be in a basic solr installation.

          if someone repackages solr to not include something that is considered a "core" feature/dependency, then that installation is absolutely, 100% broken, and we should not go out of our way to help that packager/installer mask the broken nature.

          It is broken not only because whatever out of the box feature we advertise as being available no longer works for novice users who may try to use those features, but it is broken because anyone trying to write a plugin can no longer say "all you need to load my plugin jar is this <lib.../> directive, because all of the dependencies are already core solr dependencies"

          if VelocityWriter is a core feature (and as of now it is) then velocity is a core dependency and we should not jump through this hoop to deal with the possibility of velocity dependencies being missing any more then we should jump through hoops to deal with the possibility that commons-io, or commons-fileupload is missing – in either case, the system is not a fully functional solr installation as documented, and we should not hide that from users until they actually try to use a documented feature and get a failure.

          If someone wants to bastardize a solr installation to remove core dependencies we should not be making it easier on them just because it only means changing a few line of code – that just hamstrings us with an expectation that we can never use that dependency in any other ways in the core. Either we rip that dependency out (making it a plugin instead of a core feature) or we let the bastardizers patch the affected files themselves.

          Any middle ground hurts our users by making it impossible to know what features will/won't work in any given install, and hurts development by hindering when/how we can use libraries we've already said are core dpendencies.

          Show
          Hoss Man added a comment - This is a false claim. My patch logs a warning. my apologies, i misread the patch and thought it would only warn on first usage. It doesn't change the fundemental issue however... Hoss, you apparently have a black or white view of things I absolutely consider this issue to be black/white. However I do think that if the Solr user/packager realized that Velocity is not used in their setup (perhaps using Solr in an embedded fashion) and if Solr can gracefully work without it for the rest of Solr that doesn't need it, then it should run without it. the problem with that philosophy is that it completely breaks the "contract" we make with our users – novices and plugin writers – about what they can/can't expect to be in a basic solr installation. if someone repackages solr to not include something that is considered a "core" feature/dependency, then that installation is absolutely, 100% broken, and we should not go out of our way to help that packager/installer mask the broken nature. It is broken not only because whatever out of the box feature we advertise as being available no longer works for novice users who may try to use those features, but it is broken because anyone trying to write a plugin can no longer say "all you need to load my plugin jar is this <lib.../> directive, because all of the dependencies are already core solr dependencies" if VelocityWriter is a core feature (and as of now it is) then velocity is a core dependency and we should not jump through this hoop to deal with the possibility of velocity dependencies being missing any more then we should jump through hoops to deal with the possibility that commons-io, or commons-fileupload is missing – in either case, the system is not a fully functional solr installation as documented, and we should not hide that from users until they actually try to use a documented feature and get a failure. If someone wants to bastardize a solr installation to remove core dependencies we should not be making it easier on them just because it only means changing a few line of code – that just hamstrings us with an expectation that we can never use that dependency in any other ways in the core. Either we rip that dependency out (making it a plugin instead of a core feature) or we let the bastardizers patch the affected files themselves. Any middle ground hurts our users by making it impossible to know what features will/won't work in any given install, and hurts development by hindering when/how we can use libraries we've already said are core dpendencies.
          Hide
          Ryan McKinley added a comment -

          I absolutely consider this issue to be black/white.

          I agree. I posted the hack just to show what people are talking about. Logging an error is a simple quick 'fix', but not a good long term solution.

          I know we pulled velocity into the core package, but maybe we should consider pushing it out to a module? I think this would be consistent with other modularization efforts.

          I think the module should be included in the .war and configured in the example solrconfig.

          Show
          Ryan McKinley added a comment - I absolutely consider this issue to be black/white. I agree. I posted the hack just to show what people are talking about. Logging an error is a simple quick 'fix', but not a good long term solution. I know we pulled velocity into the core package, but maybe we should consider pushing it out to a module? I think this would be consistent with other modularization efforts. I think the module should be included in the .war and configured in the example solrconfig.
          Hide
          David Smiley added a comment -

          Hoss, I don't agree with your black/white views we aren't going to come to an agreement.

          Ryan, you suggest:

          I know we pulled velocity into the core package, but maybe we should consider pushing it out to a module? I think this would be consistent with other modularization efforts.

          I think the module should be included in the .war and configured in the example solrconfig.

          I very much like the idea of making Velocity a module, one that isn't absolutely required, and one that is included by default. I'm surprised it was pulled into core in the first place (no disrespect to Erik).

          Show
          David Smiley added a comment - Hoss, I don't agree with your black/white views we aren't going to come to an agreement. Ryan, you suggest: I know we pulled velocity into the core package, but maybe we should consider pushing it out to a module? I think this would be consistent with other modularization efforts. I think the module should be included in the .war and configured in the example solrconfig. I very much like the idea of making Velocity a module, one that isn't absolutely required, and one that is included by default. I'm surprised it was pulled into core in the first place (no disrespect to Erik).
          Hide
          Hoss Man added a comment -

          I think the module should be included in the .war and configured in the example solrconfig.

          Ryan: Your comments seem contradictory ... was that a typo? did you mean it should not be in the war, but still configured in the example (like clustering, and extraction?)

          I very much like the idea of making Velocity a module, one that isn't absolutely required, and one that is included by default.

          I didn't really have any opinions about SOLR-1957 in the first place, I've got no opinions on reverting it now and pushing the VelocityWriter back into a contrib (on par with dataimporthandler, clustering, extraction, uima, etc...) that is used/demoed in the main example configs using a relative <lib /> directive (just like clustering and extraction)

          But under no circumstances can i go along with a "core" feature be treated like a second class citizen. either it's core and it's a first class citizen; or it's not, and it lives in it's own plugin jar with documented dependencies and clear instructions that it needs to be explicitly loaded via configuration.

          Show
          Hoss Man added a comment - I think the module should be included in the .war and configured in the example solrconfig. Ryan: Your comments seem contradictory ... was that a typo? did you mean it should not be in the war, but still configured in the example (like clustering, and extraction?) I very much like the idea of making Velocity a module, one that isn't absolutely required, and one that is included by default. I didn't really have any opinions about SOLR-1957 in the first place, I've got no opinions on reverting it now and pushing the VelocityWriter back into a contrib (on par with dataimporthandler, clustering, extraction, uima, etc...) that is used/demoed in the main example configs using a relative <lib /> directive (just like clustering and extraction) But under no circumstances can i go along with a "core" feature be treated like a second class citizen. either it's core and it's a first class citizen; or it's not, and it lives in it's own plugin jar with documented dependencies and clear instructions that it needs to be explicitly loaded via configuration.
          Hide
          Ryan McKinley added a comment -

          Ryan: Your comments seem contradictory ... was that a typo? did you mean it should not be in the war, but still configured in the example (like clustering, and extraction?)

          No typo, just a difference of opinion on what is core. For me the .war is not the core, it is the standard/supported way to deploy solr. I don't care if it bundles the .jar in the .war or put in the lib directory (actually forgot about that option). The real issue is if wt=velocity should automatically work if that is not defined in solrconfig.

          I don't feel strongly about this particular issue (velocity), but I do think we should start thinking about the .war dependencies differently then solr-core.jar

          Show
          Ryan McKinley added a comment - Ryan: Your comments seem contradictory ... was that a typo? did you mean it should not be in the war, but still configured in the example (like clustering, and extraction?) No typo, just a difference of opinion on what is core. For me the .war is not the core, it is the standard/supported way to deploy solr. I don't care if it bundles the .jar in the .war or put in the lib directory (actually forgot about that option). The real issue is if wt=velocity should automatically work if that is not defined in solrconfig. I don't feel strongly about this particular issue (velocity), but I do think we should start thinking about the .war dependencies differently then solr-core.jar
          Hide
          David Smiley added a comment -

          I don't feel strongly about this particular issue (velocity), but I do think we should start thinking about the .war dependencies differently then solr-core.jar

          Yes!

          Show
          David Smiley added a comment - I don't feel strongly about this particular issue (velocity), but I do think we should start thinking about the .war dependencies differently then solr-core.jar Yes!
          Hide
          Gunnar Wagenknecht added a comment -

          I like the proposal of making Velocity a module that is included and configured by default in the war file but not required for solr-core.jar to work. That would definitely solve my use-case where I'm embedding Solr in an application and don't want to include Velocity.

          Show
          Gunnar Wagenknecht added a comment - I like the proposal of making Velocity a module that is included and configured by default in the war file but not required for solr-core.jar to work. That would definitely solve my use-case where I'm embedding Solr in an application and don't want to include Velocity.
          Hide
          Hoss Man added a comment -

          No typo, just a difference of opinion on what is core. For me the .war is not the core, it is the standard/supported way to deploy solr.

          ...

          I like the proposal of making Velocity a module that is included and configured by default in the war file

          If we revert SOLR-1957 to make the velocity response writer a contrib/plugin again, but left it include in the solr.war, then it would be in a similar situation to how DIH was in Solr 1.4. (ie: a contrib that core was not allowed to depend on but was include in the war by default). Consensus was (see SOLR-2365) that that situation was confusing and unnecessary (since <lib> includes are so easy) which is why DIH was extracted from the war.

          • If it's a plugin: it shouldn't be included in the default war we ship, if other people want to re-bundle a "kitchen-sink" version of hte war with all stock plugins (or even their own plugins) embedded in it that's fine, but that shouldn't be the default.
          • If it's not a plugin, and it's included in the war by default, and then it's fair game to be a core dependency.

          (Ironicaly SOLR-2365 was actually the chief motivator for why velocity became a core dependency in SOLR-1957: so that plugins like DIH could include *.vm templates in their jar files to add UI functionality instead of needing weird JSP files.)

          The real issue is if wt=velocity should automatically work if that is not defined in solrconfig.

          I'll say it again...

          I didn't really have any opinions about SOLR-1957 in the first place, I've got no opinions on reverting it now and pushing the VelocityWriter back into a contrib (on par with dataimporthandler, clustering, extraction, uima, etc...) that is used/demoed in the main example configs using a relative <lib /> directive (just like clustering and extraction)

          ...easy enough to add a responseWriter registration for "velocity" in the example configs if folks don't think it should be a core feature. Given the current direction of the UI to use HTML+AJAX+RequestHandlers w/o any need for velocity, i would definitely lean more towards reverting SOLR-1957 and making it a plugin then leaving it as a core dep.

          but no fucking half-assed confusing "it's a contrib plugin but the jar is already in the war" or "it's a built in feature but it's not a core dep so other code can't depend on it" middle ground ... we should be trying to make the build/run deps simpler to understand, not more confusing.

          Show
          Hoss Man added a comment - No typo, just a difference of opinion on what is core. For me the .war is not the core, it is the standard/supported way to deploy solr. ... I like the proposal of making Velocity a module that is included and configured by default in the war file If we revert SOLR-1957 to make the velocity response writer a contrib/plugin again, but left it include in the solr.war, then it would be in a similar situation to how DIH was in Solr 1.4. (ie: a contrib that core was not allowed to depend on but was include in the war by default). Consensus was (see SOLR-2365 ) that that situation was confusing and unnecessary (since <lib> includes are so easy) which is why DIH was extracted from the war. If it's a plugin: it shouldn't be included in the default war we ship, if other people want to re-bundle a "kitchen-sink" version of hte war with all stock plugins (or even their own plugins) embedded in it that's fine, but that shouldn't be the default. If it's not a plugin, and it's included in the war by default, and then it's fair game to be a core dependency. (Ironicaly SOLR-2365 was actually the chief motivator for why velocity became a core dependency in SOLR-1957 : so that plugins like DIH could include *.vm templates in their jar files to add UI functionality instead of needing weird JSP files.) The real issue is if wt=velocity should automatically work if that is not defined in solrconfig. I'll say it again... I didn't really have any opinions about SOLR-1957 in the first place, I've got no opinions on reverting it now and pushing the VelocityWriter back into a contrib (on par with dataimporthandler, clustering, extraction, uima, etc...) that is used/demoed in the main example configs using a relative <lib /> directive (just like clustering and extraction) ...easy enough to add a responseWriter registration for "velocity" in the example configs if folks don't think it should be a core feature. Given the current direction of the UI to use HTML+AJAX+RequestHandlers w/o any need for velocity, i would definitely lean more towards reverting SOLR-1957 and making it a plugin then leaving it as a core dep. but no fucking half-assed confusing "it's a contrib plugin but the jar is already in the war" or "it's a built in feature but it's not a core dep so other code can't depend on it" middle ground ... we should be trying to make the build/run deps simpler to understand, not more confusing.
          Hide
          David Smiley added a comment -

          Hoss: swearing isn't called for. My corporate spam filter has been blocking some of your posts due to the language so I had to add exclusions just for you, and for Rob too on other occasions.

          So I vote for reverting SOLR-1957 in light of the fact that the UI isn't going to need velocity, and it's trivial to add velocity support back in if needed.

          Show
          David Smiley added a comment - Hoss: swearing isn't called for. My corporate spam filter has been blocking some of your posts due to the language so I had to add exclusions just for you, and for Rob too on other occasions. So I vote for reverting SOLR-1957 in light of the fact that the UI isn't going to need velocity, and it's trivial to add velocity support back in if needed.
          Hide
          Steve Rowe added a comment -

          Hoss: swearing isn't called for. My corporate spam filter has been blocking some of your posts due to the language so I had to add exclusions just for you, and for Rob too on other occasions.

          Probably me too on other other occasions, I guess. Swearing is sometimes called for, IMNSHO, but YMMV, obviously. (In this particular case, Hoss clearly intended to use the no-no word as an intensifier, and not to stir shit.)

          Good luck getting consensus on banning profanity here.

          Show
          Steve Rowe added a comment - Hoss: swearing isn't called for. My corporate spam filter has been blocking some of your posts due to the language so I had to add exclusions just for you, and for Rob too on other occasions. Probably me too on other other occasions, I guess. Swearing is sometimes called for, IMNSHO, but YMMV, obviously. (In this particular case, Hoss clearly intended to use the no-no word as an intensifier, and not to stir shit.) Good luck getting consensus on banning profanity here.
          Erik Hatcher made changes -
          Assignee David Smiley [ dsmiley ] Erik Hatcher [ ehatcher ]
          Hide
          Erik Hatcher added a comment -

          David - I'm going to tackle this one by simply moving VrW back to a contrib module. Easy enough, and makes sense in light of the unneeded core hard dependency.

          Show
          Erik Hatcher added a comment - David - I'm going to tackle this one by simply moving VrW back to a contrib module. Easy enough, and makes sense in light of the unneeded core hard dependency.
          Hide
          Erik Hatcher added a comment -

          Here's a patch that moves VelocityResponseWriter back to contrib/velocity and adjusts the example configuration to include it explicitly such that /browse still works.

          What other loose ends are there? Maybe something with Maven (POMs and such)?

          Show
          Erik Hatcher added a comment - Here's a patch that moves VelocityResponseWriter back to contrib/velocity and adjusts the example configuration to include it explicitly such that /browse still works. What other loose ends are there? Maybe something with Maven (POMs and such)?
          Erik Hatcher made changes -
          Attachment SOLR-2588.patch [ 12490731 ]
          Hide
          Steve Rowe added a comment -

          Erik, your patch was missing some files that you moved but didn't make changes to.

          I made a new patch with a few changes:

          1. All Solr contrib test-files/ directories are namespaced with the contrib name, so that same-named files shared between core and contrib don't overwrite each other (see SOLR-2659) - I moved contrib/velocity/src/test-files/solr/ under contrib/velocity/src/test-files/velocity/ and added an overridden getSolrHome() to the test class.
          2. Added IntelliJ IDEA configuration for the new velocity contrib.
          3. Generated the patch from the top-level directory instead of from the solr/ directory.

          The test passes for me.

          Here are the svn commands to run before applying the patch:

          svn mv --parents solr/core/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java solr/contrib/velocity/src/test/org/apache/solr/response/VelocityResponseWriterTest.java
          svn mv --parents solr/core/src/java/org/apache/solr/response/VelocityResponseWriter.java solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java
          svn mv --parents solr/core/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java
          svn mv --parents solr/core/src/java/org/apache/solr/response/SolrParamResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrParamResourceLoader.java
          svn mv --parents solr/core/src/test-files/solr/conf/velocity/VM_global_library.vm solr/contrib/velocity/src/test-files/velocity/solr/conf/velocity/VM_global_library.vm
          svn rm solr/core/src/test-files/solr/conf/velocity
          

          Erik, can you take a look and see if I've screwed anything up?

          I'll work on the Maven build configuration later today.

          Show
          Steve Rowe added a comment - Erik, your patch was missing some files that you moved but didn't make changes to. I made a new patch with a few changes: All Solr contrib test-files/ directories are namespaced with the contrib name, so that same-named files shared between core and contrib don't overwrite each other (see SOLR-2659 ) - I moved contrib/velocity/src/test-files/solr/ under contrib/velocity/src/test-files/velocity/ and added an overridden getSolrHome() to the test class. Added IntelliJ IDEA configuration for the new velocity contrib. Generated the patch from the top-level directory instead of from the solr/ directory. The test passes for me. Here are the svn commands to run before applying the patch: svn mv --parents solr/core/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java solr/contrib/velocity/src/test/org/apache/solr/response/VelocityResponseWriterTest.java svn mv --parents solr/core/src/java/org/apache/solr/response/VelocityResponseWriter.java solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java svn mv --parents solr/core/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java svn mv --parents solr/core/src/java/org/apache/solr/response/SolrParamResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrParamResourceLoader.java svn mv --parents solr/core/src/test-files/solr/conf/velocity/VM_global_library.vm solr/contrib/velocity/src/test-files/velocity/solr/conf/velocity/VM_global_library.vm svn rm solr/core/src/test-files/solr/conf/velocity Erik, can you take a look and see if I've screwed anything up? I'll work on the Maven build configuration later today.
          Steve Rowe made changes -
          Attachment SOLR-2588.patch [ 12490802 ]
          Hide
          Erik Hatcher added a comment -

          Here's an updated patch that removes the comment about VrW in example's solrconfig.xml

          Show
          Erik Hatcher added a comment - Here's an updated patch that removes the comment about VrW in example's solrconfig.xml
          Erik Hatcher made changes -
          Attachment SOLR-2588.patch [ 12490838 ]
          Hide
          Erik Hatcher added a comment -

          Ugh, this got more complicated thanks to core tests relying on the example config. Running on a clean (ant clean) trunk, tests that rely on the example solrconfig.xml fail because the Velocity contrib JAR isn't built yet. I guess the other contribs don't fail because their componentry is lazy loaded but response writers aren't.

          For example:

          ant test -Dtestcase=ShowFileRequestHandlerTest
          

          gives:

          junit-sequential:
              [junit] Testsuite: org.apache.solr.handler.admin.ShowFileRequestHandlerTest
              [junit] Tests run: 2, Failures: 0, Errors: 2, Time elapsed: 2.033 sec
              [junit] 
              [junit] ------------- Standard Error -----------------
              [junit] 18.08.2011 15:13:09 org.apache.solr.common.SolrException log
              [junit] SCHWERWIEGEND: org.apache.solr.common.SolrException: Error loading class 'solr.VelocityResponseWriter'
          

          Huh?

          If you run "ant dist" first, it works because the contrib apache-solr-velocity JAR is built then.

          Ultimately response writers probably should be lazy loaded and only come into existence when used the first time. Thoughts?

          Show
          Erik Hatcher added a comment - Ugh, this got more complicated thanks to core tests relying on the example config. Running on a clean (ant clean) trunk, tests that rely on the example solrconfig.xml fail because the Velocity contrib JAR isn't built yet. I guess the other contribs don't fail because their componentry is lazy loaded but response writers aren't. For example: ant test -Dtestcase=ShowFileRequestHandlerTest gives: junit-sequential: [junit] Testsuite: org.apache.solr.handler.admin.ShowFileRequestHandlerTest [junit] Tests run: 2, Failures: 0, Errors: 2, Time elapsed: 2.033 sec [junit] [junit] ------------- Standard Error ----------------- [junit] 18.08.2011 15:13:09 org.apache.solr.common.SolrException log [junit] SCHWERWIEGEND: org.apache.solr.common.SolrException: Error loading class 'solr.VelocityResponseWriter' Huh? If you run "ant dist" first, it works because the contrib apache-solr-velocity JAR is built then. Ultimately response writers probably should be lazy loaded and only come into existence when used the first time. Thoughts?
          Hide
          Steve Rowe added a comment -

          Added eclipse and maven configurations.

          Show
          Steve Rowe added a comment - Added eclipse and maven configurations.
          Steve Rowe made changes -
          Attachment SOLR-2588.patch [ 12491107 ]
          Hide
          Steve Rowe added a comment -

          In the Maven configuration, moved the velocity dependencies from solr-core to solr-velocity.

          Show
          Steve Rowe added a comment - In the Maven configuration, moved the velocity dependencies from solr-core to solr-velocity.
          Steve Rowe made changes -
          Attachment SOLR-2588.patch [ 12491108 ]
          Hide
          Steve Rowe added a comment - - edited

          Erik, I think the velocity jars should be moved from solr/lib/ to solr/contrib/velocity/lib/ - most other Solr contribs have their own lib/ dir. What do you think?

          Show
          Steve Rowe added a comment - - edited Erik, I think the velocity jars should be moved from solr/lib/ to solr/contrib/velocity/lib/ - most other Solr contribs have their own lib/ dir. What do you think?
          Hide
          Hoss Man added a comment -

          Ugh, this got more complicated thanks to core tests relying on the example config. Running on a clean (ant clean) trunk, tests that rely on the example solrconfig.xml fail because the Velocity contrib JAR isn't built yet. I guess the other contribs don't fail because their componentry is lazy loaded but response writers aren't.

          there's two aspects of this to worry about, it's not really clear to me if only one/both apply here...

          1) testing the contrib. if the majority of the tests that exist for the velocity writer are dependent on using the example, then those relaly need to be refactored into the contrib (if that's where the code is going)
          2) testing the example. the was the original point of those tests – to verify that when users try to use the example, it will owrk (many tests have start abusing those configs unneccessarily, but that's the original point) ...

          #2 is a lot harder to find a "good" solution for. on the one hand, adding a lazyload option to response writers gets us out of the dependency whole of testing the example w/o the velocity writer being built; but it side steps the true goal of saying "will the example work". It seems like the three possible solutions are:
          a) ignore the problem
          b) use lazyloading; leave the "core" example tests only testing "core" things; add a velocity contrib test that also tests the example this time explicitly excersizing the velocity writer and testing that that piece of the example configs works
          c) refactor all of the example tests into a new contrib/module/whatever that depends on all of hte contribs.

          either B or C seem like the best long term approach, and would also be a pattern that could be applied to the other contribs used in the example via lazy loading. B feels dirtier, but might actually be the wisest choice since it would help us test more permutations (a user tries the example with only the core solr stuff, a user tries the example adding velocity plugin, etc..)

          Show
          Hoss Man added a comment - Ugh, this got more complicated thanks to core tests relying on the example config. Running on a clean (ant clean) trunk, tests that rely on the example solrconfig.xml fail because the Velocity contrib JAR isn't built yet. I guess the other contribs don't fail because their componentry is lazy loaded but response writers aren't. there's two aspects of this to worry about, it's not really clear to me if only one/both apply here... 1) testing the contrib. if the majority of the tests that exist for the velocity writer are dependent on using the example, then those relaly need to be refactored into the contrib (if that's where the code is going) 2) testing the example. the was the original point of those tests – to verify that when users try to use the example, it will owrk (many tests have start abusing those configs unneccessarily, but that's the original point) ... #2 is a lot harder to find a "good" solution for. on the one hand, adding a lazyload option to response writers gets us out of the dependency whole of testing the example w/o the velocity writer being built; but it side steps the true goal of saying "will the example work". It seems like the three possible solutions are: a) ignore the problem b) use lazyloading; leave the "core" example tests only testing "core" things; add a velocity contrib test that also tests the example this time explicitly excersizing the velocity writer and testing that that piece of the example configs works c) refactor all of the example tests into a new contrib/module/whatever that depends on all of hte contribs. either B or C seem like the best long term approach, and would also be a pattern that could be applied to the other contribs used in the example via lazy loading. B feels dirtier, but might actually be the wisest choice since it would help us test more permutations (a user tries the example with only the core solr stuff, a user tries the example adding velocity plugin, etc..)
          Hoss Man made changes -
          Link This issue is related to SOLR-2718 [ SOLR-2718 ]
          Hide
          Robert Muir added a comment -

          why are B and C mutually exclusive?

          Cant the example/example tests that actually test the example be in a contrib/module/whatever (since its really more like integration tests), yet at the same time core tests only test core things and velocity contrib has a test config that tests its piece.

          Obviously these won't all share the same exact config files but the example is meant to be modified, so its realistic right?

          Show
          Robert Muir added a comment - why are B and C mutually exclusive? Cant the example/example tests that actually test the example be in a contrib/module/whatever (since its really more like integration tests), yet at the same time core tests only test core things and velocity contrib has a test config that tests its piece. Obviously these won't all share the same exact config files but the example is meant to be modified, so its realistic right?
          Hide
          Hoss Man added a comment -

          why are B and C mutually exclusive?

          i didn't mean to suggest that they were, we could certainly do both.

          Obviously these won't all share the same exact config files but the example is meant to be modified, so its realistic right?

          you lost me there ... they can use the exact same configs – that's kind of the point: testing the exact example configs as we ship them (with <lib/> declarations that point at dirs which may or may not contain jars depending on what contribs are built; and request handler / response writer declarations configured that use lazyloading to dynamic load things as needed.

          i mean – yes we could have tests that copy the example configs and modify them and test that those modifications still work, but that's not really the point. the point is "core features A,B,C should work with these example configs as is; and contrib feature X should also work with the same example configs (unmodified) as long as contrib-X is build and in dir-X; and contrib feature Y should also work with the same example configs (unmodified) as long as contrib-Y is build and in dir-Y; etc..."

          Show
          Hoss Man added a comment - why are B and C mutually exclusive? i didn't mean to suggest that they were, we could certainly do both. Obviously these won't all share the same exact config files but the example is meant to be modified, so its realistic right? you lost me there ... they can use the exact same configs – that's kind of the point: testing the exact example configs as we ship them (with <lib/> declarations that point at dirs which may or may not contain jars depending on what contribs are built; and request handler / response writer declarations configured that use lazyloading to dynamic load things as needed. i mean – yes we could have tests that copy the example configs and modify them and test that those modifications still work, but that's not really the point. the point is "core features A,B,C should work with these example configs as is; and contrib feature X should also work with the same example configs (unmodified) as long as contrib-X is build and in dir-X; and contrib feature Y should also work with the same example configs (unmodified) as long as contrib-Y is build and in dir-Y; etc..."
          Hide
          Robert Muir added a comment -

          you lost me there ... they can use the exact same configs – that's kind of the point: testing the exact example configs as we ship them (with <lib/> declarations that point at dirs which may or may not contain jars depending on what contribs are built; and request handler / response writer declarations configured that use lazyloading to dynamic load things as needed.

          Ok: what I'm suggesting here is that core/contrib modules would still test what they do, only I think with "minimal" configs? This way its easier to debug, e.g. conceptually lower-level tests. But these tests still need to be realistic, e.g. include the <lib/> delcarations you refer to?

          i mean – yes we could have tests that copy the example configs and modify them and test that those modifications still work, but that's not really the point. the point is "core features A,B,C should work with these example configs as is; and contrib feature X should also work with the same example configs (unmodified) as long as contrib-X is build and in dir-X; and contrib feature Y should also work with the same example configs (unmodified) as long as contrib-Y is build and in dir-Y; etc..."

          Right: i see, so this 'integration' testing is a separate challenge from what I mentioned above. In this case we want to test the example with different configurations... but if we separate out these 'example' tests into something thats more suitable for integration testing perhaps we can setup just an environment like this? Maybe it would mimic the core/contrib structure in svn we have for the unit tests even... the only difference is the classpaths etc will be different?

          Show
          Robert Muir added a comment - you lost me there ... they can use the exact same configs – that's kind of the point: testing the exact example configs as we ship them (with <lib/> declarations that point at dirs which may or may not contain jars depending on what contribs are built; and request handler / response writer declarations configured that use lazyloading to dynamic load things as needed. Ok: what I'm suggesting here is that core/contrib modules would still test what they do, only I think with "minimal" configs? This way its easier to debug, e.g. conceptually lower-level tests. But these tests still need to be realistic, e.g. include the <lib/> delcarations you refer to? i mean – yes we could have tests that copy the example configs and modify them and test that those modifications still work, but that's not really the point. the point is "core features A,B,C should work with these example configs as is; and contrib feature X should also work with the same example configs (unmodified) as long as contrib-X is build and in dir-X; and contrib feature Y should also work with the same example configs (unmodified) as long as contrib-Y is build and in dir-Y; etc..." Right: i see, so this 'integration' testing is a separate challenge from what I mentioned above. In this case we want to test the example with different configurations... but if we separate out these 'example' tests into something thats more suitable for integration testing perhaps we can setup just an environment like this? Maybe it would mimic the core/contrib structure in svn we have for the unit tests even... the only difference is the classpaths etc will be different?
          Hide
          Robert Muir added a comment -

          just a basic conceptual illustration of what i meant above, please don't read into the naming:

          solr/core/... <-- unit tests for solr core
          solr/contrib/velocity <-- unit tests for solr velocity
          ...
          
          solr/example-test/ <-- integration tests for example with only core configuration
          solr/example-test/velocity <-- integration tests for example with velocity configuration
          ...
          
          Show
          Robert Muir added a comment - just a basic conceptual illustration of what i meant above, please don't read into the naming: solr/core/... <-- unit tests for solr core solr/contrib/velocity <-- unit tests for solr velocity ... solr/example-test/ <-- integration tests for example with only core configuration solr/example-test/velocity <-- integration tests for example with velocity configuration ...
          Hide
          Robert Muir added a comment -

          3.4 -> 3.5

          Show
          Robert Muir added a comment - 3.4 -> 3.5
          Robert Muir made changes -
          Fix Version/s 3.5 [ 12317876 ]
          Fix Version/s 3.4 [ 12316683 ]
          Hide
          Erik Hatcher added a comment -

          I'm going to start making some commits on this issue. I've re-done the work locally on a fresh trunk checkout just to make sure things are clean, and I'm not seeing the test failures I saw before. Maybe some dependencies on the example environment were removed? Or...?

          Anyway, I'll get this rolling in the next day or so and hopefully have this resolved then.

          Show
          Erik Hatcher added a comment - I'm going to start making some commits on this issue. I've re-done the work locally on a fresh trunk checkout just to make sure things are clean, and I'm not seeing the test failures I saw before. Maybe some dependencies on the example environment were removed? Or...? Anyway, I'll get this rolling in the next day or so and hopefully have this resolved then.
          Hide
          Erik Hatcher added a comment -

          New patch that moves VrW to contrib/velocity and fixes the test issues. I was wrong in my previous comment - the test issue is still present. I worked around this using the enable flag for plugins, though this required a few uses of it in the test infrastructure. Not terrible, but not ideal either. Lazy loaded response writers would be better.

          Show
          Erik Hatcher added a comment - New patch that moves VrW to contrib/velocity and fixes the test issues. I was wrong in my previous comment - the test issue is still present. I worked around this using the enable flag for plugins, though this required a few uses of it in the test infrastructure. Not terrible, but not ideal either. Lazy loaded response writers would be better.
          Erik Hatcher made changes -
          Attachment SOLR-2588.patch [ 12500905 ]
          Hide
          Erik Hatcher added a comment -

          Same patch, just checking the ASL box this time!

          Show
          Erik Hatcher added a comment - Same patch, just checking the ASL box this time!
          Erik Hatcher made changes -
          Attachment SOLR-2588.patch [ 12500906 ]
          Hide
          Steve Rowe added a comment -

          Here's a version of Erik's patch that assumes that the following svn move script has been run first:

          svn mv --parents solr/core/src/java/org/apache/solr/response/VelocityResponseWriter.java solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java
          svn mv --parents solr/core/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java
          svn mv --parents solr/core/src/java/org/apache/solr/response/SolrParamResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrParamResourceLoader.java
          svn mv --parents solr/core/src/java/org/apache/solr/response/PageTool.java solr/contrib/velocity/src/java/org/apache/solr/response/PageTool.java
          svn mv --parents solr/core/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java solr/contrib/velocity/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java
          svn mv --parents solr/core/src/test-files/solr/conf/velocity/VM_global_library.vm solr/contrib/velocity/src/test-files/velocity/solr/conf/velocity/VM_global_library.vm
          svn rm solr/core/src/test-files/solr/conf/velocity
          svn mv --parents solr/lib/velocity-* solr/contrib/velocity/lib/
          
          Show
          Steve Rowe added a comment - Here's a version of Erik's patch that assumes that the following svn move script has been run first: svn mv --parents solr/core/src/java/org/apache/solr/response/VelocityResponseWriter.java solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java svn mv --parents solr/core/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrVelocityResourceLoader.java svn mv --parents solr/core/src/java/org/apache/solr/response/SolrParamResourceLoader.java solr/contrib/velocity/src/java/org/apache/solr/response/SolrParamResourceLoader.java svn mv --parents solr/core/src/java/org/apache/solr/response/PageTool.java solr/contrib/velocity/src/java/org/apache/solr/response/PageTool.java svn mv --parents solr/core/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java solr/contrib/velocity/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java svn mv --parents solr/core/src/test-files/solr/conf/velocity/VM_global_library.vm solr/contrib/velocity/src/test-files/velocity/solr/conf/velocity/VM_global_library.vm svn rm solr/core/src/test-files/solr/conf/velocity svn mv --parents solr/lib/velocity-* solr/contrib/velocity/lib/
          Steve Rowe made changes -
          Attachment SOLR-2588.patch [ 12500919 ]
          Hide
          Steve Rowe added a comment -

          This version of the patch includes files that I forgot to 'svn add'.

          Also, I've added Eclipse configuration.

          All tests pass.

          Show
          Steve Rowe added a comment - This version of the patch includes files that I forgot to 'svn add'. Also, I've added Eclipse configuration. All tests pass.
          Steve Rowe made changes -
          Attachment SOLR-2588.patch [ 12500926 ]
          Hide
          Steve Rowe added a comment -

          I will try the maven build and the IntelliJ IDEA build/test a little later today.

          Show
          Steve Rowe added a comment - I will try the maven build and the IntelliJ IDEA build/test a little later today.
          Erik Hatcher committed 1189383 (59 files)
          Reviews: none

          SOLR-2588: Move VelocityResponseWriter back to contrib/velocity

          Lucene trunk
          Hide
          Erik Hatcher added a comment -

          I've committed the move on trunk to contrib/velocity.

          I see this issue marked for 3.5 also, though is this really needed? I personally am fine with this change being solely for 4.0.

          Show
          Erik Hatcher added a comment - I've committed the move on trunk to contrib/velocity. I see this issue marked for 3.5 also, though is this really needed? I personally am fine with this change being solely for 4.0.
          Steve Rowe made changes -
          Assignee Erik Hatcher [ ehatcher ] Steven Rowe [ steve_rowe ]
          Hide
          Steve Rowe added a comment -

          I'll backport to branch_3x.

          Show
          Steve Rowe added a comment - I'll backport to branch_3x.
          Hide
          Steve Rowe added a comment -

          Trunk IntelliJ config works for me (rebuilding the project & running Velocity contrib tests).

          Show
          Steve Rowe added a comment - Trunk IntelliJ config works for me (rebuilding the project & running Velocity contrib tests).
          Hide
          Steve Rowe added a comment -

          This patch contains the Maven configuration.

          Committing shortly.

          Show
          Steve Rowe added a comment - This patch contains the Maven configuration. Committing shortly.
          Steve Rowe made changes -
          Attachment SOLR-2588-maven.patch [ 12500946 ]
          sarowe committed 1189455 (1 file)
          Reviews: none

          SOLR-2588: Added solr/contrib/velocity/src/java/overview.html to allow 'ant generate-maven-artifacts' to succeed

          Hide
          Steve Rowe added a comment -

          Erik, I was looking at the solrconfig.xml under test-files/, and I noticed that you have <luceneMatchVersion>LUCENE_40</luceneMatchVersion>, where in other contribs' test configurations, it's instead <luceneMatchVersion>${tests.luceneMatchVersion:LUCENE_CURRENT}</luceneMatchVersion>.

          Is there any reason to hard code LUCENE_40 there (or LUCENE_34 on branch_3x, I guess)?

          Show
          Steve Rowe added a comment - Erik, I was looking at the solrconfig.xml under test-files/ , and I noticed that you have <luceneMatchVersion>LUCENE_40</luceneMatchVersion> , where in other contribs' test configurations, it's instead <luceneMatchVersion>${tests.luceneMatchVersion:LUCENE_CURRENT}</luceneMatchVersion> . Is there any reason to hard code LUCENE_40 there (or LUCENE_34 on branch_3x, I guess)?
          Erik Hatcher made changes -
          Assignee Steven Rowe [ steve_rowe ] Erik Hatcher [ ehatcher ]
          Hide
          Erik Hatcher added a comment -

          Oops fat fingered from mobile.

          Show
          Erik Hatcher added a comment - Oops fat fingered from mobile.
          Erik Hatcher made changes -
          Assignee Erik Hatcher [ ehatcher ] Steven Rowe [ steve_rowe ]
          Hide
          Erik Hatcher added a comment -

          No reason to use any particular value. Just copied another test config.

          Show
          Erik Hatcher added a comment - No reason to use any particular value. Just copied another test config.
          Hide
          Steve Rowe added a comment -

          Okay, I'll change it.

          Show
          Steve Rowe added a comment - Okay, I'll change it.
          sarowe committed 1189487 (1 file)
          Reviews: none

          SOLR-2588: Switch away from hard-coded luceneMatchVersion in solrconfig.xml under test-files/.

          Hide
          Steve Rowe added a comment -

          Backported to branch_3x.

          Show
          Steve Rowe added a comment - Backported to branch_3x.
          Steve Rowe made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          sarowe committed 1189490 (63 files)
          Reviews: none

          SOLR-2588: backported velocity contrib to branch_3x

          Lucene branch_3x
          sarowe committed 1189495 (1 file)
          Reviews: none

          SOLR-2588: Backported CHANGES.txt changes from trunk

          sarowe committed 1189507 (1 file)
          Reviews: none

          SOLR-2588: Eclipse configuration: Remove obsolete solr/lib/ entries for velocity jars.

          sarowe committed 1189508 (1 file)
          Reviews: none

          SOLR-2588: Eclipse configuration: Remove obsolete solr/lib/ entries for velocity jars.

          Hide
          Uwe Schindler added a comment -

          Bulk close after 3.5 is released

          Show
          Uwe Schindler added a comment - Bulk close after 3.5 is released
          Uwe Schindler made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Steve Rowe
              Reporter:
              Gunnar Wagenknecht
            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development