Codebase Janitorial Services: the SolrSingleThreaded PR

[Marcus Eagan <ma...@gmail.com>]

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 <an...@anshumgupta.net> 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

Re: Codebase Janitorial Services: the SolrSingleThreaded PR

[Marcus Eagan <ma...@gmail.com>]

That is a great thought and I agree with it.

On Wed, Aug 12, 2020 at 5:55 AM Mike Drob <md...@apache.org> wrote:

> I don’t have time to write a more comprehensive email with links and
> references but the basic outline here is that Mark did some (read: a ton)
> work on a private branch that he later shared with his coworkers. Pieces of
> that branch filtered out to Apache.
>
> The intent behind this annotations was to start labeling things so that we
> can reason about our concurrency better, since right now we likely have
> bugs relating to misuse of concurrent classes and shared state. Solr is a
> complex beast that is very hard to understand, so the thought was that we
> could start chipping away at it.
>
>
> Mike
>
> On Wed, Aug 12, 2020 at 1:18 AM Marcus Eagan <ma...@gmail.com>
> wrote:
>
>> 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 <an...@anshumgupta.net> 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
>>
>>

-- 
Marcus Eagan

Re: Codebase Janitorial Services: the SolrSingleThreaded PR

[Mike Drob <md...@apache.org>]

I don’t have time to write a more comprehensive email with links and
references but the basic outline here is that Mark did some (read: a ton)
work on a private branch that he later shared with his coworkers. Pieces of
that branch filtered out to Apache.

The intent behind this annotations was to start labeling things so that we
can reason about our concurrency better, since right now we likely have
bugs relating to misuse of concurrent classes and shared state. Solr is a
complex beast that is very hard to understand, so the thought was that we
could start chipping away at it.


Mike

On Wed, Aug 12, 2020 at 1:18 AM Marcus Eagan <ma...@gmail.com> wrote:

> 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 <an...@anshumgupta.net> 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
>
>

Re: Codebase Janitorial Services: the SolrSingleThreaded PR

[Marcus Eagan <ma...@gmail.com>]

to be clear because I don't want to come off as antagonizing... the author
of the PR could be anyone. I think if we forget who and focus on what we
can all row together.

I'm writing the group as a whole because I do want to contest this sort of
way of doing things in the absence of good reason. I've modified my initial
email below to be a bit more focused.

Thanks all,

Marcus

On Tue, Aug 11, 2020 at 11:17 PM Marcus Eagan <ma...@gmail.com> wrote:

> 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, some writes that they 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
> the experts,  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. 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. Does the author u 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 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
>
>

-- 
Marcus Eagan