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

RPC Notifier Signal when Setup Complete #27481

Merged
merged 2 commits into from
Sep 1, 2022
Merged

RPC Notifier Signal when Setup Complete #27481

merged 2 commits into from
Sep 1, 2022

Conversation

bw-solana
Copy link
Contributor

@bw-solana bw-solana commented Aug 30, 2022 •
edited

Problem

See #27480. CI test flakiness

Summary of Changes

have new_with_config optionally signal back to callers when RPC Notifier thread pool routine has been installed. Have tests wait for setup to complete before proceeding with next steps.

Have new_for_tests call new_for_tests_with_blockstore to reduce redundant code

@bw-solana bw-solana linked an issue Aug 30, 2022 that may be closed by this pull request
test_logs_subscribe occasionally times out #27480
Closed
@bw-solana bw-solana marked this pull request as ready for review August 30, 2022 22:27
yihau
yihau previously approved these changes Aug 31, 2022
View reviewed changes
Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

rpc_subscriptions::tests::test_check_account_subscribe has a same problem in my branch. After I patch this commit, it seems okay.

@mergify mergify bot dismissed yihau’s stale review September 1, 2022 21:20

Pull request has been modified.

@bw-solana bw-solana changed the title Add delay for RPC notification setup RPC Notifier Signal when Setup Complete Sep 1, 2022
CriesofCarrots
CriesofCarrots approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks for reworking this! lgtm if CI is happy

@bw-solana
Copy link
Contributor Author

Thanks for reworking this! lgtm if CI is happy

Thanks for your help!

@bw-solana bw-solana merged commit 242c9cb into solana-labs:master Sep 1, 2022
@yihau
Copy link
Member

yihau commented Sep 2, 2022

just noticed there is a similar error on v1.11 https://buildkite.com/solana-labs/solana/builds/81131#0182fea8-6a0c-47d9-8dd2-2f8eb55dc509
maybe we could backfill this PR to v1.10 and v1.11 🤔

@CriesofCarrots
Copy link
Contributor

maybe we could backfill this PR to v1.10 and v1.11 🤔

Since this is in essence a test-only change, backporting seems reasonable.

@mergify mergify bot mentioned this pull request Sep 2, 2022
Merged
mergify bot pushed a commit that referenced this pull request Sep 2, 2022
@bw-solana @mergify
RPC Notifier Signal when Setup Complete (#27481)
8483023 
* RPC notifier signal when ready

(cherry picked from commit 242c9cb)
mergify bot pushed a commit that referenced this pull request Sep 2, 2022
@bw-solana @mergify
RPC Notifier Signal when Setup Complete (#27481)
4c94f77 
* RPC notifier signal when ready

(cherry picked from commit 242c9cb)
@mergify mergify bot mentioned this pull request Sep 2, 2022
Merged
mergify bot added a commit that referenced this pull request Sep 2, 2022
@mergify @bw-solana
RPC Notifier Signal when Setup Complete (backport #27481) (#27577)
fe81218 
RPC Notifier Signal when Setup Complete (#27481)

* RPC notifier signal when ready

(cherry picked from commit 242c9cb)

Co-authored-by: Brennan Watt <brennan.watt@solana.com>
mergify bot added a commit that referenced this pull request Sep 2, 2022
@mergify @bw-solana
RPC Notifier Signal when Setup Complete (backport #27481) (#27578)
c42c210 
RPC Notifier Signal when Setup Complete (#27481)

* RPC notifier signal when ready

(cherry picked from commit 242c9cb)

Co-authored-by: Brennan Watt <brennan.watt@solana.com>
hycinth22
hycinth22 reviewed Nov 23, 2022
View reviewed changes
core/src/validator.rs
@@ -722,6 +722,7 @@ impl Validator {
block_commitment_cache.clone(),
optimistically_confirmed_bank.clone(),
&config.pubsub_config,
None,

This comment was marked as resolved.

Sign in to view

hycinth22
hycinth22 reviewed Nov 23, 2022
View reviewed changes
rpc/src/rpc_subscriptions.rs
@@ -640,6 +656,9 @@ impl RpcSubscriptions {
.build()
.unwrap();
pool.install(|| {
if let Some(rpc_notifier_ready) = rpc_notifier_ready {
rpc_notifier_ready.fetch_or(true, Ordering::Relaxed);

This comment was marked as resolved.

Sign in to view

hycinth22
hycinth22 reviewed Nov 23, 2022
View reviewed changes
rpc/src/rpc_subscriptions.rs
@@ -640,6 +656,9 @@ impl RpcSubscriptions {
.build()
.unwrap();
pool.install(|| {
if let Some(rpc_notifier_ready) = rpc_notifier_ready {

This comment was marked as resolved.

Sign in to view

@solana-labs solana-labs locked as spam and limited conversation to collaborators Nov 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hycinth22 hycinth22 hycinth22 left review comments

@CriesofCarrots CriesofCarrots CriesofCarrots approved these changes

@yihau yihau yihau left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_logs_subscribe occasionally times out
4 participants
@bw-solana @yihau @CriesofCarrots @hycinth22