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

New subscription manager #1071

Merged
merged 5 commits into from Jan 8, 2024
Merged

New subscription manager #1071

merged 5 commits into from Jan 8, 2024

Conversation

cindyyan317
Copy link
Collaborator

SubscriptionManager is puffy now, containing all the subscribers management and feed generation work. It is hard to implement api_version up to it.
This PR will:
1 Separate each stream to the different feed class for better maintainability and testability, each feed focus on its own feed generation.
2 SubscriptionManager is a dispatcher class now. The API keeps the same as before, except additional api_version parameter for transaction stream.
-RPC engine and web server remove the SubscriptionManager dependency.
-Executor is injected to SubscriptionManager, remove the waiting from unittests code.
3 Utilize boost/signal2 for sub/unsub/pub
4 Implement api_version
5 Remove cleanup from SubscriptionManager, the subscription will be removed as the web session gone.
6 Keep the original log + log when session removed to better track lifespan
7 Fix issue: Duplicated feed sent if multiple watched accounts get affected in same tx.

@cindyyan317 cindyyan317 requested review from godexsoft and kuznetsss and removed request for godexsoft January 3, 2024 10:50
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 83 lines in your changes are missing coverage. Please review.

Comparison is base (07bd4b0) 53.68% compared to head (ecd74d4) 54.09%.

Files Patch % Lines
src/feed/SubscriptionManager.h 12.12% 15 Missing and 14 partials ⚠️
src/feed/impl/TransactionFeed.cpp 89.26% 0 Missing and 16 partials ⚠️
src/feed/impl/ProposedTransactionFeed.cpp 85.24% 1 Missing and 8 partials ⚠️
src/rpc/handlers/Subscribe.h 22.22% 0 Missing and 7 partials ⚠️
src/main/Main.cpp 0.00% 4 Missing ⚠️
src/feed/impl/ProposedTransactionFeed.h 40.00% 0 Missing and 3 partials ⚠️
src/feed/impl/SingleFeedBase.cpp 90.62% 0 Missing and 3 partials ⚠️
src/feed/impl/TransactionFeed.h 66.66% 0 Missing and 3 partials ⚠️
src/feed/impl/BookChangesFeed.h 60.00% 0 Missing and 2 partials ⚠️
src/feed/impl/Util.h 60.00% 0 Missing and 2 partials ⚠️
... and 5 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1071      +/-   ##
===========================================
+ Coverage    53.68%   54.09%   +0.40%     
===========================================
  Files          191      203      +12     
  Lines        10150    10286     +136     
  Branches      5417     5452      +35     
===========================================
+ Hits          5449     5564     +115     
- Misses        3390     3399       +9     
- Partials      1311     1323      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@kuznetsss kuznetsss left a comment

Choose a reason for hiding this comment

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

It is a really good imrovement, thanks! Just a few comments.

src/feed/Types.h Outdated Show resolved Hide resolved
src/main/Main.cpp Outdated Show resolved Hide resolved
src/web/interface/ConnectionBase.h Show resolved Hide resolved
src/web/interface/ConnectionBase.h Show resolved Hide resolved
src/feed/SubscriptionManager.h Outdated Show resolved Hide resolved
src/feed/impl/ProposedTransactionFeed.cpp Outdated Show resolved Hide resolved
src/feed/impl/ProposedTransactionFeed.cpp Outdated Show resolved Hide resolved
src/feed/impl/TransactionFeed.h Outdated Show resolved Hide resolved
unittests/feed/LedgerFeedTests.cpp Outdated Show resolved Hide resolved
unittests/feed/ProposedTransactionFeedTests.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@kuznetsss kuznetsss left a comment

Choose a reason for hiding this comment

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

It is a really good imrovement, thanks! Just a few comments.

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

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

Great job - huge improvement over the old version 🎉
I'm adding mostly minor comments 🔢
Also, as usual, JSON formatting can be improved in tests 🥇

src/feed/Types.h Outdated Show resolved Hide resolved
src/feed/impl/BookChangesFeed.h Show resolved Hide resolved
src/feed/impl/BookChangesFeed.h Outdated Show resolved Hide resolved
src/feed/impl/ForwardFeed.h Outdated Show resolved Hide resolved
src/feed/impl/LedgerFeed.h Show resolved Hide resolved
src/feed/impl/TrackableSignalMap.h Outdated Show resolved Hide resolved
src/feed/impl/TrackableSignalMap.h Show resolved Hide resolved
src/feed/impl/TransactionFeed.h Show resolved Hide resolved
src/feed/SubscriptionManager.h Outdated Show resolved Hide resolved
src/main/Main.cpp Outdated Show resolved Hide resolved
src/feed/Types.h Outdated Show resolved Hide resolved
src/feed/impl/ForwardFeed.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@kuznetsss kuznetsss left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

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

Looks good, even though not all suggestions got implemented in the end, i think this is so much better than what we started with that we should merge it now and improve further later if we so desire.

@cindyyan317 cindyyan317 merged commit eb1831c into XRPLF:develop Jan 8, 2024
8 checks passed
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.

None yet

3 participants