-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: CloudConnection did not forward close() call to cloud proxy #4546
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
...b/core/src/Microsoft.Azure.Devices.Edge.Hub.MqttBrokerAdapter/upstream/BrokeredCloudProxy.cs
Outdated
Show resolved
Hide resolved
|
||
try | ||
{ | ||
result = await this.CloudProxy.Match( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: suggest: .Map().GetOrElse(true)
|
||
try | ||
{ | ||
result = await this.CloudProxy.Match( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test?
|
||
public class BrokeredCloudConnection : ICloudConnection | ||
{ | ||
IIdentity identity; | ||
|
||
public BrokeredCloudConnection(BrokeredCloudProxy cloudProxy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not a ICloudProxy? We should avoid passing in actual types.. if possible.
this.IsActive = true; | ||
this.CloudProxy = Option.Some(cloudProxy as ICloudProxy); | ||
this.identity = cloudProxy.Identity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd... can we just pass in the Identity separately, like we do for CloudConnection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put at least 1 UT covering the change, if we need more thorough tests, we can add those in another pr if that is easier or not a high priority. Will approve after that :)
- BrokeredCloudConnection now relies on underlying CloudProxy for active status instead of maintaining itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
…re#4546) The main change is in BrokeredCloudConnection.cs where it did not forward the call to CloudProxy. The rest of the change is only about to being able to log the Identity in case of failure. The removed "TODO" about the subscription is not needed anymore as the proper place was in ConnectionHander.cs/RemoveConnectionAsync() to remove existing subscriptions.
The main change is in BrokeredCloudConnection.cs where it did not forward the call to CloudProxy. The rest of the change is only about to being able to log the Identity in case of failure.
The removed "TODO" about the subscription is not needed anymore as the proper place was in ConnectionHander.cs/RemoveConnectionAsync() to remove existing subscriptions.