Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rerun conditional migrations #6565
Rerun conditional migrations #6565
Changes from 9 commits
102dff1
ae2ad08
901eb32
bc8af67
ae0ac55
0388007
488f89c
30aa94a
9580a62
ea2991a
7f575b9
2c6e69e
8888db3
305bc8b
0653952
e35f1ba
79feac5
cd3a698
606334b
8806622
392813e
fc5d337
4f5ccd7
32789d7
20d71a2
1143306
f11db71
8a2340a
c850cb4
dbf7c61
49463bb
8879cb5
22a330e
98b0d70
3d03c94
1dcd887
99f8f68
d701633
42e323a
b164527
4e9737a
63c9f10
15e19dc
ed59c5f
4bd8e6e
e71e85b
1ac1042
97a1395
e127396
45702ab
c4acb56
180015f
c393971
604bd3e
4e752d9
00815e1
6e8ee7a
e7ddc3a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 migration took 779 ms on mainnet.So not converting it to Async.
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 migration took 10158 ms on mainnet.So not converting it to Async.
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 doubt it can work in environment where the owner and regular user are different, e.g.,
mirror_node
andmirror_importer
.The migration uses the owner
jdbcTemplate
bean because it deletes rows. in some environment,mirror_importer
may not be able to delete rows.When the migration runs inside
onEnd
, it's in the same database transaction, and theconnection
in the thread local storage uses regular usermirror_importer
. The migration most likely uses the same connection, and may fail to delete rows.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.
did you check if running it inside
onEnd
would actually pick a connection with different credentials other than owner?this is a blocker for both
SyntheticNftAllowanceOwnerMigration
andSyntheticTokenAllowanceOwnerMigration
No I did not. Checking now.
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 I did not. Will need to check this. looking now.
Even if it is a different user i.e. mirror_importer, I need to verify that it can delete the rows right?
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 can discuss. running with
mirror_importer
may not break ourselves but can break other mirrornode operators in case in their environmentmirror_importer
doesn't have permission to delete rows.then we may have to turn these two migrations to be async.
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.
Ok. Lets discuss further offline.
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 verified that even when migrate is called from with the onEnd it uses the mirror_node user.
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.
Moved this into another function to reduce cognitive complexity.
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.
If this is the first account balance file being processed and the DB is empty then publishing InitializeBalanceEvent which will rerun balance migrations.