-
Notifications
You must be signed in to change notification settings - Fork 408
feat(gnodev): cache path and notify conflict path #4298
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
base: master
Are you sure you want to change the base?
feat(gnodev): cache path and notify conflict path #4298
Conversation
🛠 PR Checks Summary🔴 Maintainers must be able to edit this pull request (more info) Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🔴 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
…oanvarmeta/feat-gnodev
…A-Tech/gno into 0xhoanvarmeta/feat-gnodev
Hi, could you please resolve the CI errors (except merge requirements) to proceed the review? thank you |
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.
Is a cache clearing function unnecessary? it doesn't seem like there's any part that handles cache invalidation.
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 think the cache clearing function is unnecessary, because once a package is added using gnokey addpkg, it cannot be reloaded. Moreover, the cache is reset whenever gnodev is restarted
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.
sounds resonable 👍
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
remove: review/triage-pending flag
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.
see comments
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.
please remove this package; a simple sync map in this scenario doesn't deserve its own package as it's not shared among other packages or even other structs. This cache map should exist inside the dev.Node
struct.
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.
Can you add some pertinent tests in node_test
?
for _, pkg := range pkgs { | ||
if !cachepath.Get(pkg.Path) { | ||
filterPackages = append(filterPackages, pkg) | ||
} else { | ||
n.logger.Error("Can't reload package, package conflict in ", "path", pkg.Path) | ||
} | ||
} | ||
|
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.
no need to allocate another array here, you should filter this directly in generateTx
feat(gnodev) cache path and notify conflict path
It resolves issue #4197
I continue to work #4285 in here
Cache a list of added AddPkg messages from state node.
Don't reload conflicting paths and log error