Reconnection and publish engine implementation #298
Conversation
OPC UA client reconnection procedure implemented Publish engine implementation Session refactorized Option AutoReconnect added Secure channel run context moved from client to secure channel Subscription publish mechanism removed, now implemented in publish engine Debug message added
|
Thank you. I will have a look. Is it necessary to have all these changes in a single commit or would it be possible to make a set of smaller changes? Out of curiosity: what motivated this change? Is this fixing an existing problem or cleanup? |
|
I had a look at this and I think this PR is too complex to just get merged. I think it is also refactoring too many things at the same time which makes it hard to impossible to review. Could you please split this up into smaller commits and/or PRs and first open an issue on what you are intending to change, why this needs fixing and what your approach is? It does not have to be an epic but should provide some context. Then we have a better baseline for reviewing the change. |
|
I understand this modification is a bit intrusive and I'm sorry about that... My first motivation was to create an input plugin for Telegraf. To answer your question for smaller changes per commit/pull request, I've already squashed the temporary branch and in doing so deleted it. To be honnest, at the beggining, I only wanted to implement the reconnection mechanism, but to do it correcly I had to pile up other changes sush as the publish engine... I could remove the session keep alive and the debug messages, but I doubt it would make it easier to understand, if you have any suggestion as how I could make it easier I'll be happy to do it. |
|
Ok. We will have a look then. Anybody else from the community want to lend a hand? Let’s try to do this properly. @unknownet I assume you’re open to comments and changes. |
|
I can probably do a first sweep today or tomorrow. |
|
I am interested in reviewing. But I agree with @magiconair, if we can break this into multiple PRs then it becomes a lot easier to review. |
|
Building reviewable PRs is work, I understand that. One thing to consider @unknownet is that if you don't invest the time to provide us with a change that is easy to review then you somewhat expect us to do that for you. |
|
Yes I am fully aware of your last point, I am trying split into multiple PRs while I am writing, and to come up with a suggestion of how to do it, in a way that could optimize both our time. As a first uncomplete draft, I would start by implementing the backbone of the reconnection mechanism (the bare minimum) and the session reactivation. I haven't figure the rest yet, does it sound more feasable ? |
|
I think that makes more sense. Refactoring in a separate commit before you add the feature is a good strategy. Also, please don't get me wrong. I think this will be definitely a useful feature and you're also not the first one asking for or attempting to write it. We'll help you to get this over the line. |
|
See #239 |
Hello,
I've performed some modifications of your library to follow more closely the OPC standard and so I added the following:
engine
I followed this diagram, that you can find in the lastest OPC UA specification (Part 4 - fig36 p112).
!! FYI !!
This implementation could be better with a more robust event handling system but I didn't wanted to alter too much the existing architecture.
I would suggest something similar to this as a notification mechanism.