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

Implement the LID #71

Merged
merged 21 commits into from
Jun 18, 2024
Merged

Implement the LID #71

merged 21 commits into from
Jun 18, 2024

Conversation

jmg-duarte
Copy link
Collaborator

@jmg-duarte jmg-duarte commented Jun 13, 2024

Description

This PR implements the LID component (currently, without the JSON-RPC) which supersedes the piecestore.

Fixes #33

Important points for reviewers

I've "over documented" the code with references back to the original code to shorten the time it takes for one to learn about the details and reasons behind certain choices being made.

There are some places were I've deviated from the original implementation, but hopefully those are sensible enough.

Checklist

  • Are there important points that reviewers should know?
  • Make sure that you described what this change does.
  • If there are follow-ups, have you created issues for them?
    • If yes, which ones? / List them here
  • Have you tested this solution?
  • Were there any alternative implementations considered?
  • Did you document new (or modified) APIs?

Copy link
Contributor

@th7nder th7nder 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 which parts were already there, which are new and what was changed. I was trying to read it all, but gave up in the middle of rdb.rs.
Should we separate the implementation of the Service trait somewhere, so we know what to look at? Maybe leave piecestore by and only add the Service trait, have the refactor in a separate PR?
As well, are the tests covering the new Service trait, or are they old ones?
Should we add some kind of integration tests?

By the way, it looks like a huuuge piece of work with a lots of quirks and twists that you needed to parse and implement.
It's a pleasure to try to understand and read engineering done like that!

@jmg-duarte jmg-duarte force-pushed the feat/24/boostd-data branch from 51cc15b to 4b03270 Compare June 17, 2024 16:32
@jmg-duarte jmg-duarte force-pushed the feat/24/boostd-data branch from 4b03270 to f501816 Compare June 17, 2024 16:35
@jmg-duarte jmg-duarte marked this pull request as ready for review June 17, 2024 16:36
@jmg-duarte jmg-duarte added the ready for review Review is needed label Jun 17, 2024
th7nder
th7nder previously approved these changes Jun 18, 2024
Copy link
Contributor

@th7nder th7nder left a comment

Choose a reason for hiding this comment

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

Hell of a job!
We established that integration tests will come later, when we add the RPC layer.
I digged through all of the code, don't see anything worth debating now.
If we see something wrong in the upper layers, then we'll fix it.
Let's go!

@jmg-duarte jmg-duarte added bug Something isn't working ready for review Review is needed and removed ready for review Review is needed bug Something isn't working labels Jun 18, 2024
@jmg-duarte jmg-duarte added ready for review Review is needed and removed ready for review Review is needed labels Jun 18, 2024
Copy link
Member

@cernicc cernicc left a comment

Choose a reason for hiding this comment

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

Great work 💪

Copy link
Contributor

@serg-temchenko serg-temchenko left a comment

Choose a reason for hiding this comment

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

Looks great! 🔥 💪

@jmg-duarte jmg-duarte merged commit 88c6662 into develop Jun 18, 2024
3 checks passed
@jmg-duarte jmg-duarte deleted the feat/24/boostd-data branch June 18, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Review is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexing component implementation
4 participants