rumqtt icon indicating copy to clipboard operation
rumqtt copied to clipboard

Graceful shutdown

Open silvestrpredko opened this issue 1 year ago • 12 comments

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

  • [x] Formatted with cargo fmt
  • [x] Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

silvestrpredko avatar Jun 13 '24 12:06 silvestrpredko

Short description: I wanna make a graceful shutdown without a significant redesign of the system. For me watch seems like a good solution. Please don't hesitate to suggest improvements to the initial design.

silvestrpredko avatar Jun 13 '24 14:06 silvestrpredko

i also made a poc for graceful shutdown using cancellation token, you can check it here https://github.com/bytebeamio/rumqtt/compare/main...rumqttd-shutdown !

we can compare both approaches and combine good stuff from both :)

swanandx avatar Jun 13 '24 15:06 swanandx

Pull Request Test Coverage Report for Build 9501924124

Details

  • 0 of 71 (0.0%) changed or added relevant lines in 4 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.1%) to 36.033%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rumqttd/src/link/bridge.rs 0 1 0.0%
rumqttd/src/link/timer.rs 0 1 0.0%
rumqttd/src/link/console.rs 0 4 0.0%
rumqttd/src/server/broker.rs 0 65 0.0%
<!-- Total: 0 71
Files with Coverage Reduction New Missed Lines %
rumqttd/src/link/timer.rs 1 0.0%
rumqttd/src/server/broker.rs 3 0.0%
<!-- Total: 4
Totals Coverage Status
Change from base Build 9501879560: -0.1%
Covered Lines: 5970
Relevant Lines: 16568

💛 - Coveralls

coveralls avatar Jun 13 '24 15:06 coveralls

i also made a poc for graceful shutdown using cancellation token, you can check it here main...rumqttd-shutdown !

we can compare both approaches and combine good stuff from both :)

I like that handle returns in start. The reason why I was ignoring CancellationToken it's that tokio-utils is an optional dependency and I just don't want to change it. So it's more up to you what changes you would like to make, and what it should be used at the end. Please write below your decision.

silvestrpredko avatar Jun 13 '24 15:06 silvestrpredko

Pull Request Test Coverage Report for Build 9521027502

Details

  • 0 of 73 (0.0%) changed or added relevant lines in 5 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.09%) to 36.042%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rumqttd/src/link/bridge.rs 0 1 0.0%
rumqttd/src/link/timer.rs 0 1 0.0%
rumqttd/src/link/console.rs 0 4 0.0%
rumqttd/src/main.rs 0 8 0.0%
rumqttd/src/server/broker.rs 0 59 0.0%
<!-- Total: 0 73
Files with Coverage Reduction New Missed Lines %
rumqttd/src/link/timer.rs 1 0.0%
rumqttd/src/main.rs 1 0.0%
rumqttd/src/server/broker.rs 3 0.0%
<!-- Total: 5
Totals Coverage Status
Change from base Build 9501879560: -0.09%
Covered Lines: 5970
Relevant Lines: 16564

💛 - Coveralls

coveralls avatar Jun 14 '24 19:06 coveralls

Hey @swanandx. I redesigned the approach a little bit. It seems like a good solution. Please check it out.

P.S I also added join API call for situation when you don't want to handle a graceful shutdown

silvestrpredko avatar Jun 14 '24 21:06 silvestrpredko

Pull Request Test Coverage Report for Build 9522368042

Details

  • 0 of 80 (0.0%) changed or added relevant lines in 5 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.1%) to 36.027%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rumqttd/src/link/bridge.rs 0 1 0.0%
rumqttd/src/link/timer.rs 0 1 0.0%
rumqttd/src/link/console.rs 0 4 0.0%
rumqttd/src/main.rs 0 8 0.0%
rumqttd/src/server/broker.rs 0 66 0.0%
<!-- Total: 0 80
Files with Coverage Reduction New Missed Lines %
rumqttd/src/link/timer.rs 1 0.0%
rumqttd/src/main.rs 1 0.0%
rumqttd/src/server/broker.rs 3 0.0%
<!-- Total: 5
Totals Coverage Status
Change from base Build 9501879560: -0.1%
Covered Lines: 5970
Relevant Lines: 16571

💛 - Coveralls

coveralls avatar Jun 14 '24 21:06 coveralls

Hey, @swanandx do you have a chance to look at this PR?

silvestrpredko avatar Jul 09 '24 11:07 silvestrpredko

Pull Request Test Coverage Report for Build 10043003235

Details

  • 0 of 80 (0.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on silvestr/graceful-shutdown at 36.033%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rumqttd/src/link/bridge.rs 0 1 0.0%
rumqttd/src/link/timer.rs 0 1 0.0%
rumqttd/src/link/console.rs 0 4 0.0%
rumqttd/src/main.rs 0 8 0.0%
rumqttd/src/server/broker.rs 0 66 0.0%
<!-- Total: 0 80
Totals Coverage Status
Change from base Build 9675366490: 36.0%
Covered Lines: 5970
Relevant Lines: 16568

💛 - Coveralls

coveralls avatar Jul 22 '24 15:07 coveralls

@swanandx Just I kindly reminder. Could you please take a look?

silvestrpredko avatar Jul 22 '24 16:07 silvestrpredko

@swanandx Just I kindly reminder. Could you please take a look?

Hey, I sincerely apologise for the delay, but I'm not having any bandwidth as of now or in near future. So this might be delayed 😕

swanandx avatar Jul 22 '24 16:07 swanandx

@swanandx Any updates on this? Is there another member that can take over the review?

baxterjo avatar Dec 05 '24 18:12 baxterjo