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

migrate link store #91

Merged
merged 3 commits into from
Nov 26, 2024
Merged

migrate link store #91

merged 3 commits into from
Nov 26, 2024

Conversation

aditiharini
Copy link
Contributor

@aditiharini aditiharini commented Nov 26, 2024

Support link messages in the engine.

@aditiharini aditiharini marked this pull request as ready for review November 26, 2024 22:09
@aditiharini aditiharini merged commit ca712e5 into main Nov 26, 2024
2 checks passed
@aditiharini aditiharini deleted the migrate-stores branch November 26, 2024 23:12
let prefix: Vec<u8> = LinkStore::links_by_target_key(target, 0, None)?;
let start_prefix: Vec<u8> = LinkStore::links_by_target_key(target, 0, None)?;
let stop_target = match target {
Target::TargetFid(fid) => Target::TargetFid(fid + 1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use increment_vec_u8, which is more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually preferred this method. With increment_vec_u8 you need to be sure that the key you're incrementing is the last part of the prefix otherwise it won't do what you want. Here, I actually had increment_vec_u8 first but then checked the key structure and saw that the target fid is somewhere in the middle of the prefix so I changed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually just made the change you suggested in my next feature. In that one, there's some string field we index on and it was hard to figure out how to increment that. I just changed this to be consistent too.

let next_page_token = if last_key.len() > 0 {
Some(last_key[prefix.len()..].to_vec())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changed at the db level when I pulled the db code in. The page token is the whole key and not a piece of it. Only providing the suffix in the token worked when we only iterated over a single prefix (e.g. fid) but now that we have a start and stop we can't keep only some suffix.

pub fn get_link_removes_by_fid(
store: &Store,
store: &Store<LinkStore>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did these change? Not a big deal, but we should keep it consistent with hubs if possible. The CastStore uses just Store

Copy link
Contributor Author

@aditiharini aditiharini Nov 27, 2024

Choose a reason for hiding this comment

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

I changed the cast store too earlier. The reason i did this is because deriving works better with the generic. I wanted to derive Clone and couldn't do it with the version that existed before because of the way we embedded the store with Box and dyn.

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