-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fix/meta/validate objects #3219
Conversation
It has a starting log and a finishing one, but no records in logs if notifications are missing. Signed-off-by: Pavel Karpy <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3219 +/- ##
==========================================
+ Coverage 22.16% 22.20% +0.04%
==========================================
Files 761 761
Lines 60745 60817 +72
==========================================
+ Hits 13463 13506 +43
- Misses 46299 46328 +29
Partials 983 983 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
defer wg.Done() | ||
err := s.putMPTIndexes(bInd, objCh) | ||
if err != nil { | ||
l.Error("failed to put mpt indexes", zap.Error(err)) |
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.
not all errors deserve the highest severity imo. For example, failed HEAD is ptetty usual. info
is enough to me, warn
at most
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.
failed HEAD is ptetty usual
we need to solve it somehow, because we are on our way, when it is not true. if MPT is outdated (any missing object makes it so, cause the root hash differs then), node is not trusted and it should be resynced or fixed, or smth
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.
so, when we'll see this error, we are supposed to do some particular actions? If so, then its ok to write in error
severity, but lets write what todo. failed ..., DB resync is needed
for example
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.
still think it is really close to #3140. we need to decide in general what is a missing object for us. i see this issue as the point when we are deciding it
b6e0633
to
7e918e3
Compare
Fixed tests with empty metaservice path. Hardcoded fake path, but what else can we do now... |
Signed-off-by: Pavel Karpy <[email protected]>
Required to overwrite empty config values for current test purposes. Other engine components require (panic otherwise) paths, but meta is a WIP feature, so no strict requirements for now. Signed-off-by: Pavel Karpy <[email protected]>
395ba6e
to
390645d
Compare
Tests are failing though. |
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.
make sure this does not need the changelog
At this stage this requires enabling on IR side, so it's all experimental, nothing to log. |
* add new "locked by" index to raw indexes * make MPT keys be placed only after they are put to DB (and validated) * prohibit removal of non-existent objects * prohibit removal of locked objects * prohibit locking of non-existent objects Closes #3178. Signed-off-by: Pavel Karpy <[email protected]>
390645d
to
d81f7d9
Compare
Yes, I haven't added any records to CHANGELOG about meta yet. |
No description provided.