[Service Bus] Remove ClientEntityContext

[ramya-rao-a]

(This is a post Preview 4 task)

The ClientEntityContext In Track 1

• was the context for given Queue/Topic/Subscription client.
• tracked 1 sender, 1 batching receiver, 1 streaming receiver and a list of session receivers as this was the limitation in Track 1. For example, you could have only 1 sender on a QueueClient
• shared the $management link between the ClientEntityContext for clients pointing to the same entity

In Track 2, we removed the intermediate Queue/Topic/Subscription client.
We now can create as many senders and receivers possible.
To get a head start on the implementation, each sender and receiver was made to have its own ClientEntityContext, but this is unnecessary

This PR removes the need to have a ClientEntityContext altogether and moves all the tracking of senders, receivers, messagesSessions, managementClients to the ConnectionContext.

[ramya-rao-a]

/azp run js - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ramya-rao-a]

/azp run js - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[chradek]

Making sure I understand this...
So management clients can be shared across receivers and senders if they match entity paths, right?
Can they be replaced by another sender/receiver? I'm curious why we're storing direct references to the batching/streaming receivers but not the management client.

[ramya-rao-a]

So management clients can be shared across receivers and senders if they match entity paths, right?

Yes.

Can they be replaced by another sender/receiver?

I don't follow...

I'm curious why we're storing direct references to the batching/streaming receivers but not the management client

Each Sender is backed by a MessageSender
Each Receiver is backed by a BatchingReceiver and/or StreamingReceiver
But a Sender/Receiver can share a ManagementClient as long as they are all talking to the same entity.

Receivers need to maintain more state between calls than the others.
This is due to credit management and the way message settlement works.

$management link has no state to maintain.
It mimics the http's request/response pattern.

A Sender link does not need to maintain state between calls either.
So, one can say that we should be able to share senders.
But, am guessing perf might be a reason why one would need to have a greater control of the underlying AMQP sender link and so having dedicated sender makes sense

[chradek]

Sorry I was unclear. It looks like the receiver and sender will create a management client unless one with the same entityPath already exists. It isn't possible for the management in the managementClients object to get replaced is it? I'm starting to recall that we have some logic in place that ensures we don't remove the management client unless no one is using it.

Anyway, you answered my question about why we are getting the management client from the context. I was wondering if it would be safer to also hold onto a reference to the management client so that if for some reason it was removed from the managementClients object, the receiver/sender could still make use of it. But I wasn't sure if there were other ways that the management client could be replaced that would cause a direct reference to be pointing to a stale client. Anyway, this is starting to sound out of scope since the behaviour you have now matches what we had before.

[ramya-rao-a]

It isn't possible for the management in the managementClients object to get replaced is it? I'm starting to recall that we have some logic in place that ensures we don't remove the management client unless no one is using it. I'm starting to recall that we have some logic in place that ensures we don't remove the management client unless no one is using it.

With the changes in this PR, once an entry makes it to context.managementClients it never really gets removed.
The entry in it can get closed when the connection disconnects, or due to any other error but it is still usable. The next call to a method in it will take care of "opening" the underlying RequestResponseLink.

In Track 1, we did have the code to remove the entry/reference if there are no more clients to the corresponding queue/topic/subscription.
When refactoring to the new client hierarchy in Track 2, each sender/receiver got to have its own management client, so there was no sharing.

Now, it is time to take the decision on what do we want to do about sharing the management client and when to actually clean it up from the context cache.

We can either say, we never clean the cache up.
Or, we should start keeping track of the higher level senders and receivers so that we can clear the entry in the context cache when there is no more sender/receiver for the entity

[ramya-rao-a]

@HarshaNalluru Can you check for precedence in .Net as to whether the $management link is shared between senders and receivers for the same entity? Check both Track 1 & Track 2

[HarshaNalluru]

Josh said that the $management link is not shared, each one will have its own management link. Same in track 1.

[HarshaNalluru]

About sharing the management client and cleaning it up when there is no sender/receiver for the entity...

n number of senders and n number of receivers of the same entity would rely on the exact same $management link, is there a perf implication to it?
If we are allowing multiple senders and receivers for the same entity, why should managementClient be any different and be shared?

[ramya-rao-a]

Keeping the current design for now to match with Track 1 behavior.
Using #9944 to track the suggested changes here and make each receiver/sender have its own $management link

[ramya-rao-a]

/azp run js - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ramya-rao-a]

/azp run js - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ramya-rao-a]

/azp run js - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[richardpark-msft]

A few things.

There's a few in here that have to do with removing status checks (.isOpen()) that should probably go back as well as making sure we clean up the maps for connectionContext when we close out or remove receivers (not an issue before with ClientEntityContext but is an issue now).

Otherwise, feel free to just file issues for future PRs for the rest. It's a great change, thanks for doing it :)

[richardpark-msft]

Explanation makes sense - it sounds like this should be in options then, right? Ie, it sounds truly optional.

Also, your explanation sounds great - we should toss that into a comment in the source.

[ramya-rao-a]

wait, which explanation?

[richardpark-msft]

You and @chradek had a little comment thread on this about what the parameter represents, and how it's optional, etc...

Just seemed like useful information for someone coming along and wondering what the parameter actually does beyond just mechanics.