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 events table migration #262

Merged
merged 27 commits into from
Aug 16, 2024
Merged

Add events table migration #262

merged 27 commits into from
Aug 16, 2024

Conversation

aditya1702
Copy link
Contributor

@aditya1702 aditya1702 commented Aug 8, 2024 •
edited
Loading

What

Closes #257

  • Add migration for events table.
  • Move db transaction logic for migration out of guardedMigration.

Why

  • We are moving events from in-memory to db, hence the migration support.
  • Sqlite DB cannot handle two concurrent db transactions, so we need to have a common transaction for both migrations instead of individual ones. Having per migration db tx leads to database is locked error.

Known limitations

NA

@aditya1702 aditya1702 changed the base branch from main to refactor-get-events August 8, 2024 18:07
@psheth9 psheth9 mentioned this pull request Aug 8, 2024
Merged
@aditya1702 aditya1702 changed the title [WIP] Make migrations sequential [WIP] Add events table migration Aug 9, 2024
@aditya1702 aditya1702 changed the title [WIP] Add events table migration Add events table migration Aug 12, 2024
@aditya1702 aditya1702 marked this pull request as ready for review August 12, 2024 14:25
@aditya1702 aditya1702 requested a review from tamirms August 13, 2024 17:52
@psheth9
Copy link
Contributor

psheth9 commented Aug 13, 2024

Looks good to me !! @2opremio can you review it too as you had few initial comments on migration logic

@2opremio
Copy link
Contributor

Can you add a test for the migrations? (both at once to make sure there are no transaction conflicts when applying multiple migrations at once)

@aditya1702
Copy link
Contributor Author

Can you add a test for the migrations? (both at once to make sure there are no transaction conflicts when applying multiple migrations at once)

@2opremio Doesnt the current TestMigrate function already test that? Doing both transactions and events migrations together

@aditya1702 aditya1702 requested a review from 2opremio August 15, 2024 12:51
@2opremio
Copy link
Contributor

2opremio commented Aug 16, 2024 •
edited
Loading

@aditya1702 you are right!!

@aditya1702 aditya1702 merged commit 7466e54 into stellar:refactor-get-events Aug 16, 2024
17 checks passed
psheth9 added a commit that referenced this pull request Aug 20, 2024
@psheth9 @aditya1702
Add getEvents backed by DB (#215)
a7e6459 
* Ingest events into DB

* Update tests

* Update tests

* Fix tests

* Ignore ingestion when events are empty

* Add getEvents backed by DB

* Refactor getEvents to call fetch events from db

* Remove in-memory events store

* Make use of cursor in pagination and combine filters

* Update scan function logic in order to test

* Move NewTestDb util

* Trim events db and remove eventTypes in SELECT query as event types are not indexed

* Introduce cursor range and add logs for latency

* remove event memory store

* remove event memory store from latest merge

* Fix lint issues part 1

* Fix lint issues part 2

* Fix more lint and add ledger range code for events

* Update eventHandler mock

* Fix 2 major errors in tests: nil pointer reference and unknown hash

* Fix contract Id filter logic and add cursor set to avoid duplicates

* Validate requested start ledger with stored ledger range

* Fix lint error part 4

* Add migration for events table

* Fix lint error part 5

* Fix lint error part 6

* Address review comments pt1

* Make contract id a blob type

* Remove events package and move cursor.go to db package

* Fix lint error part 6

* Fix lint error part 7

* Optimize migration code

* Introduce endLedger and remove Cursor id from schema

* Use LedgerReader to get Ledger Range in events

* Fix lint errors

* Add benchmark for testing various load parameter

* update benchmark

* Comment benchmark events

* Reduce allocs pt1

* Benchmark with 30 million events

* Refactor getEvents to backed only by Events table

* change test db path

* Use Binary encoding for saving events into DB

* Correct number of topics

* reduce events in benchmark so that tests run

* update events schema

* Fix topic count

* Fix fetch query to not stop if null topic

* Fix lint pt1

* Fix trimEvents and lint errors

* update log info

* Fix more lint errors

* Fix more lint errors pt 11

* Fix format in error

* Fix linter error pt 12

* Add nolint for GetEvents as a temp fix.

* Add events table migration (#262)

* Fix migrations - 1

* Make migrations sequential - 1

* Make migrations sequential - 2

* Fix failing unittest - 1

* Fix linting errors - 1

* Fix failing integration test - 1

* Remove %w from Fatal strings

* refactor migrationApplierFactoryF

* Add ledger seq to fatal error string

* Add comments - 1

* Fix - 1

* Optimise migrations - 1

* Optimise migrations - 2

* Optimise migrations - 3

* Fix linting - 1

* Fix linting - 2

* Remove dupicate latest ledger fetch code

* Rollback db in daemon instead of migration

* Remove unused constant

* Remove unused constant - 2

* Add rollback() statement

* Small change

* Abstract transaction and rollback management inside migration code

* Fix failing unittest

* Address review comments

* Store binary of topics instead of string

* Unify min/max topic count in event.go

* Address review comments for event Types and fix unit tests

* cleanup

* Fix linter errors for one last time

---------

Co-authored-by: Aditya Vyas <[email protected]>
@psheth9 psheth9 mentioned this pull request Aug 20, 2024
Merged
psheth9 added a commit that referenced this pull request Aug 24, 2024
@psheth9 @aditya1702
Events db backend (#230)
dbad81b 
* Ingest events into DB (#192)

* Ingest events into DB

* Update tests

* Update tests

* Fix tests

* Ignore ingestion when events are empty

* Add getEvents backed by DB  (#215)

* Ingest events into DB

* Update tests

* Update tests

* Fix tests

* Ignore ingestion when events are empty

* Add getEvents backed by DB

* Refactor getEvents to call fetch events from db

* Remove in-memory events store

* Make use of cursor in pagination and combine filters

* Update scan function logic in order to test

* Move NewTestDb util

* Trim events db and remove eventTypes in SELECT query as event types are not indexed

* Introduce cursor range and add logs for latency

* remove event memory store

* remove event memory store from latest merge

* Fix lint issues part 1

* Fix lint issues part 2

* Fix more lint and add ledger range code for events

* Update eventHandler mock

* Fix 2 major errors in tests: nil pointer reference and unknown hash

* Fix contract Id filter logic and add cursor set to avoid duplicates

* Validate requested start ledger with stored ledger range

* Fix lint error part 4

* Add migration for events table

* Fix lint error part 5

* Fix lint error part 6

* Address review comments pt1

* Make contract id a blob type

* Remove events package and move cursor.go to db package

* Fix lint error part 6

* Fix lint error part 7

* Optimize migration code

* Introduce endLedger and remove Cursor id from schema

* Use LedgerReader to get Ledger Range in events

* Fix lint errors

* Add benchmark for testing various load parameter

* update benchmark

* Comment benchmark events

* Reduce allocs pt1

* Benchmark with 30 million events

* Refactor getEvents to backed only by Events table

* change test db path

* Use Binary encoding for saving events into DB

* Correct number of topics

* reduce events in benchmark so that tests run

* update events schema

* Fix topic count

* Fix fetch query to not stop if null topic

* Fix lint pt1

* Fix trimEvents and lint errors

* update log info

* Fix more lint errors

* Fix more lint errors pt 11

* Fix format in error

* Fix linter error pt 12

* Add nolint for GetEvents as a temp fix.

* Add events table migration (#262)

* Fix migrations - 1

* Make migrations sequential - 1

* Make migrations sequential - 2

* Fix failing unittest - 1

* Fix linting errors - 1

* Fix failing integration test - 1

* Remove %w from Fatal strings

* refactor migrationApplierFactoryF

* Add ledger seq to fatal error string

* Add comments - 1

* Fix - 1

* Optimise migrations - 1

* Optimise migrations - 2

* Optimise migrations - 3

* Fix linting - 1

* Fix linting - 2

* Remove dupicate latest ledger fetch code

* Rollback db in daemon instead of migration

* Remove unused constant

* Remove unused constant - 2

* Add rollback() statement

* Small change

* Abstract transaction and rollback management inside migration code

* Fix failing unittest

* Address review comments

* Store binary of topics instead of string

* Unify min/max topic count in event.go

* Address review comments for event Types and fix unit tests

* cleanup

* Fix linter errors for one last time

---------

Co-authored-by: Aditya Vyas <[email protected]>

* Fix linter errors pt 1

* Add nolint for InsertEvents

* Add nolint for InsertEvents again

* Address review comments

* Fix lint

* Fix lint pt1

* Fix lint pt2

* Fix index out of bound error and fix logging

* Add changelog entry

---------

Co-authored-by: Aditya Vyas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@2opremio 2opremio 2opremio approved these changes

@psheth9 psheth9 Awaiting requested review from psheth9

@Shaptic Shaptic Awaiting requested review from Shaptic

@tamirms tamirms Awaiting requested review from tamirms

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

Successfully merging this pull request may close these issues.
4 participants
@aditya1702 @psheth9 @2opremio @tamirms