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-9031: Add forceUpgrade parameter to VirtualNodeUpgradeRequest #1263

Conversation

Tom-Fitzpatrick
Copy link
Contributor

@Tom-Fitzpatrick Tom-Fitzpatrick commented Sep 26, 2023

This adds an optional forceUpgrade field to the VirtualNodeUpgradeRequest schema which defaults to false. This supports the work being done in corda/corda-runtime-os#4734.

@Tom-Fitzpatrick Tom-Fitzpatrick force-pushed the tomf/CORE-9031/vnode-force-operation-in-progress branch from 919e50d to 444c0dc Compare September 28, 2023 14:20
@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Sep 28, 2023

Jenkins build for PR 1263 build 11

Build Successful:
Jar artifact version produced by this PR: 5.1.0.28-alpha-1696424291860

@Tom-Fitzpatrick Tom-Fitzpatrick self-assigned this Sep 28, 2023
@Tom-Fitzpatrick Tom-Fitzpatrick force-pushed the tomf/CORE-9031/vnode-force-operation-in-progress branch 2 times, most recently from 18f0a00 to e47e1d4 Compare October 2, 2023 16:25
@Tom-Fitzpatrick Tom-Fitzpatrick marked this pull request as ready for review October 3, 2023 09:10
@Tom-Fitzpatrick Tom-Fitzpatrick requested a review from a team as a code owner October 3, 2023 09:10
@Tom-Fitzpatrick Tom-Fitzpatrick requested a review from a team October 3, 2023 09:10
@@ -9,7 +9,7 @@ cordaProductVersion = 5.1.0
# NOTE: update this each time this module contains a breaking change
## NOTE: currently this is a top level revision, so all API versions will line up, but this could be moved to
## a per module property in which case module versions can change independently.
cordaApiRevision = 26
cordaApiRevision = 27
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a breaking change though?

I wonder if Avro will do defaulting for us automatically if the recipient has a new schema and payload does not have the value for the field?

FAO: @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.

I tried without the version bump, to see if I could get a green build in runtime-os without it. But the corda-api PR build here fails first because it tests the PR with corda-runtime-os code. The generated java file for VirtualNodeUpgradeRequest doesn't provide a constructor without forceUpgrade, and we get the following error when trying to use it:

e: file:///var/jenkins/workspace/ion-compatibility_release_os_5.1/components/virtual-node/virtual-node-rest-service-impl/src/main/kotlin/net/corda/virtualnode/rest/impl/v1/VirtualNodeRestResourceImpl.kt:348:41 None of the following functions can be called with the arguments supplied:   public constructor VirtualNodeUpgradeRequest() defined in net.corda.data.virtualnode.VirtualNodeUpgradeRequest |     public constructor VirtualNodeUpgradeRequest(virtualNodeShortHash: String!, cpiFileChecksum: String!, actor: String!, forceUpgrade: Boolean!) defined in net.corda.data.virtualnode.VirtualNodeUpgradeRequest

I don't think there's a good way around this, so I think the version bump is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree - looks like we will not be able o workaround without version bump.

@vkolomeyko vkolomeyko requested review from simon-johnson-r3, a team and vkolomeyko October 3, 2023 10:45
vkolomeyko
vkolomeyko previously approved these changes Oct 3, 2023
Copy link
Contributor

@vkolomeyko vkolomeyko left a comment

Choose a reason for hiding this comment

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

LGTM

I have also reviewed the corda-runtime-os PR and it looks fine. So, tomorrow, we can start integrating both PRs.

@Tom-Fitzpatrick Tom-Fitzpatrick force-pushed the tomf/CORE-9031/vnode-force-operation-in-progress branch from 743a8fb to a11bb9b Compare October 4, 2023 12:57
@vkolomeyko vkolomeyko self-requested a review October 4, 2023 15:41
Copy link
Contributor

@vkolomeyko vkolomeyko left a comment

Choose a reason for hiding this comment

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

LGTM

@Tom-Fitzpatrick Tom-Fitzpatrick merged commit 4f8bdaf into release/os/5.1 Oct 4, 2023
1 check passed
@Tom-Fitzpatrick Tom-Fitzpatrick deleted the tomf/CORE-9031/vnode-force-operation-in-progress branch October 4, 2023 15:43
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.

3 participants