feat: #1869 Design for reliable notifications on finalization of a transfer #288

mdebarros · 2020-12-10T10:29:48Z

Link to story: mojaloop/project#1869
Link to working branch: https://github.com/mojaloop/documentation/tree/feature/%231869Designforreliablenotificationsonfinalizationofatransfer/mojaloop-technical-overview/notifications

…Image exporter

lewisdaly

This looks great @mdebarros (and team).

A couple thoughts in addition to my inline comments:

I would like to see examples and schemas of the NotifyCMD for other transports
Related to the above, requiring the notification command handler is a noble goal, and to do this properly, we need to make sure we have a good understanding of the requirements so we can design the messages from the Notification Evt Handler with the appropriate level of detail.

mojaloop-technical-overview/notifications/README.md

mojaloop-technical-overview/notifications/assets/sequence/seq-notify-v2-1.0.0.plantuml

mojaloop-technical-overview/notifications/README.md

…notify-v2-1.0.0.plantuml Co-authored-by: Lewis Daly <[email protected]>

Co-authored-by: Lewis Daly <[email protected]>

mdebarros · 2021-01-06T13:43:22Z

Thanks @lewisdaly for the feedback!

See my inline comments below:

This looks great @mdebarros (and team).

A couple thoughts in addition to my inline comments:

I would like to see examples and schemas of the NotifyCMD for other transports
I would prefer to create the schema once the overall architecture approach has been given the DA node. Let me know if you feel that the definition for this schema is vital to achieving said nod.

Related to the above, requiring the notification command handler is a noble goal, and to do this properly, we need to make sure we have a good understanding of the requirements so we can design the messages from the Notification Evt Handler with the appropriate level of detail.
Agreed. That is exactly the kind of feedback I hope to receive from the DA. Validating the documented requirements, and also identifying any that I may have missed.

…sonfinalizationofatransfer

…re/#1869Designforreliablenotificationsonfinalizationofatransfer

1. Added schemas for both types of Domain and Command events 2. Updated SD to show validation of transport-types and content-types 3. Fixed typos

…sonfinalizationofatransfer

…forreliablenotificationsonfinalizationofatransfer

…onofatransfer' of github.com:mojaloop/documentation into feature/#1869Designforreliablenotificationsonfinalizationofatransfer

…gic, and recovery from failures

lewisdaly

A few other thoughts and questions for your consideration @mdebarros

mojaloop-technical-overview/notifications/assets/sequence/seq-notify-v2-1.0.0.plantuml

lewisdaly · 2021-03-31T11:23:24Z

mojaloop-technical-overview/notifications/README.md

+
+| State | Description | Notes |
+| --- | --- | --- |
+| received | Indicates that the NotifyCmd event has been received by the Notification Cmd Handler. |  |


Is there a better word than received here? At first glance through the diagrams I assumed received meant "received by the intended recipient", as opposed to "received by the Notification Cmd Handler"

How about pending?

That's better, although pending and in-progress have similar meanings then. Maybe we could change received to pending and in-progress to processing?

Agreed. I will make this change.

lewisdaly · 2021-03-31T11:25:31Z

mojaloop-technical-overview/notifications/assets/schemas/eventNotifyReady.schema.json

+                "NotificationCommands"
+            ]
+        },
+        "msgPartition": {


Is this required? If so, should it have a different type than null?

Yup. It's optionally, so I can remove it from the example. It allows one to override the Kafka partition.

It allows the partitioning to be managed at the application level if required. It was a pretty useful attribute in the Perf PoC. It can be removed.

lewisdaly · 2021-03-31T11:27:41Z

mojaloop-technical-overview/notifications/assets/schemas/eventNotifyReady.schema.json

+                        "endpoint"
+                    ],
+                    "properties": {
+                        "endpoint": {


The endpoints + params templating looks like it adds a lot of complexity. Does it need to be a part of this? Or could it be handled by a different component? Why can't the ml-api-adapter just spit out an already resolved endpoint?

It can. Both are supported.

The original idea was to pass additional attributes into the Notification Engine, and also reduce duplicate fields/data by using a templated URI. Using a template in the URI is optional.

It's also possible to still pass parameters even without a template URI as they will be ignored, and thus pass additional attributes to the Notification Cmd Handler which may be useful for future customization.

Ok. Yeah I'm not too fussed about this, but templating looks like it adds more complexity, and muddies the water a little bit about which component is responsible for generating valid URLs

Fair enough. It's worth pointing out that it is not difficult to add or remove this functionality.

However, I still believe it's better to not duplicate data....so I am still a fan of the template approach :D

lewisdaly · 2021-03-31T11:29:29Z

mojaloop-technical-overview/notifications/assets/schemas/eventNotifyReady.schema.json

+            },
+            "additionalProperties": false
+        },
+        "traceInfo": {


Should trace info also be duplicated in the headers? Or will the Notification Event Handler take care of this for us?

That would be handled by the Cmd Handler. It would re-generate the tracing headers with the most recent trace meta-state before sending out the notification.

There is also an argument for it to be handled by the Callback Handler instead. I am leaning towards the first as it will increase the breadth of the trace (i.e. more components that are included in the trace).

Ok, I think I understand. I'm a little wary of messages containing redundant parameters (ie. both in the payload.headers and traceInfo fields). Could there be a case where we could have misaligned trace metdata between those 2 fields?

Ok, I think I understand. I'm a little wary of messages containing redundant parameters (ie. both in the payload.headers and traceInfo fields). Could there be a case where we could have misaligned trace metdata between those 2 fields?

The headers you see in the example are originally from what was received by the Hub, as such they do NOT reflect the current trace state that is stored within the trace meta-data portion of the message. The propagation of incoming request headers is managed by the API Adapter, so it would be responsible for including or deleting them...the idea being that the incoming request would be "persisted" as-is without any modifications. The outgoing headers are generally mapped from the incoming headers, but certain headers will be deleted or replaced...in this case, the trace headers will be replaced before sending the notification, so I do not see this as an issue.

Cool, thanks for clearing that up.

lewisdaly · 2021-03-31T11:51:33Z

mojaloop-technical-overview/notifications/README.md

+
+| Components | Description | APIs | Notes |
+| --- | --- | --- | --- |
+| ML-Adapter Callback Handler | This component understands the context and semantics of the FSPIOP Specification. Consumes existing Notification (internal) events, then interprets (in context of Mojaloop use-cases) those events into an appropriate event message to some explicit recipient. Also handles feedback loop and compensating actions (defined by configurable rules) from Delivery Reports. It must also resolve any notification details required (i.e. URLs, transport type, etc) by querying the Central-Service's Admin API. This component is pluggable, and the design supports the possibility of several Callback Handlers existing for several specific transports or alternatively to support different context languages like ISO 20022. | N/A | This component is part of the "ML-API-Adapter" |


Could there be a need to de-duplicate notification events from central services before they get put on the ml-adapter's notification event que?

I am not aware of such a requirement, but this is possible. It is also possible to use Kafka to handle this with its "compaction" feature. Perhaps something to be raised in the next DA or Ref Arch meeting?

If it's not a requirement, then lets not make more work for ourselves ;). I guess maybe we need to be clear (or maybe we are clear in some documentation somewhere that I'm not aware of) that there is never a case where the central-ledger could create 2 duplicate notification events.

mojaloop-technical-overview/notifications/README.md

Added initial notifications documentation readme and asset placeholders

15c9513

mdebarros self-assigned this Dec 10, 2020

mdebarros requested a review from elnyry-sam-k December 10, 2020 10:31

mdebarros assigned shashi165 Dec 10, 2020

mdebarros mentioned this pull request Dec 10, 2020

Design for reliable notifications on finalization of a transfer mojaloop/project#1869

Closed

20 tasks

mdebarros added 9 commits December 11, 2020 16:01

updated notification-engine requirements & added example

d1560e3

Notification-engine updates

9dce0a6

updates to notification engine

f3db573

fixed sequencing on notification design

a5297ae

updated notify design svg, and added a gitignore for VSCode Plantuml …

9535332

…Image exporter

updated notification design

bed56c5

updated notification design

f88dcb3

updated notification design

5fa895e

updated notification design

9bd9287

mdebarros mentioned this pull request Jan 4, 2021

Design for reliable notifications on finalisation of a transfer mojaloop/design-authority-project#74

Closed

3 tasks

lewisdaly reviewed Jan 4, 2021

View reviewed changes

mdebarros and others added 7 commits January 6, 2021 11:09

updates to notification design docs

e5efef6

updates to notification design docs

729e264

updates to notification design docs

696ee1a

updates to notification design docs

a372cb7

updates to notification design docs

f0012a3

Update mojaloop-technical-overview/notifications/assets/sequence/seq-…

a0ffd8c

…notify-v2-1.0.0.plantuml Co-authored-by: Lewis Daly <[email protected]>

Update mojaloop-technical-overview/notifications/README.md

14606b4

Co-authored-by: Lewis Daly <[email protected]>

mdebarros added 6 commits January 27, 2021 10:46

Merge branch 'master' into feature/#1869Designforreliablenotification…

34366ef

…sonfinalizationofatransfer

Merge branch 'master' of github.com:mojaloop/documentation into featu…

ec679fd

…re/#1869Designforreliablenotificationsonfinalizationofatransfer

Updated to notification reliable design

45b90fb

1. Added schemas for both types of Domain and Command events 2. Updated SD to show validation of transport-types and content-types 3. Fixed typos

Updates to package-lock and bug fix to plantuml_diff script

2a9c365

Updates to SD for notification

7b6881d

Updated notification sd flow for alt path

bc9c8fc

mdebarros added 2 commits March 9, 2021 13:48

Added example for multiple transports

16b6bec

Updated description for notification cmd handler

2bced12

mdebarros closed this Mar 9, 2021

Fixes for circleci

fbe27aa

mdebarros reopened this Mar 9, 2021

mdebarros added 17 commits March 9, 2021 18:21

fixes for circleci config

1220fbd

Merge branch 'master' into feature/#1869Designforreliablenotification…

e6155bd

…sonfinalizationofatransfer

Updates to notif rel end2end arch diagrams

7674636

Merge remote-tracking branch 'origin/master' into feature/#1869Design…

1ce400f

…forreliablenotificationsonfinalizationofatransfer

Merge branch 'feature/#1869Designforreliablenotificationsonfinalizati…

6ccd3ca

…onofatransfer' of github.com:mojaloop/documentation into feature/#1869Designforreliablenotificationsonfinalizationofatransfer

Removed unused folder

08f004c

Updates to notification v2 e2e arch diagram

6860051

Updated notification v2 e2e arch diagram

91b6d40

updated notify design v2

175edb9

updated notify design v2

c757ab9

updates to notification design v2 - included more detail on re-try lo…

c0ea05c

…gic, and recovery from failures

updates to notification design v2 - included more detail on re-try lo…

61c9ea3

…gic, and recovery from failures

updates to notification design v2 - included more detail on re-try lo…

a1b4260

…gic, and recovery from failures

updates to notification design v2 - included more detail on re-try lo…

a471c42

…gic, and recovery from failures

updates to notification design v2 - included more detail on re-try lo…

40daafa

…gic, and recovery from failures

updates to notification design v2 - included more detail on re-try lo…

60d1a19

…gic, and recovery from failures

spelling fixes

af3a6f6

lewisdaly reviewed Mar 31, 2021

View reviewed changes

Updated mojaloop-simulator to fix subid issue - mojaloop/project#2103

42e2c80

mdebarros marked this pull request as ready for review April 7, 2021 13:58

mdebarros requested review from kjw000, millerabel and simeonoriko as code owners April 7, 2021 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: #1869 Design for reliable notifications on finalization of a transfer #288

feat: #1869 Design for reliable notifications on finalization of a transfer #288

mdebarros commented Dec 10, 2020 •

edited

Loading

lewisdaly left a comment

mdebarros commented Jan 6, 2021

lewisdaly left a comment

lewisdaly Mar 31, 2021

mdebarros Mar 31, 2021

lewisdaly Apr 6, 2021

mdebarros Apr 7, 2021

lewisdaly Mar 31, 2021

mdebarros Mar 31, 2021

lewisdaly Mar 31, 2021

mdebarros Mar 31, 2021 •

edited

Loading

lewisdaly Apr 6, 2021

mdebarros Apr 7, 2021

lewisdaly Mar 31, 2021

mdebarros Mar 31, 2021 •

edited

Loading

lewisdaly Apr 6, 2021 •

edited

Loading

mdebarros Apr 7, 2021 •

edited

Loading

lewisdaly Apr 13, 2021

lewisdaly Mar 31, 2021

mdebarros Mar 31, 2021

lewisdaly Apr 6, 2021

feat: #1869 Design for reliable notifications on finalization of a transfer #288

Are you sure you want to change the base?

feat: #1869 Design for reliable notifications on finalization of a transfer #288

Conversation

mdebarros commented Dec 10, 2020 • edited Loading

lewisdaly left a comment

Choose a reason for hiding this comment

mdebarros commented Jan 6, 2021

lewisdaly 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

mdebarros Mar 31, 2021 • 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

mdebarros Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

lewisdaly Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

mdebarros Apr 7, 2021 • 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

mdebarros commented Dec 10, 2020 •

edited

Loading

mdebarros Mar 31, 2021 •

edited

Loading

mdebarros Mar 31, 2021 •

edited

Loading

lewisdaly Apr 6, 2021 •

edited

Loading

mdebarros Apr 7, 2021 •

edited

Loading