-
Notifications
You must be signed in to change notification settings - Fork 30
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
Adapter operational state #299
Conversation
…'t correct to use
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #299 +/- ##
==========================================
+ Coverage 81.09% 81.11% +0.01%
==========================================
Files 18 18
Lines 7841 7958 +117
==========================================
+ Hits 6359 6455 +96
- Misses 1482 1503 +21
☔ View full report in Codecov by Sentry. |
|
||
/* Override timeout, rounding up as necessary */ | ||
config->ack_timeout_seconds = aws_timestamp_convert( | ||
connect_task->protocol_operation_timeout_ms + AWS_TIMESTAMP_MILLIS - 1, |
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'm curious about why would we add 1 second to the operation timeout here?
|
||
aws_mqtt5_operation_release(&publish_op->publish_op->base); | ||
|
||
aws_mem_release(operation->allocator, operation); |
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.
Would we like to release publish_op
instead of operation
here as we allocate the publish_op
in aws_mqtt3_to_mqtt5_adapter_operation_new_publish
?
Meanwhile, if it was an issue, the tests should catch it. Probably I was wrong here?
struct aws_mqtt_client_connection_5_impl *adapter = operation->base.adapter; | ||
|
||
aws_mqtt3_to_mqtt5_adapter_operation_dereference_adapter(&operation->base); | ||
aws_mqtt5_client_submit_operation_internal( |
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.
Would it make more sense to submit first then dereference the operation...?
Shared subscription set implementation * Topic tree support for mqtt 311 per-subscription publish received callbacks * Per-topic-filter subscription record tracking for resubscribe support * The subscription set could eventually replace the topic tree in the mqtt 311 implementation, but for now leave it alone --------- Co-authored-by: Bret Ambrose <[email protected]>
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.