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

CORE-18961: Up flow sandbox cache size to 20 #1403

Closed
wants to merge 4 commits into from

Conversation

vkolomeyko
Copy link
Contributor

@vkolomeyko vkolomeyko commented Dec 13, 2023

Having a current limit of 10 may too low to allow E2E tests to pass given that we have a number of vNodes running flows concurrently during E2E tests execution. With current limit of 10, sandbox context for the currently running flow may be closed, resulting bundle de-activation and ClassNotFoundException on the initiated flow execution.

Tried with: corda/corda-runtime-os#5260

Proven to have a positive effect on E2E run: https://ci02.dev.r3.com/job/Corda5/job/corda5-e2e-tests/job/PR-369/13/

@corda corda locked as off-topic and limited conversation to collaborators Dec 13, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR title failed to match regex -> ^((CORDA|EG|ENT|INFRA|CORE|DOC|ES|DA5)-\d+)(.*)

@corda corda unlocked this conversation Dec 13, 2023
@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Dec 13, 2023

Jenkins build for PR 1403 build 4

Build Successful:
Jar artifact version produced by this PR: 5.2.0.17-alpha-1702890529531

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR title failed to match regex -> ^((CORDA|EG|ENT|INFRA|CORE|DOC|ES|DA5)-\d+)(.*)

@vkolomeyko vkolomeyko self-assigned this Dec 13, 2023
@vkolomeyko vkolomeyko changed the title Up flow sandbox cache size to 20 CORE-18961: Up flow sandbox cache size to 20 Dec 14, 2023
@github-actions github-actions bot dismissed stale reviews from themself December 14, 2023 16:59

All good!

@vkolomeyko vkolomeyko requested a review from a team December 14, 2023 16:59
@vkolomeyko vkolomeyko marked this pull request as ready for review December 14, 2023 16:59
@vkolomeyko vkolomeyko requested review from a team, JamesHR3 and LWogan December 14, 2023 17:00
@corda-jenkins-ci02
Copy link
Contributor

Scanning for breaking API changes introduced by this PR

Scan Succeeded

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -22,7 +22,7 @@
"description": "The maximum number of cached flow sandboxes.",
"type": "integer",
"minimum": 0,
"default": 10
"default": 20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to be careful with this.
While I'm sure increasing this is a good idea (and maybe it should be much much higher (100+) in a production system, increasing this to get our tests to pass is masking a problem rather than fixing it.
The same issues will be hit at a higher load.

CC @simon-johnson-r3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. The underlying problem is that we do not handle flow execution very well when sandbox cache eviction happens. This PR was just an attempt to improve the current situation with E2E tests.
An alternative could be to make E2E tests run less in parallel by using @Isolated or SAME_THREAD annotations.

@vkolomeyko
Copy link
Contributor Author

Closing without merging.
E2E tests stability will be addressed by running less tests in parallel at the same time.

@vkolomeyko vkolomeyko closed this Dec 18, 2023
@vkolomeyko vkolomeyko deleted the vkolomeyko/e2e-flake-investigation branch December 18, 2023 13:54
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.

2 participants