-
Notifications
You must be signed in to change notification settings - Fork 378
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
feat: add Ready
method to Node
#1216
Conversation
mostly to check if the node is ready Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1216 +/- ##
==========================================
- Coverage 56.13% 56.12% -0.02%
==========================================
Files 438 438
Lines 66151 66164 +13
==========================================
Hits 37134 37134
- Misses 26126 26139 +13
Partials 2891 2891
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
bbf1d42
to
dfb8b92
Compare
Please don't merge this PR yet, I'm trying to compose some thoughts on this issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gfanton Can you provide the use cases with examples why we will need this changes? I thought the integration test supposed to be conducted from outside of tendermint node service instance. Also are we trying to check first block on genesis playback or first block to be committed in consensus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking this PR, it's been in the freezer long enough
The fact that we can't rely on OnStart
ending to say that the node is ready is a bit disturbing. We will tackle this in a future PR, regardless, this PR should be unblocked as it introduces a nice UX moment for tests
@gfanton please resolve merge conflicts 🙏 |
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
@zivkovicmilos Done! Perhaps I should rename the method to @piux2 This step is essential for the integration tests to work correctly. Currently, I am using a small external hack (see the code bellow) to ensure that the node is ready and can handle requests properly. With this new method, we provide a more intuitive way to check if the node is ready. gno/gno.land/pkg/gnoland/node_inmemory.go Lines 156 to 178 in 12b4b45
|
Signed-off-by: gfanton <[email protected]>
FirstBlockReceived
method to NodeReady
method to Node
This PR adds a method to
Node
that returns a channel, which will close when the node receives its first block. The primary purpose of this is to provide a mechanism to check when the node is ready and can start handling requests.Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description