REST VC: Subscribe to Beacon API events #13453
Conversation
rkapka
changed the title
rkapka
force-pushed
the
subscribe-to-slots
branch
4 times, most recently
from
d60df01 to
4c29055
Compare
| @@ -25,10 +26,6 @@ type BeaconApiJsonRestHandler struct { | |||
| // Get sends a GET request and decodes the response body as a JSON object into the passed in object. | |||
| // If an HTTP error is returned, the body is decoded as a DefaultJsonError JSON object and returned as the first return value. | |||
| func (c BeaconApiJsonRestHandler) Get(ctx context.Context, endpoint string, resp interface{}) error { | |||
| if resp == nil { | |||
There was a problem hiding this comment.
"/eth/v1/node/health" doesn't return a body
| log.WithError(err).Error("Failed to create HTTP request") | ||
| } | ||
| req.Header.Set("Accept", api.EventStreamMediaType) | ||
| req.Header.Set("Connection", "keep-alive") |
There was a problem hiding this comment.
should we add this to the API constants? ( keep-alive)
| go func() { | ||
| h.running = true |
There was a problem hiding this comment.
hmm is this the best approach to this/ is this safe?
There was a problem hiding this comment.
It can be improved, I think. I will push a new commit
|
|
||
| // EventStreamIsRunning returns a dummy value for gRPC | ||
| func (c *grpcValidatorClient) EventStreamIsRunning() bool { | ||
| return true |
There was a problem hiding this comment.
we should have a log at least for the dummy value.. same with the above
There was a problem hiding this comment.
I think it would actually be better to panic because these functions should never be called
rkapka
force-pushed
the
subscribe-to-slots
branch
from
4c29055 to
adb1638
Compare
| if err = v.StartEventStream(ctx); err != nil { | ||
| log.WithError(err).Fatal("Could not start API event stream") | ||
| if features.Get().EnableBeaconRESTApi { | ||
| if err = v.StartEventStream(ctx); err != nil { |
There was a problem hiding this comment.
why not move it into the below check, there are 2 features.Get().EnableBeaconRestApi
rkapka
force-pushed
the
subscribe-to-slots
branch
from
8cf821a to
0f873aa
Compare
|
There are possible issues with setting the runner bool on errors that should be accounted for. please do not approve until they are resolved. |
What type of PR is this?
Other
What does this PR do? Why is it needed?
This is a better alternative to #13354. We reverted the original PR before a release because the functionality was not tested properly. As it turns out, restarting the node broke the stream and the original PR didn't have a recovery mechanism.
There is not need to review the first commit because it's only reapplying the reverted PR.