-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Mqtt subtitute library + fix retain topics + fix lose message #4039
Conversation
…16-beta with a more used MQTTnet Version="4.2.0.706" this library is industry used - i connect e disconnect broker, not work for retain topic (every time resend all topics) - fix sometimes connect and disconnect from broker lose some mqtt message
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.
Thanks for the great work! I think that after a little bit of cleanup we can merge it.
//using Elsa.Activities.Mqtt.Testing; | ||
//using Elsa.Activities.Mqtt.Testing.Handlers; |
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.
Should we remove these commented lines?
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.
Sorry, I deleted in the last p.r.
Not necessary
using Elsa.Activities.Mqtt.Services; | ||
using Elsa.ActivityResults; | ||
using Elsa.Attributes; | ||
using Elsa.Services.Models; | ||
|
||
|
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.
I see a couple of blank lines added and two namespaces imported, but I don't see any types from them being used here. Perhaps the changes in this file aren't necessary? I could be wrong, I haven't opened it in an IDE, just asking if you can check to make sure 😄
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.
Sorry, I deleted in the last p.r.
Not necessary
var newMessageReceiver = ActivatorUtilities.CreateInstance<MqttClientWrapper>(_serviceProvider, newClient, options); | ||
|
||
_receivers.Add(options.GetHashCode(), newMessageReceiver); | ||
_receivers.Add(options.GetHashCode(), new ( DateTime.Now, newMessageReceiver )); |
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.
Instead of accessing DateTie.Now
directly, it would be better to inject IClock
and get the current date from there.
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.
Sorry, I deleted because not used (take a look new p.r.)
return messageReceiver; | ||
if (_receivers.TryGetValue(options.GetHashCode(), out var messageReceiverDateTime)) | ||
{ | ||
messageReceiverDateTime.Item1 = DateTime.Now; //Timeout. |
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.
Instead of accessing DateTie.Now
directly, it would be better to inject IClock
and get the current date from there.
this library is industry used