You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2023/01/25 06:17:53 UTC
[GitHub] [solr] dsmiley opened a new pull request, #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
dsmiley opened a new pull request, #1313:
URL: https://github.com/apache/solr/pull/1313
https://issues.apache.org/jira/browse/SOLR-16629
TODO double-check compileOnly in SolrJ means won't result in transitive dependency to 3rd parties. Or maybe we don't care?
Trying to see if Lift/Infer/RacerD is helped by these annotations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1407268031
> I don't see where we specify a version to use for this plugin.
Some (all?) of the plugin versions are here: https://github.com/apache/solr/blob/main/build.gradle#L21
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1406517603
So I'm not sure off hand right now - the only thing that jumps out from versions.lock is this:
```
net.jcip:jcip-annotations:1.0 (6 constraints: 6130731b)
```
There is no `com.github.stephenc.jcip:jcip-annotations` in https://github.com/apache/solr/blob/main/versions.props - yet this PR adds an explicit `com.github.stephenc.jcip:jcip-annotations` dependency so I think there should be a versions.props entry?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1406079664
Some of the edits to build.gradle was a bit of re-organization; all I _really_ did of substance is add one compileOnly to two files. Maybe I should do such re-org separately; I would do even more organization. I was trying to loosely order the dependencies.
@risdenk I could use some assistance:
```
* What went wrong:
[24](https://github.com/apache/solr/actions/runs/4003404424/jobs/6871480844#step:7:25)
Could not determine the dependencies of task ':solr:core:analyzeClassesDependencies'.
[25](https://github.com/apache/solr/actions/runs/4003404424/jobs/6871480844#step:7:26)
> Could not resolve all task dependencies for configuration ':solr:core:compileOnlyHelper'.
[26](https://github.com/apache/solr/actions/runs/4003404424/jobs/6871480844#step:7:27)
> Could not find com.github.stephenc.jcip:jcip-annotations:.
[27](https://github.com/apache/solr/actions/runs/4003404424/jobs/6871480844#step:7:28)
Required by:
[28](https://github.com/apache/solr/actions/runs/4003404424/jobs/6871480844#step:7:29)
project :solr:core
```
I have some dissatisfaction with this dependency enforcement tool that you added. I'm onboard with the best practice of declaring one's dependencies that are directly called but (a) the tool isn't smart enough -- we need to add permitUnusedDeclared and similar things, and (b) it doesn't understand the gradle concept of "api" in that it should be though of as inheriting a direct dependency rather than transitive. So it's rather annoying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1406527276
> (a) the tool isn't smart enough -- we need to add permitUnusedDeclared and similar things
Do you have examples of this? I think `spotbugs` is one that might get fixed in later versions and hasn't been cleaned up. The other case being things that are used by reflection that a build tool can't understand. I don't see any egregious usages here. I know during reviews I've pushed back on adding permit* cases since they usually aren't needed. Those dependencies should be runtimeOnly for permitUnusedDeclared or other replacements without permit*.
> (b) it doesn't understand the gradle concept of "api" in that it should be though of as inheriting a direct dependency rather than transitive.
I'm not sure I understand this case here. Related to this PR, I'm guessing you mean adding `com.github.stephenc.jcip:jcip-annotations` explicitly to build.gradle to both core and solrj. The idea here is to ensure that each Gradle module declares the dependencies that are used directly. This makes it really easy to see what dependencies are used by that module. This prevents breakages if the transitive dependency were to change out from underneath the module.
ie:
* `moduleA` has an api dependency on `moduleB`
* `moduleB` includes api dependency `thingA`
* `moduleA` uses classes from dependency `thingA`
This works as long as `moduleB` includes dependency `thingA`. If someone comes along and says "o look only `moduleB` uses dependency `thingA` lets replace it with `thingB`", then that would break `moduleA`.
Th dependency analyze plugin forces `moduleA` to depend directly on `thingA` so that `moduleB` changes don't break `moduleA`.
Yes it looks redundant on the surface, but this only happens for dependencies directly used by a module to prevent this type of breakage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1407136130
> Thus I think it's needlessly verbose for solr:hadoop-auth's build (or any other things that depend on solr-core) to specify any of solr-core's "api" dependencies. These dependencies are on the surface, not buried below (like Guava say).
So as far as I'm aware - for the dependency to need to be declared - classes must be used explicitly in the module in question. I would not be surprised if hadoop-auth is using classes directly from solrj-zookeeper so it needs to be defined in hadoop-auth. This protects against solr:core from removing the api dependency on solrj-zookeeper and breaking hadoop-auth module. It sounds obvious to a human that we would never remove solrj-zookeeper as an api dependency from core, but the computer doesn't know that. so we need to be explicit there on the hadoop-auth module.
I think the more interesting part is the ability to remove dependencies when they are no longer referenced - the negative to your example. Say guava or commons-io was removed from module classes (ie: replaced with JDK classes) in theory the dependency analysis would say "hey you have too many dependencies declared and they can now be removed" - this would also go for the case if hadoop-auth removed whatever was referring to solrj-zookeeper.
I understand the example you are giving, but we don't have to declare EVERY dependency form solr:core in hadoop-auth - only the ones that hadoop-auth is actually using in its classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1407120367
Thanks for your help Kevin! User error on my part -- doh! Maybe if that dependency analysis could dependOn "check". I took a quick stab at this; sorta worked, sorta didn't. Then I noticed it's likely the latest version of this plugin added this. I don't see where we specify a version to use for this plugin.
RE "api": Here's a practical example. The solr:hadoop-auth module depends on solr:core (obviously). Solr:core has an "api" dependency on solr:solrj-zookeeper. The idea is that we have a dependency that is significant / prominent; it's not an internal implementation detail. Its types show up in public methods/classes of solr-core in places. Thus I think it's needlessly verbose for solr:hadoop-auth's build (or any other things that depend on solr-core) to specify any of solr-core's "api" dependencies. These dependencies are on the surface, not buried below (like Guava say). Today it has to because of this build dependency analysis plugin. This plugin was ported from the Maven world where this "api" vs "implementation" distinction didn't exist. I'm dubious about how much pain/suffering is prevented from this plugin when dependencies change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1406540656
> So I'm not sure off hand right now - the only thing that jumps out from versions.lock is this:
>
> ```
> net.jcip:jcip-annotations:1.0 (6 constraints: 6130731b)
> ```
>
> This is a different jcip-annotations dependency :/
>
> Additionally, there is no `com.github.stephenc.jcip:jcip-annotations` in https://github.com/apache/solr/blob/main/versions.props - yet this PR adds an explicit `com.github.stephenc.jcip:jcip-annotations` dependency so I think there should be a versions.props entry?
@dsmiley checked on your branch and adding `com.github.stephenc.jcip:jcip-annotations=1.0-1` to versions.props fixed the issue. I thought there was a better error message for this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1313:
URL: https://github.com/apache/solr/pull/1313#issuecomment-1407285268
> It sounds obvious to a human that we would never remove solrj-zookeeper as an api dependency from core, but the computer doesn't know that. so we need to be explicit there on the hadoop-auth module.
"need"? Or else what bad thing could happen? It's only "need"ed because this plugin we elected to use, for both better and here worse, doesn't recognize "api" dependencies as being different from "implementation". IMO of course.
> but we don't have to declare EVERY dependency form solr:core in hadoop-auth - only the ones that hadoop-auth is actually using in its classes.
It may not be many lines but I argue these particular lines, ones for transitive "api" dependencies, have zero value. The best example I can give is solr-core depending on solrJ. Basically every module depends on solr-core, and thus thanks to this build plugin further specifies SolrJ explicitly. Is there value (relevance or consequence) in this tedious house-keeping?
Any way, I suppose the plugin offers more value than annoyance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #1313: SOLR-16629 Replace SolrSingleThreaded and SolrThreadSafe annotations with JCIP
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #1313:
URL: https://github.com/apache/solr/pull/1313
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org