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

CCIP-1277 LogPoller - Fixing leaky abstraction by removing Q() from the ORM interface #11200

Merged
merged 5 commits into from
Nov 13, 2023
Merged

CCIP-1277 LogPoller - Fixing leaky abstraction by removing Q() from the ORM interface #11200

merged 5 commits into from
Nov 13, 2023

Conversation

mateusz-sekara
Copy link
Collaborator

@mateusz-sekara mateusz-sekara commented Nov 7, 2023 •
edited
Loading

Moving all the internals related to opening TXs to the ORM layer. This has a couple of advantages:

  • Better boundaries between the domain logic and the storage
  • We can provide any ORM layer for persisting logs, bc we don't expose any DB details to the LogPoller and keep them encapsulated within ORM
  • We can cache the latest LogPollerBlock within the ORM layer. This could be a potential improvement by removing all nested queries from nestedBlockNumberQuery. The codebase will be much simpler
  • With caching the latest block we can enable LogPoller to allow any SQL queries

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2023

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 0.0% 0.0% Coverage on New Code (is less than 75%)

See analysis details on SonarQube

mateusz-sekara
mateusz-sekara commented Nov 7, 2023
View reviewed changes
core/chains/evm/logpoller/log_poller.go
err = lp.orm.Q().WithOpts(pg.WithParentCtx(ctx)).Transaction(func(tx pg.Queryer) error {
return lp.orm.InsertLogs(convertLogs(gethLogs, blocks, lp.lggr, lp.ec.ConfiguredChainID()), pg.WithQueryer(tx))
})
err = lp.orm.InsertLogs(convertLogs(gethLogs, blocks, lp.lggr, lp.ec.ConfiguredChainID()), pg.WithParentCtx(ctx))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a single query, we don't have to open TX for that

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, not sure how it got like that!

Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove this outer tx, we need to open a tx inside InsertLogs since we may break that into multiple inserts

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Yes, seems like that should have always been down in the ORM. Maybe at some point there was another ORM call inside the same tx?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is my fault; I've removed that TX prematurely. We can just open TX within the insertLogs to guarantee they are inserted together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kudos to @connorwstein for this finding!

mateusz-sekara
mateusz-sekara commented Nov 7, 2023
View reviewed changes
core/chains/evm/logpoller/orm.go Outdated
@@ -681,6 +698,20 @@ func (o *DbORM) SelectIndexedLogsWithSigsExcluding(sigA, sigB common.Hash, topic
return logs, nil
}

func (o *DbORM) InsertLogsWithBlock(logs []Log, block LogPollerBlock, qopts ...pg.QOpt) error {
// Optimization, don't open TX when there is only block to be persisted
if len(logs) == 0 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK, opening TX requires an additional round trip to the database. This could be a nice improvement for PollAndSave ticks when no logs are emitted

@mateusz-sekara mateusz-sekara changed the title LogPoller - Fixing leaky abstraction by removing Q() from the ORM interface CCIP-1277 LogPoller - Fixing leaky abstraction by removing Q() from the ORM interface Nov 7, 2023
@mateusz-sekara mateusz-sekara force-pushed the logpoller/moving-tx-to-orm branch 2 times, most recently from 30e055a to 5981c76 Compare November 8, 2023 13:59
@mateusz-sekara mateusz-sekara marked this pull request as ready for review November 8, 2023 14:00
@mateusz-sekara mateusz-sekara force-pushed the logpoller/moving-tx-to-orm branch from 7ae9d7f to 0023762 Compare November 8, 2023 15:04
@mateusz-sekara mateusz-sekara force-pushed the logpoller/moving-tx-to-orm branch from 0023762 to d14c4a0 Compare November 8, 2023 15:05
reductionista
reductionista previously approved these changes Nov 9, 2023
View reviewed changes
@mateusz-sekara mateusz-sekara added this pull request to the merge queue Nov 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 9, 2023
@mateusz-sekara mateusz-sekara added this pull request to the merge queue Nov 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 9, 2023
@mateusz-sekara mateusz-sekara added this pull request to the merge queue Nov 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 9, 2023
@mateusz-sekara mateusz-sekara force-pushed the logpoller/moving-tx-to-orm branch from a66a61d to 75b12e7 Compare November 13, 2023 07:27
@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.4% 80.4% Coverage
0.0% 0.0% Duplication

@mateusz-sekara mateusz-sekara added this pull request to the merge queue Nov 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 13, 2023
@mateusz-sekara mateusz-sekara added this pull request to the merge queue Nov 13, 2023
Merged via the queue into develop with commit f7981f5 Nov 13, 2023
86 checks passed
@mateusz-sekara mateusz-sekara deleted the logpoller/moving-tx-to-orm branch November 13, 2023 16:59
@mateusz-sekara mateusz-sekara mentioned this pull request Nov 14, 2023
Merged
@snehaagni snehaagni mentioned this pull request Jan 24, 2024
Merged
asoliman92 pushed a commit that referenced this pull request Jul 31, 2024
@mateusz-sekara
CCIP-1277 LogPoller - Fixing leaky abstraction by removing Q() from t…
0f9bf06 
…he ORM interface (#11200)

* Fixing leaky abstraction by removing Q() from the ORM interface. Moving all the TX internals into the ORM implementation

* Adding test

* Post review fix

* Post rebase fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connorwstein connorwstein connorwstein approved these changes

@reductionista reductionista Awaiting requested review from reductionista

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mateusz-sekara @reductionista @connorwstein