CORE-17387 - flow checkpoint maintenance events #4746

driessamyn · 2023-09-30T13:38:20Z

Create a component in the flow service responsible for processing scheduled task triggers and generating checkpoint maintenance events.
This includes integration with the State Manager component and as this is the first component that uses it, this change includes a number of small changes to that library.

API change: corda/corda-api#1272

driessamyn · 2023-09-30T13:39:25Z

components/flow/flow-service/src/main/kotlin/net/corda/flow/maintenance/FlowMaintenanceImpl.kt

+    private val coordinator = coordinatorFactory.createCoordinator<FlowExecutor>(::eventHandler)
+    override fun onConfigChange(config: Map<String, SmartConfig>) {
+        val messagingConfig = config.getConfig(ConfigKeys.MESSAGING_CONFIG)
+        // TODO - fix config key. The state manager has nothing to do with messaging.


@conalsmith-r3 , @jujoramos please advice here.
I think we need a ConfigKey for the StateManager.

I've created a ticket for this already 👍 .

driessamyn · 2023-09-30T13:40:20Z

.../flow/flow-service/src/main/kotlin/net/corda/flow/maintenance/SessionTimeoutTaskProcessor.kt

+            // TODO - we must be able to specify additional filters so we can limit to selecting those sessions that are still open
+            // TODO - we must be able to limit by type of state


@jujoramos , @conalsmith-r3 , please advice how this can be implemented.

We haven't implemented custom filters yet, but there's already a ticket created for it.

driessamyn · 2023-09-30T13:41:05Z

.../flow/flow-service/src/main/kotlin/net/corda/flow/maintenance/SessionTimeoutTaskProcessor.kt

+                // TODO - return an avro message (schema TBC) for each checkpoint
+                // TODO - define topic to publish message on


@JamesHR3 , should this be a new topic?
Do we know yet what the schema for the message needs to look like?

jujoramos

I know it's still in draft and some changes are blocked by the state manager implementation, just leaving some comments.

applications/examples/sandbox-app/build.gradle

jujoramos · 2023-10-02T12:49:07Z

components/flow/flow-service/src/main/kotlin/net/corda/flow/maintenance/FlowMaintenanceImpl.kt

+    private val coordinator = coordinatorFactory.createCoordinator<FlowExecutor>(::eventHandler)
+    override fun onConfigChange(config: Map<String, SmartConfig>) {
+        val messagingConfig = config.getConfig(ConfigKeys.MESSAGING_CONFIG)
+        // TODO - fix config key. The state manager has nothing to do with messaging.


I've created a ticket for this already 👍 .

jujoramos · 2023-10-02T12:55:03Z

.../flow/flow-service/src/main/kotlin/net/corda/flow/maintenance/SessionTimeoutTaskProcessor.kt

+            // TODO - we must be able to specify additional filters so we can limit to selecting those sessions that are still open
+            // TODO - we must be able to limit by type of state


We haven't implemented custom filters yet, but there's already a ticket created for it.

components/ledger/ledger-common-flow/build.gradle

components/ledger/ledger-consensual-flow/build.gradle

components/ledger/ledger-utxo-flow/build.gradle

…tests that need it.

- Fix FlowServiceTest

…ect. - Fix Metadata so it's an immutable map. - Add tests for Metadata checks.

- detekt fix

corda-jenkins-ci02 · 2023-10-02T18:17:14Z

Jenkins build for PR 4746 build 16

Build Successful:
Jar artifact version produced by this PR: 5.1.0.0-alpha-1696347157836
Helm chart version produced by this PR: 5.1.0-alpha.1696347157836
Helm chart pushed to: oci://corda-os-docker-dev.software.r3.com/helm-charts/pr-4746/corda

jujoramos

LGTM, just some minor comments.

components/flow/flow-service/build.gradle

components/flow/flow-service/src/main/kotlin/net/corda/flow/maintenance/FlowMaintenanceImpl.kt

jujoramos · 2023-10-03T08:20:29Z

...e-manager/state-manager-api/src/test/kotlin/net/corda/libs/statemanager/api/MetadataTests.kt

+        assertDoesNotThrow {
+            Metadata(mapOf("foo" to value))
+        }


Nitpick: why the assertDoesNotThrow{} here?, if the exception is thrown the test will fail anyways (and removing it also removes an unneeded dependency to jupiter).

It just makes it more explicit I guess.
The test framework does threat the 2 things different (test failed vs error).
E.g.

@Test fun foo() { throw IllegalArgumentException("foo") } @Test fun bar() { assertDoesNotThrow { throw IllegalArgumentException("bar") } }

results in:

I agree it technically doesn't make much difference, but semantically maybe it does.

In that case, and just to keep consistency across this single test (use single assertion library), I would use something like the following:

assertThatCode { Metadata(mapOf("foo" to value)) }.doesNotThrowAnyException()

jujoramos · 2023-10-03T08:21:27Z

...e-manager/state-manager-api/src/test/kotlin/net/corda/libs/statemanager/api/MetadataTests.kt

+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertDoesNotThrow
+import org.junit.jupiter.api.assertThrows


Nitpick: we are mixing two assertions libraries in the test, can we use only one (`assert4j would be my preferred choice but it's up to you)?

I guess the assertThrows syntax is a bit more natural than assertJ's assertThatThrownBy and is more common across the codebase:

❯ grep -r --include "*.kt" assertThat | wc -l 9129 ❯ grep -r --include "*.kt" assertThrows | wc -l 1792 ❯ grep -r --include "*.kt" assertThatThrownBy | wc -l 369

Both libraries seem to be used widely, with a slight preference for the junit assertions according to the imports:

❯ grep -r --include "*.kt" "import org.assertj.core.api.Assertions" | wc -l 852 ❯ grep -r --include "*.kt" "import org.junit.jupiter.api.assert" | wc -l 1169

I don't really mind. I don't mind we settle to use one, but using multiple equally doesn't bother me as long as the code is readable.

Agreed, but I'd argue that we should keep consistency and use a single assertions framework per test class, even though we are not consistent across the codebase.

...e-manager/state-manager-api/src/test/kotlin/net/corda/libs/statemanager/api/MetadataTests.kt

JamesHR3

Couple of nitpicking things but looks good.

components/flow/flow-service/build.gradle

components/ledger/ledger-common-flow/build.gradle

.../state-manager/state-manager-api/src/main/kotlin/net/corda/libs/statemanager/api/Metadata.kt

mag1c-48

LGTM - build files

blsemo

Ledger changes are fine

blsemo

Re-approved for ledger

mcgovc

LGTM - build files

driessamyn commented Sep 30, 2023

View reviewed changes

jujoramos reviewed Oct 2, 2023

View reviewed changes

driessamyn force-pushed the driessamyn/CORE-17387/flow-checkpoint-maintenance-events branch from 51ece0e to d73c655 Compare October 2, 2023 15:18

driessamyn added 7 commits October 2, 2023 16:42

SessionTimeoutTaskProcessor implementation.

6ed83d5

Flow maintenance service.

addec41

Missing annotation & package export

6f0aa0f

Ensure StateManagerFactory OSGi service is available to those OSGi …

80fd640

…tests that need it.

More OSGi nonsense.

63985fe

- Don't create if the correct config not passed in.

083efc2

- Fix FlowServiceTest

- SessionTimeoutTaskProcessor integration with new topic and AVRO obj…

297c5ba

…ect. - Fix Metadata so it's an immutable map. - Add tests for Metadata checks.

driessamyn force-pushed the driessamyn/CORE-17387/flow-checkpoint-maintenance-events branch from d73c655 to 297c5ba Compare October 2, 2023 15:42

driessamyn requested a review from JamesHR3 October 2, 2023 15:54

driessamyn added 4 commits October 2, 2023 17:03

- Revert api version

1e47209

- detekt fix

- Add postgres driver

c7979da

Fix json deserialiser & add test

943995f

Fix json deserialiser & add test

5171e1c

driessamyn marked this pull request as ready for review October 2, 2023 19:03

driessamyn requested review from a team as code owners October 2, 2023 19:03

jujoramos previously approved these changes Oct 3, 2023

View reviewed changes

PR feedback on use of assertThrows vs assertThatThrownBy

8fcf4c4

driessamyn dismissed jujoramos’s stale review via 8fcf4c4 October 3, 2023 09:01

JamesHR3 reviewed Oct 3, 2023

View reviewed changes

components/flow/flow-service/build.gradle Outdated Show resolved Hide resolved

components/ledger/ledger-common-flow/build.gradle Outdated Show resolved Hide resolved

.../state-manager/state-manager-api/src/main/kotlin/net/corda/libs/statemanager/api/Metadata.kt Show resolved Hide resolved

PR feedback

9516cbb

driessamyn requested a review from JamesHR3 October 3, 2023 12:16

driessamyn requested a review from jujoramos October 3, 2023 12:16

PR feedback

cc9d374

JamesHR3 previously approved these changes Oct 3, 2023

View reviewed changes

mag1c-48 previously approved these changes Oct 3, 2023

View reviewed changes

blsemo previously approved these changes Oct 3, 2023

View reviewed changes

jujoramos previously approved these changes Oct 3, 2023

View reviewed changes

Fix Liquibase dependency

3e81936

driessamyn dismissed stale reviews from jujoramos, blsemo, mag1c-48, and JamesHR3 via 3e81936 October 3, 2023 14:55

simon-johnson-r3 approved these changes Oct 3, 2023

View reviewed changes

blsemo approved these changes Oct 3, 2023

View reviewed changes

JamesHR3 approved these changes Oct 3, 2023

View reviewed changes

mcgovc approved these changes Oct 3, 2023

View reviewed changes

driessamyn requested a review from mag1c-48 October 3, 2023 16:52

driessamyn merged commit 6a29a06 into release/os/5.1 Oct 3, 2023
1 check passed

driessamyn deleted the driessamyn/CORE-17387/flow-checkpoint-maintenance-events branch October 3, 2023 16:53

corda-jenkins-ci02 bot mentioned this pull request Oct 3, 2023

CORE-17387: Merging forward updates from release/os/5.1 to release/interop/syntax - 2023-10-03 #4769

Merged

corda-jenkins-ci02 bot mentioned this pull request Oct 18, 2023

CORE-17627: Merging forward updates from release/os/5.1 to release/interop/syntax - 2023-10-18 #4922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-17387 - flow checkpoint maintenance events #4746

CORE-17387 - flow checkpoint maintenance events #4746

driessamyn commented Sep 30, 2023 •

edited

Loading

driessamyn Sep 30, 2023

jujoramos Oct 2, 2023

driessamyn Sep 30, 2023

jujoramos Oct 2, 2023

driessamyn Sep 30, 2023

jujoramos left a comment

jujoramos Oct 2, 2023

jujoramos Oct 2, 2023

corda-jenkins-ci02 bot commented Oct 2, 2023 •

edited

Loading

jujoramos left a comment

jujoramos Oct 3, 2023

driessamyn Oct 3, 2023

jujoramos Oct 3, 2023

jujoramos Oct 3, 2023

driessamyn Oct 3, 2023

jujoramos Oct 3, 2023 •

edited

Loading

JamesHR3 left a comment

mag1c-48 left a comment

blsemo left a comment

blsemo left a comment

mcgovc left a comment

		// TODO - we must be able to specify additional filters so we can limit to selecting those sessions that are still open
		// TODO - we must be able to limit by type of state

		// TODO - return an avro message (schema TBC) for each checkpoint
		// TODO - define topic to publish message on

CORE-17387 - flow checkpoint maintenance events #4746

CORE-17387 - flow checkpoint maintenance events #4746

Conversation

driessamyn commented Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jujoramos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corda-jenkins-ci02 bot commented Oct 2, 2023 • edited Loading

jujoramos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jujoramos Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

JamesHR3 left a comment

Choose a reason for hiding this comment

mag1c-48 left a comment

Choose a reason for hiding this comment

blsemo left a comment

Choose a reason for hiding this comment

blsemo left a comment

Choose a reason for hiding this comment

mcgovc left a comment

Choose a reason for hiding this comment

driessamyn commented Sep 30, 2023 •

edited

Loading

corda-jenkins-ci02 bot commented Oct 2, 2023 •

edited

Loading

jujoramos Oct 3, 2023 •

edited

Loading