lucene-solr icon indicating copy to clipboard operation
lucene-solr copied to clipboard

SOLR-14731: Add SingleThreaded Annotation to Class

Open MarcusSorealheis opened this issue 5 years ago • 9 comments

Description

Makes use of @anshumg and @markrmiller's single threaded annotation so that it is more obvious to maintainers.

Solution

adds it.

Tests

Tests do not change for this one.

Checklist

Please review the following and check all that apply:

  • [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • [x] I have created a Jira issue and added the issue ID to my pull request title.
  • [x] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • [x] I have developed this patch against the master branch.
  • [ ] I have run ant precommit and the appropriate test suite.
  • [x] I have added tests for my changes.
  • [x] I have added documentation for the Ref Guide (for Solr changes only).

MarcusSorealheis avatar Aug 12 '20 08:08 MarcusSorealheis

simple PR here, will the package masters @chatman and @noblepaul please take a look, as well as @anshumg. I'll use this annotation going forward as applicable. Thanks for adding it! ☺️

MarcusSorealheis avatar Aug 12 '20 08:08 MarcusSorealheis

The annotation usage is correct, however I haven't taken a look at the code itself to validate if it's meant to be single threaded or not. Perhaps other folks who've been in here can chime in on that.

anshumg avatar Aug 13 '20 00:08 anshumg

I don't really know what purpose is solved by this annotation. If it is for documentation purpose, this may make sense. But, there are a million other places where the classes are not thread-safe. What's the point of just adding it here

noblepaul avatar Aug 13 '20 08:08 noblepaul

Rome was not built in one day, so too our code documentation Is in not finished in one PR

madrob avatar Aug 13 '20 12:08 madrob

@anshumg

The annotation usage is correct, however I haven't taken a look at the code itself to validate if it's meant to be single threaded or not. Perhaps other folks who've been in here can chime in on that.

@anshumg are you unable to look at it? It's one file.

MarcusSorealheis avatar Aug 14 '20 17:08 MarcusSorealheis

I'm not sure what the end result is of adding all these annotations in the short term, but if it is very challenging to get them reviewed for something as simple as this one, do you think I should invest the time? Also, maybe @noblepaul but feels like he is doing a lot in Solr atm..

MarcusSorealheis avatar Aug 14 '20 17:08 MarcusSorealheis

Looks good. I'll merge this soon. Thanks @MarcusSorealheis.

chatman avatar Aug 18 '20 10:08 chatman

@chatman Can you re-run this test and commit this one as well?

MarcusSorealheis avatar Aug 19 '20 21:08 MarcusSorealheis

are we able to first the tests to run again?

MarcusSorealheis avatar Aug 24 '20 19:08 MarcusSorealheis