-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Access] Enable Event streaming on REST API #4547
[Access] Enable Event streaming on REST API #4547
Conversation
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.
overall this approach looks good. I added a few comments, but didn't review thoroughly since this is still a work in progress.
… to separate module
…remove unnecessary logs
module/state_synchronization/requester/execution_data_requester_test.go
Outdated
Show resolved
Hide resolved
…g to last comments. Linted
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.
thanks for the updates! Added a few more, mostly nits and testing requests.
I tried to test this out using localnet and this curl command:
curl -vvv --include \
--no-buffer \
--header "Connection: Upgrade" \
--header "Upgrade: websocket" \
--header "Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==" \
--header "Sec-WebSocket-Version: 13" \
http://localhost:4004/v1/subscribe_events
it seems to timeout after a few seconds. I modified the code a bit to return errors from the read handler, and it returned
�>�read tcp 192.168.80.10:8070->192.168.80.1:37030: i/o timeout* Connection #0 to host localhost left intact
I suspect this is from the pong handler config.
Thank you for all your comments! The timeout issue you encountered related to the WebSocket ping-pong mechanism. Curl is not well-suited for WebSocket testing as it does not handle WebSocket ping-pong processes. I tried using curl with the Gorilla WebSocket 'chat' example and received the same I recommend |
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.
Looks good from my side, good job.
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.
Looking good. I think this is ready after these last few comments.
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.
looks good. Great work @UlyanaAndrukhiv!
#4379
This pull request:
wsroute
withSubscribeEvents
and its handler implementation to existing REST router,Handler
by moving common logic toHttpHandler
,