Batch confirm messages, and update events to contain topic+tag #600

peterbroadhurst · 2022-03-14T17:48:00Z

Currently there is no clean way to query messages on a given topic in the same order that they were dispatched to your application.

More worryingly, we imply that the that you might be able to use the confirmed timestamp of the message as a substitute, as currently it's unique for each message. The problem with this is that timestamps are not guaranteed to only move forwards.

The only object in FireFly that is guaranteed to have a sequence you can rely upon is events, and these are delivered to your application with the message in-line in the case of a message_confirmed event. We just added ?fetchreferences (thanks @shorsher) on the /events resource collection, so that's an important piece of the puzzle. So with the extra change in this PR, if you want to query messages on a topic in the order they happened you just do:

/api/v1/namespaces/{NS}/events?type=message_confirmed&fetchreferences&topic=MYTOPIC
- Reverse sequence is the default sort - so you can add &sort=sequence for ascending

Note that this does not return you the data (there is no 2nd tier ?fetchdata option). Adding this needs extra discussion as we'd drawn a line in the sand of only doing one level of auto-fetching for you. However, the convenience of fetchdata might be the exception that proves the rule 🤔

In a PR chain with #599

Adds topic as am indexed field on events (64 chars)
If there are multiple topics in a message to generates multiple message_confirmed events.
Batch confirms messages, so multiple messages have the same confirmed timestamp
Updates E2E test to apply the best practice described above
Topics are assigned to all events, including non-message events (table below)

This has two benefits:

Performance increase due to being able to do a blanket set of confirmed to a single timestamp in the aggregator.
The ability to query all history for a topic cleanly in applications, without implicitly relying on positively increasing timestamps, and being confident its the same order that the confirmations happened in and your application was delivered the events.

Topic events

Event	Topic	Comment
`message_confirmed`	Message Topic	The key use case of filtering on topic
`message_rejected`	Message Topic	"
`blockchain_event_received`	`ff_batch_pin` for the built-in batch pin listener. Topic from the listener if specified. Listener ID as string otherwise	Setting a topic on the blockchain listener allows grouping of multiple events into a common stream
`token_pool_confirmed`	Token pool ID as string	Allows querying of all events on a token pool
`token_transfer_confirmed`	Token pool ID as string	"
`token_transfer_op_failed`	Token pool ID as string	"
`token_approval_confirmed`	Token pool ID as string	"
`namespace_confirmed`	`ff_definitions`	Catch all topic for FireFly system definitions
`datatype_confirmed`	`ff_definitions`	"
`identity_confirmed`	`ff_definitions`	"
`identity_updated`	`ff_definitions`	"
`contract_interface_confirmed`	`ff_definitions`	"
`contract_api_confirmed`	`ff_definitions`	"

codecov-commenter · 2022-03-14T18:48:04Z

Codecov Report

Merging #600 (169b71f) into main (0598d9f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #600   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          305       305           
  Lines        18297     18357   +60     
=========================================
+ Hits         18297     18357   +60

Impacted Files	Coverage Δ
pkg/fftypes/contract_listener.go	`100.00% <ø> (ø)`
internal/batch/batch_processor.go	`100.00% <100.00%> (ø)`
internal/config/config.go	`100.00% <100.00%> (ø)`
...ternal/database/sqlcommon/contractlisteners_sql.go	`100.00% <100.00%> (ø)`
internal/database/sqlcommon/event_sql.go	`100.00% <100.00%> (ø)`
...ternal/definitions/definition_handler_contracts.go	`100.00% <100.00%> (ø)`
...nternal/definitions/definition_handler_datatype.go	`100.00% <100.00%> (ø)`
...l/definitions/definition_handler_identity_claim.go	`100.00% <100.00%> (ø)`
.../definitions/definition_handler_identity_update.go	`100.00% <100.00%> (ø)`
...ternal/definitions/definition_handler_namespace.go	`100.00% <100.00%> (ø)`
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0598d9f...169b71f. Read the comment docs.

…e messages to dispatched Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

awrichar · 2022-03-15T19:59:45Z

internal/batch/batch_processor.go

-					if err := bp.database.InsertEvent(ctx, event); err != nil {
-						return err
+					for _, topic := range msg.Header.Topics {
+						// One event per topic


Nothing inherently wrong with this, but it's one of those things that will need to be clearly release-noted. For any apps that use multiple topics, they'll suddenly get more events with copies of the same data - so it just reinforces the need for those apps to have idempotent data processing.

awrichar

There are 2 weird things about this PR:

We have two new fields ("topic" and "tag") that are unused for the majority of our event types (although they are used with the most common event type "message_confirmed")
Messages with multiple topics will now get multiple events emitted which all contain the same message

But after staring and thinking for a while, I'm on the same page - I think the benefits (being able to query messages in order and being able to batch update the messages upon confirmation) outweigh the weirdness. I haven't been able to think of any alternate proposal that's better on those two counts. Since the weird things are mostly only visible to "advanced" users and are fairly easy to explain/design around, I'm on board.

awrichar · 2022-03-16T05:53:49Z

Alright the one other alternative that occurs to me is adding a special query param to /messages that would indicate "fetch confirmed messages only, and in the order they were confirmed, by doing a join with the events table".

In theory, that would eliminate the need to put topic and tag on the events table...I think.

While "topic" is clearly useful in terms of ordering, "tag" is not quite as justifiable to be duplicated this way. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar · 2022-03-16T20:24:13Z

Pushed a proposal to pull "tag" back out, as I think it's harder to justify here. If we can take this a step further and make "topic" meaningful for events other than messages, then it feels like a more complete change.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Removing approval as it sounds like this is going to continue evolving a bit longer

peterbroadhurst · 2022-03-19T03:41:11Z

Further updates being added:

Allowing a custom topic to specified on the blockchain listener, and passed to the FireFly event emitted
Addressing an existing TODO in the code to add a blockchain listener lookup cache to event manager

…ents Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

nguyer · 2022-03-19T14:44:05Z

internal/events/blockchain_event.go

+func (em *eventManager) getChainListenerByIDCached(ctx context.Context, id *fftypes.UUID) (*fftypes.ContractListener, error) {
+	return em.getChainListenerCached(fmt.Sprintf("id:%s", id), func() (*fftypes.ContractListener, error) {
+		return em.database.GetContractListenerByID(ctx, id)
+	})
+}


Glad to see this can take advantage of caching now

peterbroadhurst requested review from awrichar, nguyer and nickgaski as code owners March 14, 2022 17:48

peterbroadhurst force-pushed the batch-confirm branch from cb63884 to 9c3bf75 Compare March 14, 2022 18:44

peterbroadhurst mentioned this pull request Mar 15, 2022

IPFS Download Workers #601

Closed

peterbroadhurst added 3 commits March 15, 2022 12:22

Add tag and topic to events, reconcile event filters, and batch updat…

59345f9

…e messages to dispatched Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

See if it is possible to reduce latency without affecting throughput

cf60eef

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Log error for parse failure on batch download

90948ee

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst force-pushed the batch-confirm branch from dae5d2e to 90948ee Compare March 15, 2022 16:22

awrichar reviewed Mar 15, 2022

View reviewed changes

awrichar previously approved these changes Mar 15, 2022

View reviewed changes

awrichar force-pushed the batch-confirm branch from e1fa8a0 to 9708ff1 Compare March 16, 2022 19:45

Pull "tag" back out of events

57ba622

While "topic" is clearly useful in terms of ordering, "tag" is not quite as justifiable to be duplicated this way. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar force-pushed the batch-confirm branch from 9708ff1 to 57ba622 Compare March 16, 2022 20:16

peterbroadhurst added 2 commits March 18, 2022 14:44

Merge branch 'main' of github.com:hyperledger/firefly into batch-confirm

def3c3d

Add topics to all events and merge with main

e768bfd

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Propagate topic from chain listener to event, and set topic on all ev…

169b71f

…ents Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

nguyer approved these changes Mar 19, 2022

View reviewed changes

peterbroadhurst merged commit 224ea30 into hyperledger:main Mar 20, 2022

peterbroadhurst deleted the batch-confirm branch March 20, 2022 01:49

awrichar mentioned this pull request Aug 9, 2022

Token pool definitions are all broadcast on a single topic #597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Batch confirm messages, and update events to contain topic+tag #600

Batch confirm messages, and update events to contain topic+tag #600

Uh oh!

peterbroadhurst commented Mar 14, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Mar 14, 2022 •

edited

Loading

Uh oh!

awrichar Mar 15, 2022

Uh oh!

awrichar left a comment

Uh oh!

awrichar commented Mar 16, 2022

Uh oh!

awrichar commented Mar 16, 2022

Uh oh!

peterbroadhurst commented Mar 19, 2022 •

edited

Loading

Uh oh!

nguyer Mar 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Batch confirm messages, and update events to contain topic+tag #600

Batch confirm messages, and update events to contain topic+tag #600

Uh oh!

Conversation

peterbroadhurst commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Topic events

Uh oh!

codecov-commenter commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

awrichar Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

awrichar left a comment

Choose a reason for hiding this comment

Uh oh!

awrichar commented Mar 16, 2022

Uh oh!

awrichar commented Mar 16, 2022

Uh oh!

peterbroadhurst commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nguyer Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peterbroadhurst commented Mar 14, 2022 •

edited

Loading

codecov-commenter commented Mar 14, 2022 •

edited

Loading

peterbroadhurst commented Mar 19, 2022 •

edited

Loading