CORE-18630 Add state manager for outbound sessions #5438

YashNabar · 2024-01-15T12:38:27Z

Add state manager transitions to the stateful session manager for outbound sessions. Introduce caching for established sessions.
Note: This feature is disabled in this change.

…er-for-inbound-sessions' into yash/outbound-sessions Conflicts: components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

yift-r3 · 2024-01-15T14:56:53Z

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

+            (sessionsNotInCache.map { it.first } - inboundSessionsFromStateManager.map { it.first }.toSet()).map {
+                getSessionIdFilter(getSessionId(it))
+            }
+        val outboundSessionsFromStateManager: List<Pair<T, SessionManager.SessionDirection>> =


I'm confused; where in the state API we asked for only inbound or outbound?

The inbound and outbound sessions are keyed differently, so to retrieve inbound sessions (keyed by session ID) we simply do a stateManager.get(), but for outbound sessions we query based on metadata with stateManager.findByMetadataMatchingAny().

That said, if we're happy to have a common metadata type for inbound and outbound (as I have here), we could query for both types of sessions with stateManager.findByMetadataMatchingAny()

Note that stateManager.findByMetadataMatchingAny() is order O(N) where N is the size of the table as stateManager.get() is O(1). So I think we should avoid stateManager.findByMetadataMatchingAny() where possible (as we need to lookup outbound sessions by sessionId sometimes we can't always avoid this).

Can't we key both by session ID (maybe add input/output before hand?)

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

corda-jenkins-ci02 · 2024-01-16T14:13:59Z

Jenkins build for PR 5438 build 16

Build Successful:
Jar artifact version produced by this PR: 5.2.0.0-alpha-1706115052476
Helm chart version produced by this PR: 5.2.0-alpha.1706115052476
Helm chart pushed to: oci://corda-os-docker-dev.software.r3.com/helm-charts/pr-5438/corda

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

…on messages from StatefulSessionManager

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

dimosr · 2024-01-18T09:31:35Z

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

+                    }
+
+                    SessionStatus.SentInitiatorHello, SessionStatus.SentInitiatorHandshake -> {
+                        state.replaySessionMessage()?.let {


I think returning NewSessionNeeded here can be a bit misleading, since that was supposed to be used when a session doesn't exist at all, while here it exists and we just need to replay.

I understand why you probably did it, just to comply with the logic in the outbound message processor which otherwise won't send these messages. But, there are a few options that might keep the codebase a bit easier to understand:

we could also have a publisher in this component and publish the message here instead of relying on the outbound message processor (we don't require any atomic semantics). That way, we could still return SessionAlreadyPending which I think will be slightly cleaner. The downside is if we do that before we try and update the state manager, there's still a chance we'll replay from multiple link managers.

we could introduce an extra field in SessionAlreadyPending for messages that need to be sent by upstream clients. Then, we can include the messages that the outbound message processor needs to send there. That way, we can process the result of the state manager updates and filter out messages in cases where updates failed because another link manager managed to write first (so that we replay only once).

Note: This is more of a cleanliness thing I think, since functionally what you have could work. So, you could do that as a follow-up cleanup if you want.

I think we could rename NewSessionNeeded to SendSessionNegotiationMessage (one button in InteliJ so we can do this quickly now).

Not sure what the second option would do to the layer above (would the logic become the same for SessionAlreadyPending and NewSessionNeeded ). Not sure I like adding the complexity of adding an extra publisher (first option).

For the second option, it would probably be an extra field in SessionAlreadyPending with messages to send optionally & a piece of logic / conditional on the outbound processor that would check in that case whether there are messages to send if a session is already pending.

Should I raise a follow up ticket to do this?

yeah, if the other people agree, we can raise a follow-up ticket to refactor this a bit.

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

williamvigorr3 · 2024-01-18T10:22:29Z

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

+                    }
+
+                    SessionStatus.SentInitiatorHello, SessionStatus.SentInitiatorHandshake -> {
+                        state.replaySessionMessage()?.let {


I think we could rename NewSessionNeeded to SendSessionNegotiationMessage (one button in InteliJ so we can do this quickly now).

Not sure what the second option would do to the layer above (would the logic become the same for SessionAlreadyPending and NewSessionNeeded ). Not sure I like adding the complexity of adding an extra publisher (first option).

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

...onents/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/SessionManagerImpl.kt

...ts/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/SessionManagerWarnings.kt

.../link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/metadata/SessionMetadata.kt

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

…er-for-inbound-sessions' into yash/outbound-sessions/CORE-18630 Conflicts: components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/common/CommonComponents.kt components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/SessionManagerImpl.kt components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/metadata/InboundSessionMetadata.kt

…0' into yash/outbound-sessions/CORE-18630

This reverts commit 2d6a6c0.

…/enable-e2e-tests-over-yash # Conflicts: # components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

CORE-18630 Respond to review comments

…0' into yash/caching/CORE-18630 Conflicts: components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

CORE-18630 Revise caching for outbound sessions

sonarqubecloud · 2024-01-24T16:53:37Z

Quality Gate passed

Kudos, no new issues were introduced!

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

See analysis details on SonarCloud

williamvigorr3

LGTM

YashNabar added 2 commits January 15, 2024 12:29

Add outbound transitions

ca59499

Merge remote-tracking branch 'origin/WillV/CORE-18631-add-state-manag…

74a22e8

…er-for-inbound-sessions' into yash/outbound-sessions Conflicts: components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

yift-r3 reviewed Jan 15, 2024

View reviewed changes

dimosr reviewed Jan 16, 2024

View reviewed changes

dimosr mentioned this pull request Jan 16, 2024

CORE-18631, CORE-18630: Add state manager for inbound and outbound sessions #5424

Merged

Fix build/detekt, address some PR comments

8468379

dimosr reviewed Jan 17, 2024

View reviewed changes

williamvigorr3 and others added 2 commits January 17, 2024 12:41

Change the SessionManager to not use heartbeat manager + replay sessi…

38f9ed4

…on messages from StatefulSessionManager

Change outbound session keys, separate metadata types

cde54de

williamvigorr3 mentioned this pull request Jan 17, 2024

CORE-18631: Start unit tests, some clean up #5441

Merged

dimosr reviewed Jan 18, 2024

View reviewed changes

williamvigorr3 reviewed Jan 18, 2024

View reviewed changes

...ink-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt Outdated Show resolved Hide resolved

yift-r3 reviewed Jan 18, 2024

View reviewed changes

YashNabar and others added 15 commits January 18, 2024 15:42

Fix state version

e75331e

Tidy up the SessionMetadata

7fdfc27

Merge remote-tracking branch 'origin/yash/outbound-sessions/CORE-1863…

e935e4d

…0' into yash/outbound-sessions/CORE-18630

Handle failed updates, fix test

4e0636c

Respond to minor review comments

b164401

Missed review comment

fa43394

CORE-18631: Run e2e tests

7d55084

Add sessionID cache

2d6a6c0

Add crypto to service access

940d37f

Revert Yash changes for now

7c323f1

Revert "Add sessionID cache"

b5bd5c9

This reverts commit 2d6a6c0.

Merge branch 'yash/outbound-sessions/CORE-18630' into yift/core-18631…

72e996e

…/enable-e2e-tests-over-yash # Conflicts: # components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

Read arguments

cb44622

Apply review comments

45540a5

Respond to review comments

8b27fac

williamvigorr3 mentioned this pull request Jan 23, 2024

CORE-18630 Respond to review comments #5487

Merged

YashNabar and others added 8 commits January 23, 2024 10:12

CORE-18630 Revise caching for outbound sessions

2d4ee25

Fix test

bd3b711

Fix tests, address comments

414cb30

Merge pull request #5487 from corda/wvigor/CORE-18630-respond-to-review

2ac9016

CORE-18630 Respond to review comments

Merge remote-tracking branch 'origin/yash/outbound-sessions/CORE-1863…

f44a910

…0' into yash/caching/CORE-18630 Conflicts: components/link-manager/src/main/kotlin/net/corda/p2p/linkmanager/sessions/StatefulSessionManagerImpl.kt

Address comments, fix test

6fa2ff5

Merge pull request #5488 from corda/yash/caching/CORE-18630

3eb780d

CORE-18630 Revise caching for outbound sessions

CORE-18630 Disable stateful session manager

7bccd77

YashNabar changed the title ~~CORE-18630 Add state manager for outbound sessions (WIP)~~ CORE-18630 Add state manager for outbound sessions Jan 24, 2024

yift-r3 approved these changes Jan 24, 2024

View reviewed changes

williamvigorr3 approved these changes Jan 24, 2024

View reviewed changes

YashNabar marked this pull request as ready for review January 25, 2024 09:28

YashNabar requested review from a team as code owners January 25, 2024 09:28

YashNabar merged commit 88b86b2 into WillV/CORE-18631-add-state-manager-for-inbound-sessions Jan 25, 2024

YashNabar deleted the yash/outbound-sessions/CORE-18630 branch January 25, 2024 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-18630 Add state manager for outbound sessions #5438

CORE-18630 Add state manager for outbound sessions #5438

YashNabar commented Jan 15, 2024 •

edited

Loading

yift-r3 Jan 15, 2024

YashNabar Jan 15, 2024

williamvigorr3 Jan 18, 2024

yift-r3 Jan 18, 2024

corda-jenkins-ci02 bot commented Jan 16, 2024 •

edited

Loading

dimosr Jan 18, 2024

williamvigorr3 Jan 18, 2024

dimosr Jan 18, 2024

williamvigorr3 Jan 24, 2024

dimosr Jan 25, 2024

williamvigorr3 Jan 18, 2024

sonarqubecloud bot commented Jan 24, 2024

williamvigorr3 left a comment

CORE-18630 Add state manager for outbound sessions #5438

CORE-18630 Add state manager for outbound sessions #5438

Conversation

YashNabar commented Jan 15, 2024 • 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

corda-jenkins-ci02 bot commented Jan 16, 2024 • 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

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 24, 2024

Quality Gate passed

williamvigorr3 left a comment

Choose a reason for hiding this comment

YashNabar commented Jan 15, 2024 •

edited

Loading

corda-jenkins-ci02 bot commented Jan 16, 2024 •

edited

Loading