Stream: fhircast-github
Topic: fhircast-docs / PR #331 STU2-Improved-Diagrams
Github Notifications (FHIRcast) (Aug 18 2020 at 13:01):
NiklasSvenzen opened PR #331 from STU2-Improved-Diagrams to master:
FHIR-25650 - Diagrams for each Section
FHIR-25837 - Destruction of websockets
Github Notifications (FHIRcast) (Aug 18 2020 at 13:01):
NiklasSvenzen requested isaacvetter for a review on PR #331.
Github Notifications (FHIRcast) (Aug 18 2020 at 13:01):
NiklasSvenzen requested isaacvetter and wmaethner for a review on PR #331.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:24):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:24):
wmaethner created PR Review Comment:
I think we talked about moving these source files into a subdirectory under img. Maybe src? So the path would be docs/img/src/ErrorSequence.drawio. This would clean up the img folder a good amount.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:33):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:35):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:35):
wmaethner created PR Review Comment:
The swim lanes on this diagram are pretty long considering there are only two interactions. Could they be shortened?
Github Notifications (FHIRcast) (Sep 01 2020 at 12:40):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:40):
wmaethner created PR Review Comment:
Would it be worth having one of the apps respond instead with a 202 to show that received the notification but need more time to actually perform the change? I think this would basically mean moving the "internal context change" lower and maybe a waiting period (arrow that loops back itself?) saying "validating event notification" or something similar.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:41):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:41):
wmaethner created PR Review Comment:
The Webhook and Websocket headers overlap eachother.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:42):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:42):
wmaethner created PR Review Comment:
If the X has color, should the lightning bolts as well? Or do we want to go completely colorless?
Github Notifications (FHIRcast) (Sep 01 2020 at 12:46):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Sep 01 2020 at 12:46):
wmaethner created PR Review Comment:
This isn't specific to the diagram, but it made me wonder (which wouldn't have happened if not for the diagrams so good job :+1:), if say the first subscriber has an error which would generate a sync error, does the Hub not send the original notification to the rest of the subscribers? In the case where the subscriber's http response shows an error maybe this would be possible, but if they respond with a 202 then send the Hub a sync error after, then there would be no way for the Hub to delay meaning all subscribers would receive the original notification. That feels fine to me, as long as the Hub additionally sends the sync error, but I think both cases should be consistent. Either we send the notification and a sync error or we don't send the notification (I am leaning towards the former).
Github Notifications (FHIRcast) (Sep 01 2020 at 14:05):
wmaethner created PR Review Comment:
Actually I think I misread the diagram. The event notifications are sent to both subscribers then one causes the error which generates the sync error to the other. That looks good to me.
The lanes are a bit off though, the box for the webhook app doesn't line up with the dotted line.
Github Notifications (FHIRcast) (Sep 01 2020 at 14:05):
wmaethner submitted PR Review.
Github Notifications (FHIRcast) (Oct 13 2020 at 12:55):
NiklasSvenzen submitted PR Review.
Github Notifications (FHIRcast) (Oct 13 2020 at 12:55):
NiklasSvenzen created PR Review Comment:
Yes, will do
Github Notifications (FHIRcast) (Oct 13 2020 at 12:55):
NiklasSvenzen submitted PR Review.
Github Notifications (FHIRcast) (Oct 13 2020 at 12:55):
NiklasSvenzen created PR Review Comment:
Yes, makes sense, will try to move them
Github Notifications (FHIRcast) (Oct 14 2020 at 08:34):
NiklasSvenzen submitted PR Review.
Github Notifications (FHIRcast) (Oct 14 2020 at 08:34):
NiklasSvenzen created PR Review Comment:
Added color to the bolts
Github Notifications (FHIRcast) (Oct 14 2020 at 08:35):
NiklasSvenzen submitted PR Review.
Github Notifications (FHIRcast) (Oct 14 2020 at 08:35):
NiklasSvenzen created PR Review Comment:
Realigned the box and dotted line
Github Notifications (FHIRcast) (Oct 14 2020 at 08:36):
NiklasSvenzen submitted PR Review.
Github Notifications (FHIRcast) (Oct 14 2020 at 08:36):
NiklasSvenzen created PR Review Comment:
Updated diagram with more space
Github Notifications (FHIRcast) (Oct 14 2020 at 08:44):
NiklasSvenzen submitted PR Review.
Github Notifications (FHIRcast) (Oct 14 2020 at 08:44):
NiklasSvenzen created PR Review Comment:
Good idea, diagram updated
Github Notifications (FHIRcast) (Oct 14 2020 at 08:48):
NiklasSvenzen updated PR #331 from STU2-Improved-Diagrams to master:
FHIR-25650 - Diagrams for each Section
FHIR-25837 - Destruction of websockets
Github Notifications (FHIRcast) (Oct 14 2020 at 08:56):
NiklasSvenzen requested isaacvetter and wmaethner for a review on PR #331.
Github Notifications (FHIRcast) (Oct 26 2020 at 16:36):
isaacvetter updated PR #331 from STU2-Improved-Diagrams to master:
FHIR-25650 - Diagrams for each Section
FHIR-25837 - Destruction of websockets
Github Notifications (FHIRcast) (Oct 26 2020 at 16:38):
isaacvetter submitted PR Review.
Github Notifications (FHIRcast) (Nov 24 2020 at 14:33):
NiklasSvenzen merged PR #331.
Last updated: Apr 12 2022 at 19:14 UTC
