-
Notifications
You must be signed in to change notification settings - Fork 46
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
Reorder migrations after merging latest master #248
Reorder migrations after merging latest master #248
Conversation
|
||
func init() { | ||
up := batch(` | ||
DROP MATERIALIZED VIEW IF EXISTS public.chain_visualizer_chain_data_view; -- artifact from chainwatch |
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 view is being used by an external project and is removed in the hotfix's v17 migration. Removing this to prevent an error here.
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 view depends on some of the tables being dropped below:
https://github.com/filecoin-project/sentinel-visor/blob/master/storage/migrations/7_chainvis_views.go#L35
https://github.com/filecoin-project/sentinel-visor/blob/master/storage/migrations/7_chainvis_views.go#L37
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 view doesn't add any value to visor directly. Is there any reason the external project can't make a similar view to meet their needs? I don't see why this SQL needs to live here.
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.
The view supports this tool. If we are dubious about its value, I'd like to talk that through first, but maybe not as part of this PR.
Agree that the view needs to be corrected for the dropped tables. Will fix that.
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.
An alternative to fixing it here could be updating the view here: https://github.com/DigitalMOB2/filecoin-lotus-explorer-playground/blob/master/2.create-views.sql#L1. But happy to go with either option.
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 don't intend to block this change from landing in the release branch, but I don't see how this will make it into master, let's discuss when we reach that point.
aa859af
to
ab86476
Compare
LEFT JOIN | ||
block_headers parent_block ON parent_block.cid = bp.parent | ||
LEFT JOIN | ||
blocks_synced synced ON synced.cid = main_block.cid |
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 believe this table has been removed.
ab86476
to
807e651
Compare
* origin/master: fix: Remove hack to RestartOnFailure fix: Reorder migrations after merging latest master (#248) feat: add repo-read-only flag to enable read or write on lotus repo (#250) chore: Update CHANGELOG chore: Incl migration in CI test fix: Make ChainVis into basic views failure to get lock when ExitOnFailure is true now exits chore: remove redundant branch image push dont use branch name when tag is set remove negation use git tag in docker image Adds an option to exit on failure. (#202)
Just want to be explicit about the changes I'm making to make latest hotfix migrations agree with what is already deployed to
master
.NOTE: This will FBUAR anyone's visor database who has already migrated up to v17.