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

Add poll-based event source #3323

Merged
merged 36 commits into from
Jun 6, 2023
Merged

Add poll-based event source #3323

merged 36 commits into from
Jun 6, 2023

Conversation

romac
Copy link
Member

@romac romac commented May 9, 2023 •
edited
Loading

Closes: #2850

Fixes: #3190
Fixes: #2809

Might address: #3206 (see also #1299)

Description

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@romac romac changed the title Prepare for different types of event sources Add poll-based event source May 9, 2023
@romac romac mentioned this pull request May 9, 2023
Closed
7 tasks
@romac romac force-pushed the romac/poll-vs-push branch from 3ec205a to 396efc5 Compare May 9, 2023 12:52
@romac romac marked this pull request as ready for review May 16, 2023 10:18
@seanchen1991
Copy link
Contributor

As far as how this addresses the REST endpoint failing, Hermes would essentially switch to purely poll-based relaying in the case when the WebSocket connection gets closed?

@romac
Copy link
Member Author

romac commented May 16, 2023

No the hope is that this problem (which I have never been able to reproduce) wouldn't manifest itself when Hermes is configured in poll mode. There is no automatic switching between the two modes.

seanchen1991
seanchen1991 reviewed May 16, 2023
View reviewed changes
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

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

I'm a bit confused by the semantics around event monitor vs. event source. The terms seem to be used interchangeably. Do they carry different semantic meanings?

crates/relayer/src/chain/cosmos.rs Show resolved Hide resolved
crates/relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
crates/relayer/src/event/source/rpc.rs Show resolved Hide resolved
seanchen1991 and others added 4 commits May 16, 2023 10:56
@seanchen1991
Cargo fmt
ceb210a
@romac
Add more tracing to event sources
586c217
@romac @seanchen1991
Apply suggestions from code review
d9a691b 
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>
@romac
Change uses of event monitor to event source
1d40aed
@romac romac requested a review from seanchen1991 May 17, 2023 07:41
@kaisbaccour
Copy link

kaisbaccour commented May 21, 2023 •
edited
Loading

FYI I tested this branch for wasm packets between nois-testnet-005 and uni-6 with the event_source.rpc (1s poll_interval) and it seems to work well so far.

@romac
Copy link
Member Author

romac commented May 22, 2023

FYI I testnet this branch for wasm packets between nois-testnet-005 and uni-6 with the event_source.rpc (1s poll_interval) and it seems to work well so far.

Glad to hear, thanks for the heads-up! 🙏

ancazamfir
ancazamfir reviewed May 23, 2023
View reviewed changes
config.toml Outdated Show resolved Hide resolved
@romac romac added this to the v1.6 milestone May 23, 2023
@romac
Fix typo
164c05e 
Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>
ancazamfir
ancazamfir requested changes May 29, 2023
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Reviewed the code and did some tests with both modes with ordered and unordered channels and some channel handshake events also. Looks good, left a couple of comments.

crates/relayer/src/event/source/rpc/extract.rs Outdated Show resolved Hide resolved
config.toml Outdated Show resolved Hide resolved
crates/relayer/src/event/source/rpc.rs Outdated Show resolved Hide resolved
@romac romac requested a review from ancazamfir May 31, 2023 12:33
ancazamfir
ancazamfir approved these changes Jun 5, 2023
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Amazing work @romac !

@romac
Copy link
Member Author

romac commented Jun 5, 2023

Let's merge this after 1.5.1 is released.

@romac romac merged commit c1dbc79 into master Jun 6, 2023
@romac romac deleted the romac/poll-vs-push branch June 6, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ancazamfir ancazamfir ancazamfir approved these changes

@seanchen1991 seanchen1991 seanchen1991 approved these changes

@ljoss17 ljoss17 Awaiting requested review from ljoss17

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.6
4 participants
@romac @seanchen1991 @kaisbaccour @ancazamfir