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

ledger-db restructuring #3441

Merged
merged 9 commits into from
Feb 1, 2023
Merged

ledger-db restructuring #3441

merged 9 commits into from
Feb 1, 2023

Conversation

Ben-PH
Copy link
Contributor

@Ben-PH Ben-PH commented Jan 18, 2023

  • document all added functions
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@Ben-PH Ben-PH changed the title Refactor code layout ledger-db refactor Jan 18, 2023
@Ben-PH
Copy link
Contributor Author

Ben-PH commented Jan 18, 2023

I've been looking into making the results of the getters return a byte-slice instead of a vec as well. The issue is that the library uses the DBPinnableSlice struct to manage the lifetime of the data-pinning. When it goes out of scope, a rocksdb defined ffi function is used to unpin (free up) the data. Fundamentally, the question seems to be this:

How to make the v-table containing the drop method available outside of massa-ledger-worker library, without making our other libraries also dependent on rocksdb (which leads to longer compile times. like, really long)

@Ben-PH Ben-PH requested a review from Eitu33 January 18, 2023 16:30
@Ben-PH Ben-PH force-pushed the ben_ledgerdb-refactor branch 2 times, most recently from d1d027e to 8db0a92 Compare January 19, 2023 12:21
@Ben-PH Ben-PH marked this pull request as ready for review January 19, 2023 13:08
@Eitu33 Eitu33 changed the title ledger-db refactor ledger-db restructuring Jan 20, 2023
@Eitu33
Copy link
Contributor

Eitu33 commented Jan 20, 2023

We usually employ "refactor" when dealing with major behavioral changes of a module, I changed the title to restructuring to avoid misleading anyone. Also I'm not a big fan of the ledger_db subdir, I don't feel there is a need for that as the module is entirely related to the the ledger_db file anyway, feels weird to have a file and a directory with the same name too, I'd rather we keep everything at the root. Otherwise lgtm.

@Ben-PH
Copy link
Contributor Author

Ben-PH commented Jan 20, 2023

I'll revert file separation, then merge on monday.

I'll also raise a discussion in github about that (I personally prefer not to have mod.rs files, but I'll raise it in the discussion)

massa-ledger-worker/src/ledger_db.rs Outdated Show resolved Hide resolved
massa-ledger-worker/src/ledger_db.rs Outdated Show resolved Hide resolved
@Ben-PH Ben-PH merged commit fad42ae into testnet_19 Feb 1, 2023
@AurelienFT AurelienFT deleted the ben_ledgerdb-refactor branch May 29, 2023 08:14
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.

2 participants