[FabricRuntime] Support Package change notification callbacks #96
Conversation
cgettys-microsoft
changed the title
cgettys-microsoft
changed the title
| handle: ConfigurationPackageChangeHandlerId | ||
| } | ||
|
|
||
| impl ConfigurationPackageChangeEventHandlerManager |
There was a problem hiding this comment.
There was a problem hiding this comment.
And for simplicity, just return a handle and let caller unregister it: https://github.com/Azure/service-fabric-rs/blob/2c8933b52016bb9f864f1e7aa55a49df2cb27d85/crates/libs/core/src/client/svc_mgmt_client.rs#L226C1-L232C1
There was a problem hiding this comment.
Provided a simpler struct for caller to drop. Would rather not risk leaks if we can help it. I see why you did what you did, and lifetime bound would also work, but I think this is cleaner from a usage perspective - drop handle, everything is cleaned up.
There was a problem hiding this comment.
There is already another handle in fabric client with similar intention:
There should be 2 separate concerns in mssf. 1) Exposing the api in rust and make it clear to SF user what this api corresponds to in COM. 2) make the rust api safe.
The code currently achieves 2), but not 1).
The most wrapper type in mssf can convert from rust type and ffi raw type. My recommendation is to provide COM corresponding rust functions at 1 layer, and another layer to make things safe.
There was a problem hiding this comment.
Did it your way
cgettys-microsoft
changed the title
cgettys-microsoft
changed the title
|
|
||
| // One might wish to use such a callback to e.g. trigger custom handling of configuration changes | ||
| // This doesn't require the config feature to be enabled | ||
| let _handler = my_ctx.register_config_package_change_handler( |c| |
There was a problem hiding this comment.
U should move this to one of the example apps.
There was a problem hiding this comment.
Huh? this is one of the samples
There was a problem hiding this comment.
Oh, you mean one of the full applications.
Yeah, that could make sense.
I do want to ensure this continues to build without those features though.
There was a problem hiding this comment.
I think the logical example to add it to would be echomain
As we could demonstrate how to hook it up to trigger a Config reload
There was a problem hiding this comment.
Basically this but with our source:
https://github.com/rust-cli/config-rs/blob/main/examples/watch/main.rs
There was a problem hiding this comment.
Given I'd like to have this build here as well, can I defer that to a second PR?
There was a problem hiding this comment.
U can keep it here or add to the working app.
But note that this code here cannot run because it is not launched by SF, and we are not packaging it in an app.
There was a problem hiding this comment.
Added it to echomain as well (but haven't had a chance to test that app yet)
Right, point of this is to make sure we can package w/o tokio
End user applications probably want to handle package change notifications.
This PR currently tackles only ConfigurationPackage.
If you're happy with the approach, I'll do the same exact implementation (edit: in additional PRs) for CodePackage and DataPackage and that will close out #97.