milo
milo copied to clipboard
Add support for client redundancy via subscription transfer (#523)
Signed-off-by: Marcus Orchard [email protected]
Can one of the admins verify this patch?
Hey Marcus,
It looks like you have an ECA on file for [email protected] but you've authored this commit with your Emerson email address.
Oh crud. Ok, let me try to reauthor that.
Alright, looks like everything is happy now.
rebuild
Thanks Marcus, this looks really good on the surface. I'm going to check it out and take a closer look sometime today. I might try to build a transfer example using it to get a feel for the API.
Ok, just so you have my approach for reference (leaving out obvious null and empty checks):
- Verify diagnostics are enabled by querying the
EnabledFlag
on theServerDiagnostics
node. - Call
getSessionDiagnosticsArray
onSessionsDiagnosticsSummaryNode
. - Filter the results for items containing my expected session name, ordered by that name and select the head (name is appended with tick stamp).
- Then I call
getObjectNode
on the client address space and provide thesessionId
of the session I just retrieved andclassOf[SessionDiagnosticsObjectNode]
as the arguments. - Finally, I call
getSubscriptionDiagnosticsArray
on the object node returned.
Now I have subscription diagnostic information for each subscription I want to transfer.
- I call
transferSubscription
on theUaClient
. - From here I can call
GetMonitoredItems
for each subscription. - I take the results of that and get the first client handle, then lookup the subscription configuration associated with that client handle. Now I know which server subscription id matches which of my subscription configurations.
- Then I construct the
TransferMonitoredItemsRequest
for each grouping of monitoredItem/notification handlers that I need to reconstruct. - This also requires me to create the
MonitoredItemTransferRequest
s for each item, using the earlierGetMonitoredItems
results for server handles and and my configuration. - Finally I call
transferSubscription
on theUaSubscriptionManager
passing in the information from my subscription diagnostics, subscription config, and the constructedTransferMonitoredItemRequests
.
I think I wrote this in a way to allow other approaches as needed by other implementations, but having mine as reference I thought my be helpful.
I want to see if I can piggy back on this to add a "local" transfer API as well. That way there's an easy to use transfer for when you're transferring on the same/local machine and have UaSubscription
and UaMonitoredItems
on hand, and this more sophisticated "remote" transfer option for transferring to a remote machine.
If there's not an easy to use "local" transfer API I'm going to have to spend a lot of time trying to support and explain how to use this "remote" transfer API.
A separate "local" transfer makes sense to me, no need for some of the overhead that the "remote" transfer requires.
@marcus-orchard do you mind if I start pushing some minor changes to this PR as I review?
@marcus-orchard do you mind if I start pushing some minor changes to this PR as I review?
Not at all. Do you want to address the items you mentioned in your review as well, or would you like me to tackle those?
I think TransferMonitorItemResponse
also needs to be refactored to TransferMonitorItemsResponse
.
I'll make the changes, I've got it open right now.
I've intentionally not opted into Java's broken serialization system (and gone towards immutable objects with final fields), so no-arg constructors aren't necessary or desirable.
I like the "trick" where you call modify when "transferring" monitored items to make sure you have current values for all the revised fields. I'm wondering if that same trick might be worth doing for subscriptions, even though you can technically get that information from the diagnostics. If an application configuration had all the other original/requested subscription values it might eliminate the need to read from the diagnostics?
So, I did consider this at one point.
I think where I abandoned the idea is when I realized I have to query the subscription diagnostics anyways to get the subscription Ids to transfer, so I just grab the diagnostics when I'm querying that, and calling modifySubscription
would be more overhead.
What initially interested me in this approach is that it reduces the information you need to provide when transferring a subscription, leading me to entertain the idea that it could potentially satisfy a larger number of unknown use cases.
But, another part of me thinks, "if they have a means to know the subscriptionId that the server assigned, I don't know why they wouldn't have the means to also know publishingInterval, lifetimeCount, etc." ; and if that is true then, again, calling modifySubscription
would be just more overhead.
Does that make sense? I did consider this as an option, but I'm open to the idea that I missed something that moves the needle more toward using the modifySubscription
approach.
Maybe to be safe we add an overload that uses this approach on the off chance there is a customer that somehow knows which subscriptionId
's they need to transfer without using diagnostics and that only has access to the subscriptionId
and their config?
I get what you're saying. I forgot that you don't even have subscription ids as the client taking over.
An overload might not be a bad idea, I'll think about it a bit.
If a client did somehow have subscription ids, or if there were some method that could be called to obtain subscription ids (not sure how that would work... call a method to get existing session ids belonging to the same user, then call a method against the most recent session id to get existing subscriptions?), it might be possible to do a transfer without relying on diagnostics.
What I'm getting at is, while right now it appears diagnostics are required, I'm wondering if we can learn something from implementing this and I can use it to make some suggestions for OPC UA 1.05 on how to make client redundancy and subscription transfer suck less.
@marcus-orchard what do you think about taking the transferMonitoredItems
methods off of the UaSubscription
interface and just giving them local visibility in OpcUaSubscription?
Is there some scenario where a user might be calling these themselves rather than letting UaSubscriptionManager
handle the transfer of the subscription and monitored items? Making these visible to the user seems like it might increase the odds of misuse.
TODO: my local transfer implementation is still slightly broken because it doesn't offer the caller a chance to hook up monitored item event and value callbacks before the notification semaphore is released.
@marcus-orchard what do you think about taking the
transferMonitoredItems
methods off of theUaSubscription
interface and just giving them local visibility in OpcUaSubscription?Is there some scenario where a user might be calling these themselves rather than letting
UaSubscriptionManager
handle the transfer of the subscription and monitored items? Making these visible to the user seems like it might increase the odds of misuse.
Agreed.
Hey, just wanted to let you know I haven't forgotten this, I've just been wildly busy the last couple weeks at both work and home. Things should calm down after next week.
No worries at all and I was worried you thought the same about me. I'll keep a look out for activity and keep pressing on with other things.
Hello there, any chance of this PR being merged in the foreseeable future? Or is there some blocking problem with it? I would have a use for inter process subscriptions transfer.
@FSangouard I don't know... it will take some work to make it mergeable and then try to understand it and make sure it's (still) sane.
I probably won't be able to take a good look at this until after I attend the OPC interoperability event in October. In the mean time I am working hard towards OPC UA 1.05 support that I plan to test at the event.