SOLR-15904: Move SQL support into new sql module #637
Conversation
risdenk
force-pushed
the
SOLR-15904
branch
2 times, most recently
from
2a144b3
to
a5915b9
Compare
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
Outdated
Show resolved
Hide resolved
risdenk
force-pushed
the
SOLR-15904
branch
5 times, most recently
from
c77d8c7
to
a2d0009
Compare
|
Is there a page for this SQL functionality that we can add the module information to? |
|
@HoustonPutman I think you mean https://nightlies.apache.org/solr/draft-guides/solr-reference-guide-antora/solr/10_0/query-guide/sql-query.html potentially? I added a note to enable the plugin to use. |
Sadly I haven't made any progress on this yet. I tracked down how these ImplicitPlugins are referenced and created. It just doesn't look like there is a nice way to ignore the failures. |
|
@risdenk would you mind not force-pushing your changes? Makes it hard to see what has changed since I last viewed. It's easy to squash and merge in the end anyways 🙂 |
|
If you use the PR in github and viewed files it shows exactly what changed. But yea I can try to avoid force pushing. Its clearer to me to make sure the commit is exactly what should be merged. |
|
Updated based on @dsmiley comment in jira. Use startup=lazy and then fix dependencies for tests. |
| @@ -62,6 +62,7 @@ dependencies { | |||
| testImplementation project(':solr:test-framework') | |||
| testImplementation project(':solr:core') | |||
| testImplementation project(':solr:solrj') | |||
| testRuntimeOnly project(':solr:modules:sql') | |||
There was a problem hiding this comment.
Whenever the solrj-jdbc solrj-module is split out, this will go in there, correct?
So the dependency tree at that point will look like solr:solrj:jdbc (test) -> solr:modules:sql -> solr:solrj
There was a problem hiding this comment.
@dsmiley had a comment about this on the jira as well regarding moving the solrj tests -> sql module. JdbcTest and one of the streamingexpressions jdbc test are what is failing without the testRuntimeOnly project(':solr:modules:sql'). I'm not sure it makes sense to move the solrj jdbc tests to solr:modules:sql since they are really solrj tests. The solrj tests are testing that solrj actually works w/ the SQL module. So I kinda like how the dependencies are defined right now.
Regarding solrj-jdbc module - I would think so unless streaming expressions is somehow still in solrj-core or whatever it will be called.
There was a problem hiding this comment.
yeah, definitely don't think they should go in the module, definitely leave them where they are for now. I think this is good to go, and the somewhat-recursive looking dependency (though it's test vs src, so not really recursive) will eventually go away.
There was a problem hiding this comment.
I disagree but I'm not going to hold this up. If we're adding a dependency between our modules that can be avoided simply by moving a test -- that's a "smell" to me, and the test should move. I think it's natural that a module might have "solrj level tests" to exercise the backend functional by using higher level APIs... that shouldn't mean (IMO) that SolrJ tests need to depend on nearly everything. At least that's the direction this will go if we follow your logic.
There was a problem hiding this comment.
Put differently, if we can make simple moves to reduce intermodule dependencies, even just tests, that's a good thing.
There was a problem hiding this comment.
I understand your point David, and I think if this was a long term thing I would definitely agree. However I don't particularly mind which solution we go with in this case, since it's really a temporary band-aid until we have the solrj sub-modules, which shouldn't be too far off
risdenk
changed the title
https://issues.apache.org/jira/browse/SOLR-15904