Use separate event stream per namespace #1388

nguyer · 2023-08-10T20:53:53Z

This PR resolves #1381 and #1382

Because we are moving from a single event stream to one per namespace, existing subscriptions will be recreated and events will be replayed on a new event stream the first time a FireFly Core node starts with these changes. Otherwise there is no user impacting change in this PR.

Event streams now follow the format <plugin_topic_name>/<namespace_name>. For example, event stream for the default namespace with a plugin topic of 0 would now be: 0/default.

There is still outstanding work to do in the ~~tokens plugin and~~ tokens connectors, but I'm splitting those out into separate PRs.

Signed-off-by: Nicko Guyer <[email protected]>

codecov-commenter · 2023-08-30T19:33:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b1fd7dc) 99.99% compared to head (7972561) 99.99%.
Report is 20 commits behind head on main.

❗ Current head 7972561 differs from pull request most recent head ba212a9. Consider uploading reports for the commit ba212a9 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #1388    +/-   ##
========================================
  Coverage   99.99%   99.99%            
========================================
  Files         322      322            
  Lines       23301    23407   +106     
========================================
+ Hits        23299    23405   +106     
  Misses          1        1            
  Partials        1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nguyer · 2023-08-31T18:48:19Z

If we have multiple namespaces using the same token connector (which I think is a valid configuration) with these changes FireFly would create multiple WebSocket connections to that one token connector which may not work correctly until the token connectors themselves are updated. I'm not sure if that's an issue or not.

peterbroadhurst

This is a great piece of work @nguyer 👍

A few comments/requests to think through.

peterbroadhurst · 2023-08-31T19:34:48Z

internal/blockchain/ethereum/ethereum.go


-		return nil
+func (e *Ethereum) getTopic(namespace string) string {
+	return fmt.Sprintf("%s/%s", e.pluginTopic, namespace)


Think it's important we document this, in the docs for the configuration of the EthconnectConfigTopic config entry

peterbroadhurst · 2023-08-31T19:38:14Z

internal/blockchain/ethereum/ethereum.go

@@ -279,7 +282,7 @@ func (e *Ethereum) AddFireflySubscription(ctx context.Context, namespace *core.N
 		return "", err
 	}

-	sub, err := e.streams.ensureFireFlySubscription(ctx, namespace.Name, version, ethLocation.Address, contract.FirstEvent, e.streamID, batchPinEventABI)
+	sub, err := e.streams.ensureFireFlySubscription(ctx, namespace.Name, version, ethLocation.Address, contract.FirstEvent, e.streamID[namespace.Name], batchPinEventABI)


Do we need a check that the map entry is set (to avoid a nil panic)?

Or is the dynamic config loading threading model such that we know it's impossible for an API call to come down to this layer to create a subscription for a new listener, after the stream has been cleaned up?

peterbroadhurst · 2023-08-31T19:42:47Z

internal/blockchain/ethereum/ethereum.go

-	defer close(e.closed)
+func (e *Ethereum) eventLoop(namespace string) {
+	topic := e.getTopic(namespace)
+	wsconn := e.wsconn[namespace]


Almost certain not to be a problem, but technically I think this should be passed in on the creation of the go-routine, as there's no locking around the map. So the starting routine sets this before kicking off the eventLoop and then we're querying the map in that newly kicked off routine.

peterbroadhurst · 2023-08-31T19:47:39Z

internal/blockchain/fabric/fabric.go

+	return fmt.Sprintf("%s/%s", f.pluginTopic, namespace)
+}
+
+func (f *Fabric) StartNamespace(ctx context.Context, namespace string) (err error) {


I have a recollection that FabConnect currently has a different restriction around how multiple event streams work, to EthConnect. I wonder if it is possible that this PR will change behavior for Fabric in a way that needs to be documented, particularly for multiple namespace scenarios.

This is a good call out. I'm investigating this...

Circling back to the feedback on this PR. The conclusion of this thread was that Fabconnect has never properly supported multiple namespaces using the same contract on the same runtime. These changes do not alter this, but now they will likely result in an error, rather than silently going into completely unpredictable behavior.

Feels like an important callout in the migration docs

Noted in: https://github.com/hyperledger/firefly/pull/1461/files#diff-b2e77d2a6574b8e68136b226be7c14dff6897e0c30751d33585d9d2bb916cea9R65

peterbroadhurst · 2023-08-31T19:52:48Z

internal/namespace/manager.go

-				return err
-			}
-		case pluginCategoryDataexchange:
+		if plugin.category == pluginCategoryDataexchange {


I observe that the same issue addressed in this PR, exists in the connections to DataExchange.

I am not proposing any change is made under this PR to create separate per-namespace event listeners to DataExchange, but on the journey to full completion active/active (which this PR is a step towards) there should be a work item tracked to investigate if a similar code change is required there to have safe operation.

peterbroadhurst · 2023-08-31T19:53:09Z

internal/namespace/manager_test.go

 	nmm.mdx.On("Start", mock.Anything).Return(nil)
-	nmm.mti[0].On("Start", mock.Anything).Return(nil)
-	nmm.mti[1].On("Start", mock.Anything).Return(nil)
+	// nmm.mti[0].On("StartNamespace", mock.Anything, "default").Return(nil)


Probably just delete these lines rather than commenting them out

peterbroadhurst · 2023-08-31T19:53:16Z

internal/namespace/manager_test.go

-func TestStartTokensFail(t *testing.T) {
-	nm, nmm, cleanup := newTestNamespaceManager(t, true)
-	defer cleanup()
+// func TestStartTokensFail(t *testing.T) {


peterbroadhurst · 2023-08-31T19:55:07Z

internal/orchestrator/orchestrator.go

@@ -249,6 +249,16 @@ func (or *orchestrator) Init() (err error) {
 func Purge(ctx context.Context, ns *core.Namespace, plugins *Plugins, dxNodeName string) {
 	// Clear all handlers on all plugins, as this namespace is never coming back
 	setHandlers(ctx, plugins, ns, dxNodeName, nil, nil)
+	err := plugins.Blockchain.Plugin.StopNamespace(ctx, ns.Name)


Could you explain why this one is in Purge (guaranteed never to come back) rather than Stop (we might just be reloading).

It's probably just in the wrong spot. I'll move it. Thanks.

Looking at this one again... it doesn't look like Stop gets called from Purge. Should it? Or does something else call both of them?

peterbroadhurst · 2023-08-31T20:01:44Z

internal/tokens/fftokens/fftokens.go

I think it is only safe to make the tokens change, in coordination with a change to the token connectors themselves to have a separate event stream for each namespace. Because otherwise, there will still only be one ack stream, so whichever namespace receives/acks the events from the token connector will "eat" those events preventing them being dispatched to other namespaces.

Maybe the best thing is to split the token connector work out into another PR that can be coordinated with a change to the token connectors themselves?

Yes. I had originally intended to make this two PRs. But it sat open long enough I kept tacking on more commits 🙂 I'll split out the tokens changes as a separate branch/PR that we can hold until the token connectors are ready.

Signed-off-by: Nicko Guyer <[email protected]>

peterbroadhurst

There are a few minor additional comments from me to think about @nguyer, but none are a blocker to getting this important piece of work into mainline and progressing forwards to a release.

peterbroadhurst · 2024-02-15T21:52:16Z

internal/blockchain/ethereum/ethereum.go

+	if err != nil {
+		return err
+	}
+	// Otherwise, make sure that our event stream is in place


Otherwise feels superfluous here

peterbroadhurst · 2024-02-15T21:52:53Z

internal/blockchain/ethereum/ethereum.go

-	log.L(e.ctx).Infof("Event stream: %s (topic=%s)", e.streamID, e.topic)
+	e.closed[namespace] = make(chan struct{})
+
+	go e.eventLoop(namespace, e.wsconn[namespace], e.closed[namespace])


Probably worth including the namespace in the logging context for this

Thanks. Added below in the event loop

peterbroadhurst · 2024-02-15T21:53:25Z

internal/blockchain/ethereum/ethereum.go

+func (e *Ethereum) StopNamespace(ctx context.Context, namespace string) (err error) {
+	wsconn, ok := e.wsconn[namespace]
+	if ok {
+		wsconn.Close()


Feels like we should wait for the eventLoop to close

Looks like we have e.closed[namespace] for this

Hmm. I added a wait for this here, but it then gets into a deadlock trying to stop the namespace

if ok { <-e.closed[namespace] wsconn.Close() }

peterbroadhurst · 2024-02-15T21:56:51Z

internal/blockchain/ethereum/eventstream.go

@@ -127,7 +127,7 @@ func (s *streamManager) updateEventStream(ctx context.Context, topic string, bat
 	return stream, nil
 }

-func (s *streamManager) ensureEventStream(ctx context.Context, topic string) (*eventStream, error) {
+func (s *streamManager) ensureEventStream(ctx context.Context, topic, pluginTopic string) (*eventStream, error) {
 	existingStreams, err := s.getEventStreams(ctx)


This feels like we should be able to query EVMConnect with a query parameter (which EthConnect might ignore) to scope this down.

Understand this isn't new, but previously it would be much less likely to have many event streams, because there's just one per core runtime. Now we might have 100s... in fact with FFTM I suspect there's a query-limit that means we might miss them after we get above a certain page size.

peterbroadhurst · 2024-02-15T21:57:10Z

internal/blockchain/ethereum/eventstream.go

@@ -140,10 +140,29 @@ func (s *streamManager) ensureEventStream(ctx context.Context, topic string) (*e
 			}
 			return stream, nil
 		}
+		if stream.Name == pluginTopic {


Note comment above would have to be an or select

Sorry I'm not sure I understand this comment. Do you mean the comment on line 144?

peterbroadhurst · 2024-02-15T21:58:42Z

internal/blockchain/fabric/fabric.go

+	return fmt.Sprintf("%s/%s", f.pluginTopic, namespace)
+}
+
+func (f *Fabric) StartNamespace(ctx context.Context, namespace string) (err error) {


Feels like an important callout in the migration docs

Signed-off-by: Nicko Guyer <[email protected]>

nguyer · 2024-02-21T21:48:52Z

I am also noting that the Tezos plugin was developed in parallel to this PR so it does not have these changes. However, TezosConnect does not yet have evenstream support implemented. Once Tezosconnect supports eventstreams we should circle back and make these same changes there too. If I were to do the changes as part of this PR today I wouldn't be able to test them, so I think it's best to hold off for now.

Signed-off-by: Nicko Guyer <[email protected]>

Use separate event stream per namespace

7ad5304

Signed-off-by: Nicko Guyer <[email protected]>

nguyer requested a review from a team as a code owner August 10, 2023 20:53

nguyer added 3 commits August 29, 2023 14:22

Use separate event stream per namespace in tokens plugin

3f04c1e

Signed-off-by: Nicko Guyer <[email protected]>

Implement StopNamespace in fftokens. Fix unit tests.

a37304f

Signed-off-by: Nicko Guyer <[email protected]>

Only init bc if not nil

596caed

Signed-off-by: Nicko Guyer <[email protected]>

peterbroadhurst added the migration_consideration label Aug 31, 2023

peterbroadhurst requested changes Aug 31, 2023

View reviewed changes

Recreate custom contract subscriptions after evenstream migration

66173b2

Signed-off-by: Nicko Guyer <[email protected]>

nguyer marked this pull request as draft September 5, 2023 13:56

denisandreenko mentioned this pull request Sep 19, 2023

Tezos plugin support #1402

Merged

nguyer added 3 commits October 13, 2023 12:06

Merge branch 'main' into eventstreams

e5f1ccc

Signed-off-by: Nicko Guyer <[email protected]>

Update Tezos plugin and fix Ethereum/Fabric EventStream migration

9655d6f

Signed-off-by: Nicko Guyer <[email protected]>

Add namespace to token connector API calls

91a18fc

Signed-off-by: Nicko Guyer <[email protected]>

nguyer mentioned this pull request Nov 3, 2023

Use separate eventstream per namespace hyperledger/firefly-tokens-erc20-erc721#140

Merged

nguyer added 2 commits November 8, 2023 13:27

Fix migration issues and increase test coverage

94bfc75

Signed-off-by: Nicko Guyer <[email protected]>

Test coverage back to 100%

0559710

Signed-off-by: Nicko Guyer <[email protected]>

nguyer force-pushed the eventstreams branch from a01b291 to 0559710 Compare November 9, 2023 16:15

nguyer added 7 commits November 10, 2023 16:07

Re-activate token pool on startup

cecee27

Signed-off-by: Nicko Guyer <[email protected]>

Fix unit tests

aa64620

Signed-off-by: Nicko Guyer <[email protected]>

Address PR feedback

e0b34a5

Signed-off-by: Nicko Guyer <[email protected]>

Merge branch 'dependencies' into eventstreams

3fe2b08

Signed-off-by: Nicko Guyer <[email protected]>

Add AlternateLocators

6b4fd88

Signed-off-by: Nicko Guyer <[email protected]>

Fixes for updating pool locators

bf4656b

Signed-off-by: Nicko Guyer <[email protected]>

Fixes for updating pool locators

a52d3bc

Signed-off-by: Nicko Guyer <[email protected]>

nguyer marked this pull request as ready for review February 2, 2024 18:34

nguyer mentioned this pull request Feb 2, 2024

Migration Issue with ERC-1155 Token Pools #1457

Closed

nguyer added 2 commits February 13, 2024 11:03

Update copyright year

3ad8416

Signed-off-by: Nicko Guyer <[email protected]>

Fix unit test

65283ec

Signed-off-by: Nicko Guyer <[email protected]>

nguyer added 2 commits February 13, 2024 13:34

Add unit tests for updating pool locator

813e29b

Signed-off-by: Nicko Guyer <[email protected]>

Merge branch 'main' into eventstreams

7972561

Signed-off-by: Nicko Guyer <[email protected]>

peterbroadhurst approved these changes Feb 15, 2024

View reviewed changes

nguyer added 2 commits February 21, 2024 16:34

Address PR feedback

ba212a9

Signed-off-by: Nicko Guyer <[email protected]>

Merge branch 'main' into eventstreams

99c7bc1

Signed-off-by: Nicko Guyer <[email protected]>

nguyer added 4 commits February 26, 2024 10:06

PR feedback

ee0770d

Signed-off-by: Nicko Guyer <[email protected]>

Update fftokens unit tests

b9b7a29

Signed-off-by: Nicko Guyer <[email protected]>

Merge branch 'main' into eventstreams

716efcd

Signed-off-by: Nicko Guyer <[email protected]>

Use updated token connectors

dd281b8

Signed-off-by: Nicko Guyer <[email protected]>

nguyer merged commit 5964d51 into hyperledger:main Mar 4, 2024
14 checks passed

nguyer deleted the eventstreams branch March 4, 2024 15:44

peterbroadhurst mentioned this pull request Jun 23, 2024

Migration to v1.3.0 can cause missed events when contract listeners are listening from latest #1531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use separate event stream per namespace #1388

Use separate event stream per namespace #1388

nguyer commented Aug 10, 2023 •

edited

Loading

codecov-commenter commented Aug 30, 2023 •

edited

Loading

nguyer commented Aug 31, 2023

peterbroadhurst left a comment

peterbroadhurst Aug 31, 2023

peterbroadhurst Aug 31, 2023

peterbroadhurst Aug 31, 2023

peterbroadhurst Aug 31, 2023

nguyer Aug 31, 2023

nguyer Nov 15, 2023

peterbroadhurst Feb 15, 2024

nguyer Feb 21, 2024

peterbroadhurst Aug 31, 2023

peterbroadhurst Aug 31, 2023

peterbroadhurst Aug 31, 2023

peterbroadhurst Aug 31, 2023

nguyer Aug 31, 2023

nguyer Nov 15, 2023

peterbroadhurst Aug 31, 2023

nguyer Aug 31, 2023

peterbroadhurst left a comment

peterbroadhurst Feb 15, 2024

peterbroadhurst Feb 15, 2024

nguyer Feb 21, 2024

peterbroadhurst Feb 15, 2024

peterbroadhurst Feb 15, 2024

nguyer Feb 21, 2024

peterbroadhurst Feb 15, 2024

peterbroadhurst Feb 15, 2024

nguyer Feb 21, 2024

peterbroadhurst Feb 15, 2024

nguyer commented Feb 21, 2024

Use separate event stream per namespace #1388

Use separate event stream per namespace #1388

Conversation

nguyer commented Aug 10, 2023 • edited Loading

codecov-commenter commented Aug 30, 2023 • edited Loading

Codecov Report

nguyer commented Aug 31, 2023

peterbroadhurst 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

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

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

peterbroadhurst 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

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

nguyer commented Feb 21, 2024

nguyer commented Aug 10, 2023 •

edited

Loading

codecov-commenter commented Aug 30, 2023 •

edited

Loading