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.

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 by Bors] - Make grpc servers' start methods be synchronous #4866

Closed
wants to merge 6 commits into from

Conversation

piersy
Copy link
Contributor

@piersy piersy commented Aug 17, 2023

Motivation

Closes #4861

Changes

Updates grpc server startup code to execute synchronously, returning only after the server has started listening.

Test Plan

Existing tests should pass

DevOps Notes

  • This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
  • This PR does not affect public APIs
  • This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
  • This PR does not make changes to log messages (which monitoring infrastructure may rely on)

api/grpcserver/grpcserver_test.go Outdated Show resolved Hide resolved
cmd/bootstrapper/generator_test.go Outdated Show resolved Hide resolved
@piersy
Copy link
Contributor Author

piersy commented Aug 17, 2023

bors merge

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Motivation
Closes #4861 
## Changes

Updates grpc server startup code to execute synchronously, returning only after the server has started listening.

## Test Plan
Existing tests should pass

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
@piersy
Copy link
Contributor Author

piersy commented Aug 17, 2023

bors cancel

@bors
Copy link

bors bot commented Aug 17, 2023

Canceled.

@piersy
Copy link
Contributor Author

piersy commented Aug 17, 2023

bors merge

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Motivation
Closes #4861 
## Changes

Updates grpc server startup code to execute synchronously, returning only after the server has started listening.

## Test Plan
Existing tests should pass

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
@bors
Copy link

bors bot commented Aug 17, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Aug 17, 2023
## Motivation
Closes #4861 
## Changes

Updates grpc server startup code to execute synchronously, returning only after the server has started listening.

## Test Plan
Existing tests should pass

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
@bors
Copy link

bors bot commented Aug 17, 2023

Build failed:

Remove erroneous blocking call to serve in grpc http server.
Return error returned by grpc server Start methods.
Fix bad peer test config.
@piersy piersy force-pushed the piersy/grpcserver-synchronous-start branch from 19d609e to 33808b6 Compare August 17, 2023 17:49
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #4866 (5ec4c6a) into develop (c479d5b) will decrease coverage by 0.1%.
Report is 2 commits behind head on develop.
The diff coverage is 61.3%.

@@            Coverage Diff            @@
##           develop   #4866     +/-   ##
=========================================
- Coverage     76.9%   76.8%   -0.1%     
=========================================
  Files          261     261             
  Lines        30103   30091     -12     
=========================================
- Hits         23166   23128     -38     
- Misses        5453    5474     +21     
- Partials      1484    1489      +5     
Files Changed Coverage Δ
node/node.go 65.0% <0.0%> (-0.5%) ⬇️
api/grpcserver/grpc.go 75.0% <50.0%> (-25.0%) ⬇️
api/grpcserver/http_server.go 79.1% <78.5%> (-9.8%) ⬇️

... and 3 files with indirect coverage changes

@piersy
Copy link
Contributor Author

piersy commented Aug 18, 2023

bors merge

bors bot pushed a commit that referenced this pull request Aug 18, 2023
## Motivation
Closes #4861 
## Changes

Updates grpc server startup code to execute synchronously, returning only after the server has started listening.

## Test Plan
Existing tests should pass

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
@bors
Copy link

bors bot commented Aug 18, 2023

Pull request successfully merged into develop.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Make grpc servers' start methods be synchronous [Merged by Bors] - Make grpc servers' start methods be synchronous Aug 18, 2023
@bors bors bot closed this Aug 18, 2023
@bors bors bot deleted the piersy/grpcserver-synchronous-start branch August 18, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GRPC servers unnecessarily do startup asynchronously
3 participants