Hi Community, As I have taken it upon myself to slowly document opportunities for improving the overall quality and maintainability of the code base, I was looking into my notes on test failures.
As you all know many test failures in distributed systems repos sometimes can be traced back to race conditions or trying to simulate thread safety with waits. So I went digging into any work in this area. I stumbled across a peculiar contribution: SOLR-13998: Add thread safety annotations to classes (#1053) <https://github.com/apache/lucene-solr/pull/1057>. In the title of the PR, @Anshum Gupta <[email protected]> writes that he adds thread safety annotation to classes. This is misleading and confusing to me for multiple reasons: 1. I was thrown off a little bit by the title. While I tend to defer to Anshum the expert, and I think it might be helpful to have thread safety annotations for the compiler in many cases, there were literally no annotations added to any classes in this PR. H the changes notes from the PR say: > SOLR-13998: Add thread safety annotations to Solr. This only introduces > the annotations and doesn't add these to existing classes. (Anshum Gupta, > Mark Miller) > existing classes. Since this is true, I think the title should change. What was Mark's involvement? Is there a commit/comment missing? 2. What is the value in adding this SingleThreaded annotation 8 months ago, yet not implementing it once since? In fact, it has only been used once, in one file by Dat, which is great. The only place I found it was here: https://github.com/apache/lucene-solr/pull/1470 from last month's shard handler improvements from Dat. I'm mostly concerned about how the project operates with regard to this question. 3. As a general engagement model that could help the project, I think that if someone is going to seemingly add something to Solr, they should implement it in a few places, or point the rest of the community at places where they think it could help the project. Getting in the habit of adding a class, and then not using it anywhere should be something that the committers and contributors on the project reconsider. Otherwise, the codebase will be sprawling. It is sprawling in some areas. More on that later. There are other much older manifestations of this behavior, so I am asking the community, in particular the contributor of this recently added but scantly implemented class, to take a more active role in spearheading the addition of the SolrSingleThreaded annotations where/if they can add value. 4. Ideas I have around this: a) maybe you could share why you added this code in the first place. b) where and why you or the community see we should be annotating more in this manner. c) re-examine your own contribution and ask yourself "should I have contributed this code without more discussion about how it would be implemented in the project?" I stumbled upon this looking for what the PR says it is, but in reality there's not much there. I understand that, and I really like micro-PRs. They're easier to review. But I think we should take this opportunity to build on that work, or get rid of it and other PRs like it. 5. Anshum, did you have some other objectives or aims back in December when you added the SolrSingleThreaded annotation? Does anyone else have any objectives around supporting this effort? Can I and others be of assistance in helping you get to where you intended to go with the PR? I really want to be helpful. 6. Most of all, maybe Anshum or someone here can actually explain to me what SolrSingleThreaded actually does that is needed? I have thoughts and have used annotations like this before with Spring, but in my very limited knowledge about what this PR was intended to accomplish, this implementation does nothing. Please advise, Marcus -- Marcus Eagan
