Skip to content

refactor[websocket]: improve websocket factory start #1145

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

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

luislhl
Copy link
Collaborator

@luislhl luislhl commented Oct 4, 2024

Motivation

I had a problem while trying to write a test for run_node.py in another PR. The reason was that the HathorAdminWebsocketFactory was started and never stopped.

I noticed that its stop method was never called, and its start was called inside ResourcesBuilder.create_resources, which was unlike any other resource we start.

I've included a test similar to the one I had problems with to make my difficulties clearer.

Acceptance Criteria

  • The HathorAdminWebsocketFactory should be started as a consequence of starting the Manager, and should be stopped when the Manager is stopped
  • There should be no impact in the overall logic of hathor-core. Everything should work like before.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@luislhl luislhl self-assigned this Oct 4, 2024
Copy link

github-actions bot commented Oct 4, 2024

🐰 Bencher Report

Branchchore/improve-websocket-factory-start
Testbedubuntu-22.04
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)
Lower Boundary
nanoseconds (ns)
(Limit %)
Upper Boundary
nanoseconds (ns)
(Limit %)
sync-v2 (up to 20000 blocks)📈 view plot
🚷 view threshold
101,433,125,219.18
(-0.47%)
91,721,030,329.48
(90.43%)
112,103,481,513.80
(90.48%)
🐰 View full continuous benchmarking report in Bencher

@luislhl luislhl force-pushed the chore/improve-websocket-factory-start branch 2 times, most recently from b6e9207 to 132943f Compare October 4, 2024 20:56
@luislhl luislhl changed the title chore[websocket]: improve websocket factory start refactor[websocket]: improve websocket factory start Oct 4, 2024
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.47%. Comparing base (22f478c) to head (09e7c50).

Files with missing lines Patch % Lines
hathor/builder/sysctl_builder.py 0.00% 1 Missing ⚠️
hathor/metrics.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1145      +/-   ##
==========================================
- Coverage   84.50%   84.47%   -0.04%     
==========================================
  Files         317      317              
  Lines       24384    24389       +5     
  Branches     3707     3709       +2     
==========================================
- Hits        20606    20602       -4     
- Misses       3069     3072       +3     
- Partials      709      715       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jansegre
jansegre previously approved these changes Oct 7, 2024
@luislhl luislhl force-pushed the chore/improve-websocket-factory-start branch from 132943f to 59af74d Compare October 7, 2024 19:34
@luislhl luislhl enabled auto-merge (squash) October 7, 2024 22:33
@luislhl luislhl force-pushed the chore/improve-websocket-factory-start branch from 59af74d to 1dc63f2 Compare October 7, 2024 22:33
@jansegre jansegre mentioned this pull request Oct 7, 2024
2 tasks
@luislhl luislhl force-pushed the chore/improve-websocket-factory-start branch from 1dc63f2 to 09e7c50 Compare October 8, 2024 15:37
@luislhl luislhl merged commit d85f88c into master Oct 8, 2024
12 of 13 checks passed
@luislhl luislhl deleted the chore/improve-websocket-factory-start branch October 8, 2024 16:03
@jansegre jansegre mentioned this pull request Oct 14, 2024
2 tasks
@jansegre jansegre mentioned this pull request Dec 11, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants