Skip to content
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

UPGRADE: Fix multi-use invitation performance #2116

Merged

Conversation

KimEbert42
Copy link
Contributor

Multiuse invitation performance degrades over time. By adding state into the tag names, performance doesn't degrade over time while using multi-use invitations.

This breaks backwards compatibility, but while load testing it can be seen that performance no longer degrades when using a multi-use invitation. Previously, only 1600 connections could be established while load testing, and on a second run is reduced to 300. After the modification, the load testing exceeded 2400 connections and the testing client machine ran out of ram for clients.

This shows a clear improvement in performance.

@KimEbert42 KimEbert42 changed the title Feature/fix record loading size Fix multi-use invitation performance Feb 11, 2023
@ianco
Copy link
Contributor

ianco commented Feb 13, 2023

We can add these new tags on startup. I don't recall the details but @shaangill025 had to do this at one point

@dbluhm
Copy link
Contributor

dbluhm commented Feb 14, 2023

@ianco are you referring to the upgrade command or is there another mechanism for adding the new tags?

@ianco
Copy link
Contributor

ianco commented Feb 14, 2023

@ianco are you referring to the upgrade command or is there another mechanism for adding the new tags?

Yes the upgrade command, that's right.

@cvarjao cvarjao mentioned this pull request Feb 14, 2023
4 tasks
@KimEbert42
Copy link
Contributor Author

Add upgrade command to upgrade connections.

@KimEbert42 KimEbert42 marked this pull request as ready for review February 15, 2023 22:20
@ianco
Copy link
Contributor

ianco commented Feb 16, 2023

@reflectivedevelopment this looks good! I'm just doing some testing ... can you sync up with the latest main branch?

@ianco ianco enabled auto-merge February 16, 2023 15:56
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ianco ianco merged commit a033cd0 into openwallet-foundation:main Feb 17, 2023
@ianco ianco changed the title Fix multi-use invitation performance UPGRADE: Fix multi-use invitation performance Feb 17, 2023
@ianco
Copy link
Contributor

ianco commented Feb 17, 2023

Added "UPGRADE" to the PR title so we remember to add instructions re upgrading the db to the release docs.

@swcurran
Copy link
Contributor

@ianco or @reflectivedevelopment — can one of you add the details of what has to be done to upgrade that I can put into the ChangeLog. Will it happen automagically, or does something have to be done by the deployer to trigger the upgrade? I’m not clear on exactly what is happening — there isn’t a lot of code changing in the PR :-)

@ianco
Copy link
Contributor

ianco commented Feb 17, 2023

I think you just need to run aca-py upgrade but @reflectivedevelopment can confirm what are the details

@KimEbert42
Copy link
Contributor Author

KimEbert42 commented Feb 17, 2023

It is the aca-py upgrade command that needs to be run.

Here is the command I used to run the upgrade.

aca-py upgrade --wallet-type askar --wallet-storage-type postgres_storage --from-version=v0.8.0 --upgrade-config-path ./upgrade.yml

The other wallet values were include in my environment

@swcurran swcurran mentioned this pull request Mar 15, 2023
@swcurran
Copy link
Contributor

Shouldn’t the —from-version be something earlier than v0.8.0, since that is the version that has this fix?

@swcurran
Copy link
Contributor

@reflectivedevelopment -- I added the following to the CHANGELOG.md -- does this seem right:

The way that multiuse invitations in previous versions of ACA-Py caused
performance to degrade over time. An update was made to add state into the tag
names that eliminated the need to scan the tags when querying storage for the
invitation.

If you are using multiuse invitations in your existing (pre-0.8.0 deployment
of ACA-Py, you can run an upgrade to apply this change. To run upgrade from
previous versions, use the following command using the 0.8.0 version of
ACA-Py, adding you wallet settings:

aca-py upgrade <other wallet config settings> --from-version=v0.7.5 --upgrade-config-path ./upgrade.yml

@KimEbert42
Copy link
Contributor Author

@swcurran this seems okay.

@swcurran
Copy link
Contributor

Awesome — thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants