[GitHub] [geode] mcmellawatt opened pull request #3929: GEODE-7089: Each client registration thread uses its own queue

["mcmellawatt (GitHub)" <gi...@apache.org>]

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [N/A] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/3929 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] nabarunnag commented on pull request #3929: GEODE-7089: Each client registration thread uses its own queue

["nabarunnag (GitHub)" <gi...@apache.org>]

Hi Ryan, just wanted to know that the test mentions that clientRegistrationFails**QueueStillDrains**. Is there an assert needed to make sure that the queue actually drained? the last assert is that an IOException is thrown

[ Full content available at: https://github.com/apache/geode/pull/3929 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #3929: GEODE-7089: Each client registration thread uses its own queue

["mcmellawatt (GitHub)" <gi...@apache.org>]

You are right that this test is not really complete.  I think initially I was punting on that validation because the registration queue manager is not publicly accessible.  I am going to make a commit so we can inject and verify these calls on the queue manager.

[ Full content available at: https://github.com/apache/geode/pull/3929 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #3929: GEODE-7089: Each client registration thread uses its own queue

["mcmellawatt (GitHub)" <gi...@apache.org>]

So we still need to do the locking so that no new events are added while we are processing and removing the queue.  We need to decrement the put in progress counter on every event in the queue, and also need to ensure no new events are being added while we do that (otherwise we would remove the queue from the manager while it may still have events in it, thereby resulting in events leaking without the counter being decremented).  If you look at the drain logic, the only thing it does is decrement the putinprogress counter for each event if the proxy is null, so we aren't really doing any unnecessary work.  But we do need to do that for every event in the queue under synchronization.

[ Full content available at: https://github.com/apache/geode/pull/3929 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt closed pull request #3929: GEODE-7089: Each client registration thread uses its own queue

["mcmellawatt (GitHub)" <gi...@apache.org>]

[ pull request closed by mcmellawatt ]

[ Full content available at: https://github.com/apache/geode/pull/3929 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] nabarunnag commented on pull request #3929: GEODE-7089: Each client registration thread uses its own queue

["nabarunnag (GitHub)" <gi...@apache.org>]

ryan, do you feel that a null check here will avoid couple of calls and locks ? I mean if cache client proxy is null we just decrement the put in progress counter and return from here.


[ Full content available at: https://github.com/apache/geode/pull/3929 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #3929: GEODE-7089: Each client registration thread uses its own queue

["mcmellawatt (GitHub)" <gi...@apache.org>]

I think maybe the confusion here is that the put in progress counter is incremented _per event received_ during registration, not just once for the whole registration.

[ Full content available at: https://github.com/apache/geode/pull/3929 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org