Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add jds-do-not-fail-on-jdc-shutdown integration test #1360

Merged
merged 9 commits into from
Feb 1, 2025
Merged

Add jds-do-not-fail-on-jdc-shutdown integration test #1360

merged 9 commits into from
Feb 1, 2025

Conversation

Shourya742
Copy link
Contributor

@Shourya742 Shourya742 commented Jan 20, 2025 •
edited
Loading

Closes: #1348 #1320 #1349

  1. On side this PR changes the shutdown api on tproxy and jdc:
pub fn shutdown(&self)
  1. Add unit test for these
  2. Add IT for jds-do-not-fail-on-jdc-shutdown
  3. Remove jds-do-not-fail-on-jdc-shutdown MG test

@codecov Codecov
Copy link

codecov bot commented Jan 20, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.

Project coverage is 17.95%. Comparing base (41e12e2) to head (1fe2395).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
roles/jd-client/src/lib/mod.rs 9.09% 10 Missing ⚠️
roles/translator/src/lib/mod.rs 33.33% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1360      +/-   ##
==========================================
- Coverage   18.79%   17.95%   -0.85%     
==========================================
  Files         167      167              
  Lines       11348    11936     +588     
==========================================
+ Hits         2133     2143      +10     
- Misses       9215     9793     +578
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <ø> (ø)
binary_serde_sv2-coverage 3.40% <ø> (ø)
binary_sv2-coverage 5.11% <ø> (ø)
bip32_derivation-coverage 0.00% <ø> (ø)
buffer_sv2-coverage 25.02% <ø> (ø)
codec_sv2-coverage 0.01% <ø> (ø)
common_messages_sv2-coverage 0.12% <ø> (ø)
const_sv2-coverage 0.00% <ø> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage 0.27% <ø> (ø)
jd_client-coverage 0.38% <9.09%> (+0.38%) ⬆️
jd_server-coverage 7.79% <ø> (ø)
job_declaration_sv2-coverage 0.00% <ø> (ø)
key-utils-coverage 2.39% <ø> (ø)
mining-coverage 2.32% <ø> (-0.02%) ⬇️
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.70% <ø> (ø)
noise_sv2-coverage 4.25% <ø> (ø)
pool_sv2-coverage 2.04% <ø> (ø)
protocols 23.86% <ø> (ø)
roles 5.83% <14.28%> (-0.72%) ⬇️
roles_logic_sv2-coverage 8.70% <ø> (ø)
sv2_ffi-coverage 0.00% <ø> (ø)
template_distribution_sv2-coverage 0.00% <ø> (ø)
translator_sv2-coverage 8.72% <33.33%> (-0.88%) ⬇️
utils 25.13% <ø> (ø)
v1-coverage 2.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Shourya742 Shourya742 force-pushed the 2025-01-20-change-shutdown-api-and-add-unit-test branch from 68191ad to e0825a8 Compare January 20, 2025 11:52
@plebhash plebhash mentioned this pull request Jan 20, 2025
Open
jbesraa
jbesraa suggested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks super with the tests!

roles/jd-client/src/lib/mod.rs Show resolved Hide resolved
roles/jd-client/src/lib/mod.rs Show resolved Hide resolved
roles/jd-client/src/lib/mod.rs Show resolved Hide resolved
roles/jd-client/src/lib/mod.rs Outdated Show resolved Hide resolved
roles/tests-integration/tests/translator_integration.rs Outdated Show resolved Hide resolved
roles/tests-integration/tests/translator_integration.rs Outdated Show resolved Hide resolved
roles/tests-integration/tests/translator_integration.rs Outdated Show resolved Hide resolved
roles/jd-client/src/lib/mod.rs Outdated Show resolved Hide resolved
test/config/jds-do-not-panic-if-jdc-close-connection/jds-config.toml Show resolved Hide resolved
@Shourya742 Shourya742 force-pushed the 2025-01-20-change-shutdown-api-and-add-unit-test branch 2 times, most recently from a2203ff to 5b42f52 Compare January 21, 2025 16:35
jbesraa
jbesraa suggested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Added few nits. Could you also please apply the docs changes to JDC::shutdown?

roles/jd-client/src/lib/mod.rs Show resolved Hide resolved
roles/jd-client/src/lib/mod.rs Show resolved Hide resolved
roles/translator/src/lib/mod.rs Outdated Show resolved Hide resolved
roles/translator/src/lib/mod.rs Outdated Show resolved Hide resolved
@Shourya742 Shourya742 force-pushed the 2025-01-20-change-shutdown-api-and-add-unit-test branch 4 times, most recently from 086f3a4 to 6f05a39 Compare January 22, 2025 15:26
@jbesraa
Copy link
Contributor

jbesraa commented Jan 23, 2025

@Shourya742 Please fixup 6f05a39 to its correct place

@Shourya742 Shourya742 force-pushed the 2025-01-20-change-shutdown-api-and-add-unit-test branch 4 times, most recently from b933a2b to d147540 Compare January 24, 2025 06:41
@Shourya742 Shourya742 mentioned this pull request Feb 1, 2025
Closed
@plebhash plebhash added the ready-to-be-merged triggers auto rebase bot label Feb 1, 2025
@pavlenex pavlenex force-pushed the 2025-01-20-change-shutdown-api-and-add-unit-test branch from d147540 to 353c1d6 Compare February 1, 2025 01:17
@Shourya742
modify translator shutdown api: to directly shutdown rather than prov…
b6ba18a 
…iding shutdown handler
@Shourya742
terminate all child task in translator
c0f0c49
@Shourya742
�[200~add unit test for translator shutdown api
db083f8
@Shourya742
modify jdc shutdown api: to directly shutdown rather than providing s…
29fbb91 
…hutdown handler

add unit test for jdc shutdown api
@Shourya742
Upon shutdown signalling exit process via loop exit and not via proce…
21b3bc5 
…ss exit syscall
@Shourya742
Handle Root Future Termination in Shutdown Scenarios
b631089 
This is crucial because when the `start` method is executed
in a runtime that is not the main one, the lifecycle of futures
spawned within the `start` method continues, even after the `jdc`
process terminates.The earlier approach of simply exiting the
loop worked because the `start` method was executed on the main
blocking thread, where the Tokio runtime was defined. However,
in the case of integration tests, the runtime runs on a different
blocking thread, necessitating proper handling of the root
future termination.
@Shourya742
remove jd-does-not-panic-if-jdc-disconnect from ci's mg yaml and mg t…
263183e 
…est from test module
@Shourya742 Shourya742 force-pushed the 2025-01-20-change-shutdown-api-and-add-unit-test branch from 353c1d6 to 1fe2395 Compare February 1, 2025 02:09
@plebhash plebhash merged commit 1ee8977 into stratum-mining:main Feb 1, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plebhash plebhash plebhash approved these changes

@jbesraa jbesraa jbesraa requested changes

Assignees
No one assigned
Labels
ready-to-be-merged triggers auto rebase bot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test JDC shutdown behavior
3 participants
@Shourya742 @jbesraa @plebhash