-
Notifications
You must be signed in to change notification settings - Fork 585
Caqti upgrade with fix for rosetta #16704
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
Conversation
|
!ci-build-me |
5c03c4b to
0dbf742
Compare
|
!ci-build-me |
|
!ci-build-me |
|
!ci-nightly-me |
|
!ci-toolchain-me |
|
!ci-build-me |
|
!ci-toolchain-me |
|
Is this PR still relevant? |
|
!ci-build-me |
2b3c5fd to
f73f6ea
Compare
|
!ci-toolchain-me |
f73f6ea to
1494fa9
Compare
|
!ci-build-me |
2 similar comments
|
!ci-build-me |
|
!ci-build-me |
|
!ci-nightly-me |
|
!ci-build-me |
e206f8c to
03db4c2
Compare
|
!ci-build-me |
|
!ci-nigthly-me |
9901c66 to
f7ec359
Compare
|
!ci-build-me |
|
!ci-build-me |
…y always casting field to its type when checking if null
ba454bc to
2803494
Compare
|
Although I'm not requested to review this: This is too big, is it possible to break it down to several smaller PRs? |
|
@glyh I thought about it, but it is a library bump PR so it is hard/artificial to do it partially |
|
|
||
| echo "Running archive node test" | ||
| echo "WARN: Silencing the errors from the archive node test app. One of the tests is expected to fail." | ||
| MINA_TEST_POSTGRES_URI=$MINA_TEST_POSTGRES_URI $ARCHIVE_TEST_APP || true |
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.
In a follow-up PR let's remove the line:
echo "WARN: Silencing the errors from the archive node test app. One of the tests is expected to fail."|
!ci-build-me |
SanabriaRusso
left a comment
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 on infra side. Updated toolchains and updated scripts.
Reimplementing caqti upgrade after revertion (#16694).
PR Structure
Previous PR was reverted as it introduced regression in rosetta. Lesson was learnt and we added more E2E tests for rosetta which now will catch it (#16692). What is more, we stabilized tests here (#16695).
Regarding source of the issue:
After caqti update postgres postgres lost information about type of parameter in some cases. Especially in where clause in search command:
Which resulted in postgres error
Solution
The possible solution is to always add
::{type}suffix to parameter when using it. For example $1::int or $2::text. For this purpose is modified a bit code responsible for formatting parameter and always use ::{type} suffix for checking for null and CAST when comparing with expected valueHow it is tested:
Currently All known rosetta tests are automated in CI:
We added load test to measure memory gain:
We added new test coverage as well for archive node:
Performance improvements metrics
Archive bootstrap test is used as a load test since it try to insert mainnet ledger to archive db in one go.
Rosetta load test fires steady load and measure postgres memory
Running load tests on orchestrator env using rosetta & archive is now possible, thanks to caqti upgrade. Before archive couldn't bootstrap with such big genesis ledger