-
Notifications
You must be signed in to change notification settings - Fork 8
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
[MET-68] Skip indexing if on chain data is missing and add metadata p… #40
Conversation
…resent bool to indexes
rocks-db/src/column.rs
Outdated
|
||
pub fn key_exist(&self, key: C::KeyType) -> bool { | ||
self.backend | ||
.key_may_exist_cf(self.handle(), C::encode_key(key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we happy using a bloom filter we have not configured for a column? Is it ok to have false positives here? If both answers are positive - the method should be renamed. If at least one is negative - we should use another method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes the indexing. If the dynamic data is missing for an asset, or it's present, but the on-chain data is missing from it - the asset is not indexed, until both the dynamic data and the on-chain data in it are available.
It's always a great idea to add some PR description so the reader doesn't have to guess, whether the logic change is intended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_grouping will not be able to use the index
ast_metadata_present is not set in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pay closer attention to conditional indexes and on data being persisted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will it work in case when we got an asset update, synchroniser put that data to the Postgre and set ast_metadata_present as false because json_downloader did not download it yet. Then after few minutes json_downloader downloads JSON and saves it to the Rocks. Will synchroniser update ast_metadata_present value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
nft_ingester/src/db_v2.rs
Outdated
pub status: TaskStatus, | ||
pub attempts: i16, | ||
pub max_attempts: i16, | ||
} | ||
|
||
pub struct UpdatedTask { | ||
pub status: TaskStatus, | ||
pub metadata_url_key: i64, | ||
pub metadata_url_key: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Url, right? let's rename it, as it's getting confusing reading those mappings
CREATE TABLE metadata ( | ||
mtd_id bigserial | ||
CONSTRAINT metadata_pk | ||
CREATE TABLE tasks ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced a meaningful name for it is tasks. metadata seems to be be easier to understand in this context.
…resent bool to indexes