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-18320 Support for a flow specific timeout on Flow Message API #1394

Merged
merged 25 commits into from
Dec 20, 2023

Conversation

mbrkic-r3
Copy link
Contributor

@mbrkic-r3 mbrkic-r3 commented Dec 12, 2023

Support for a flow specific timeout on Flow Message API. Client can specify a flow session specific timeout on Flow Message API. If set, it will be passed to the initiated party to also use it. If the client does not provide a timeout value then the value from Corda Configuration is used.

A new class FlowSessionConfiguration is introduced to store session configuration. Methods that used configuration parameter requireClose will be deprecated with another PR.

Avro schema for SessionState was modified, field requireClose was removed as this value will be stored in sessionProperties.

…lient can specify a flow session specific timeout on Flow Message API. If set, it will be passed to the initiated party to also use it. If the client does not provide a timeout value then the value from Corda Configuration is used.
@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Dec 12, 2023

Scanning for breaking API changes introduced by this PR

Scan Succeeded

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Dec 12, 2023

Non-blocking downstream job failed for corda-non-functional-test

https://ci02.dev.r3.com/job/Corda5/job/corda-api-compatibility/job/PR-1394/14/ has failed for PR 1394 build 14

Please investigate if your changes may have broken compilation on https://github.com/corda/corda-non-functional-test

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Dec 12, 2023

Non-blocking downstream job failed for corda-e2e-test

https://ci02.dev.r3.com/job/Corda5/job/corda-api-compatibility/job/PR-1394/15/ has failed for PR 1394 build 15

Please investigate if your changes may have broken compilation on https://github.com/corda/corda-e2e-tests

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Dec 12, 2023

Jenkins build for PR 1394 build 25

Build Successful:
Jar artifact version produced by this PR: 5.2.0.20-alpha-1703066309585

@mbrkic-r3 mbrkic-r3 marked this pull request as ready for review December 14, 2023 10:05
@mbrkic-r3 mbrkic-r3 requested a review from a team as a code owner December 14, 2023 10:05
@mbrkic-r3 mbrkic-r3 requested a review from a team December 14, 2023 10:08
Copy link
Contributor

@LWogan LWogan left a comment

Choose a reason for hiding this comment

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

The method overloading is not ideal like you already said. Could we potentially add a field flowSessionConfig/sessionProperties Map<String, String> or something like that to the initiateFlow API? I think it's too late for requireClose but there may be more things we want to configure per session in the future.

@LWogan
Copy link
Contributor

LWogan commented Dec 14, 2023

The compatibility scan failed due to the API change. Is this just a warning or something that needs to be addressed?

@mbrkic-r3
Copy link
Contributor Author

The compatibility scan failed due to the API change. Is this just a warning or something that needs to be addressed?

https://r3holdco.slack.com/archives/C039J1A3ZQB/p1702390578322899

Looks like an issue with the API Scanner, for now - since the change doesn't look like it is breaking - I would run ./gradlew cementApi and push the changes

@mbrkic-r3
Copy link
Contributor Author

The method overloading is not ideal like you already said. Could we potentially add a field flowSessionConfig/sessionProperties Map<String, String> or something like that to the initiateFlow API? I think it's too late for requireClose but there may be more things we want to configure per session in the future.

Yes, we might add that to make it future-proof, in case we'll need to add more properties. It might be better to use SessionConfigBuilder as that would enforce setting properties that actually exist and also type safety. We can add requireClose there and deprecate methods that use it as separate parameter.

LWogan
LWogan previously approved these changes Dec 15, 2023
Copy link
Contributor

@LWogan LWogan left a comment

Choose a reason for hiding this comment

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

LGTM

@mbrkic-r3 mbrkic-r3 requested a review from LWogan December 19, 2023 09:17
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

@mbrkic-r3 mbrkic-r3 merged commit c632389 into release/os/5.2 Dec 20, 2023
6 checks passed
@mbrkic-r3 mbrkic-r3 deleted the mbrkic-r3/CORE-18320/flow-timeout branch December 20, 2023 10:07
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