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-16879 - replace max retry attempts with max retry window duration #1236

Merged
merged 10 commits into from
Sep 8, 2023

Conversation

conalsmith-r3
Copy link
Contributor

@conalsmith-r3 conalsmith-r3 commented Sep 5, 2023

Runtime-os change - corda/corda-runtime-os#4587

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Sep 5, 2023

Jenkins build for PR 1236 build 11

Build Successful:
Jar artifact version produced by this PR: 5.1.0.19-alpha-1694179391729

Comment on lines 14 to 20
"maxRetryWindowDuration": {
"description": "The duration in milliseconds after a transient error that Corda retries a flow before failing it. A value of zero disables retries.",
"type": "integer",
"minimum": 0,
"maximum": 1000,
"default": 5
"maximum": 2147483647,
"default": 120000
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this considered an API breaking change, as the flow configuration itself is modifiable by clients?.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah it is - this should be in a new version of the schema.

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, unless we add maxRetryWindowDuration in a non-breaking way (as an optional config, leaving in maxRetryAttempts, even though it's non-functional).

I'll pull this change out into a new version of the corda.flow.json, I assume the new config will be version 1.1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Conal - yes I think that's the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing with Simon, we can't make breaking changes to config schema such as removing config properties due to reasons highlighted in the wiki page. I've added the new property with a default thus a non-breaking change.

@conalsmith-r3 conalsmith-r3 force-pushed the conal/CORE-16879-retry-window-duration branch from cb44ddb to 9cf0ae6 Compare September 7, 2023 13:41
"minimum": 0,
"maximum": 2147483647,
"default": 120000
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GET on /config/flow

{
    "section": "corda.flow",
    "sourceConfig": "",
    "configWithDefaults": "{\"event\":{\"maxRetries\":5,\"messageResendWindow\":60000},\"processing\":{\"cleanupTime\":600000,\"maxFlowSleepDuration\":900000,\"maxRetryAttempts\":5,\"maxRetryDelay\":16000,\"maxRetryWindowDuration\":120000},\"session\":{\"cleanupTime\":600000,\"p2pTTL\":300000,\"timeout\":1800000}}",
    "schemaVersion": {
        "major": 1,
        "minor": 0
    },
    "version": 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

POST to /config with

{
    "section": "corda.flow",
    "version": 0,
    "config": {
        "processing":{
            "maxRetryAttempts": 1,
            "maxRetryWindowDuration": 1
        }
    },
    "schemaVersion": {
        "major": 1,
        "minor": 0
    }
}

GET /config/flow:

{
    "section": "corda.flow",
    "sourceConfig": "{\"processing\":{\"maxRetryAttempts\":1,\"maxRetryWindowDuration\":1}}",
    "configWithDefaults": "{\"event\":{\"maxRetries\":5,\"messageResendWindow\":60000},\"processing\":{\"cleanupTime\":600000,\"maxFlowSleepDuration\":900000,\"maxRetryAttempts\":1,\"maxRetryDelay\":16000,\"maxRetryWindowDuration\":1},\"session\":{\"cleanupTime\":600000,\"p2pTTL\":300000,\"timeout\":1800000}}",
    "schemaVersion": {
        "major": 1,
        "minor": 0
    },
    "version": 1
}

@JamesHR3 JamesHR3 marked this pull request as ready for review September 8, 2023 10:08
@JamesHR3 JamesHR3 requested a review from a team as a code owner September 8, 2023 10:08
jujoramos
jujoramos previously approved these changes Sep 8, 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

@JamesHR3 JamesHR3 merged commit d4b81e2 into release/os/5.1 Sep 8, 2023
1 check passed
@JamesHR3 JamesHR3 deleted the conal/CORE-16879-retry-window-duration branch September 8, 2023 13:35
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