IHE / DEV.SDPi

IHE Devices Service-oriented Device Point-of-care Interoperability (SDPi) profiles supplement specification materials and related tooling.
9 stars 1 forks source link

#152 Add discovery proxy transactions scaffold and actor description. #300

Closed d-gregorczyk closed 2 months ago

d-gregorczyk commented 4 months ago

☑ Mandatory Tasks

The following aspects have been respected by the pull request assignee and at least one reviewer:

ToddCooper commented 3 months ago

SDPi Friday 2024.07.26 Discussion - See update notes for this issue.

ToddCooper commented 3 months ago

SDPi Friday 2024.08.16 Review -

  1. Actor & Transaction Names - discussion verified that "discovery proxy" could be improved, such as "Network Discovery Manager" or "Network Presence Registry"
  2. Use Case Supporting PnT + DP: Recognized that there is a general assumption that PnT happens _ DP is a purely architectural / technical capability with no clinician visibility; Decision to add a technical pre-condition to 1:C.3.4 for discovered and connected
  3. Reviewers: Add Marlene G. & Ken F. ... others invited
ToddCooper commented 2 months ago

@d-gregorczyk -- Updated the TF-1 content + refactored stuff + fixed a few things along the way. Please review. ALSO note that the TF-1 option text refers to the DEV-46 & DEV-47 transactions; however, I got the wrong reference and it renders to the TF-2 sections and not to the "DEV-46" text and link. Is there a reference that renders this text but links to the TF-2 section? Or do they always go back to the TF-0 Transactions listing?

At this point, other than the above issue, Discovery Proxy should be ready for a final review.

ToddCooper commented 2 months ago

@d-gregorczyk - should we also add a note to the TF-1 DP Option section that it is modeled after the WS-Discovery Proxy? If so, could you add it OR let me know the specific reference to include?

ToddCooper commented 2 months ago

TF-1 Review Comments from @d-gregorczyk :

  • The "shall" sentences you dropped in the text won't be exported to the JSON as they are not bundled as sdpi_requirement blocks.
  • You named "transaction" in lowercase, but "Actor" in capital - is that on purpose?
  • Is the discovery proxy supposed to be "SOMDS Discovery Proxy" or just "Discovery Proxy" - it is currently mixed up in texts and pictures; e.g. see Figure 1:10.1-1

@ToddCooper will incorporate updates per these comments.

ToddCooper commented 2 months ago

IHE DCC Discovery Proxy Actor & Transactions Review Comments (2024.08.27):

The IHE Domain Coordination Committee (DCC) did an initial review of the proposed Discovery Proxy Actor & related DEV-46 & DEV=47 transactions.

Here is a summary of their comments:

As a final note and as feared, there was yet more discussion about the use of "SOMDS", an issue that we all remember well after hours (literally) of group discussion in 2023 about all the alternatives that Kevin O. proposed and not finding any alternative that wasn't crazy long & "gross" or that made no sense to our community- both implementers and users. Having something "SOMDS" meant people had to ask what in the word THAT was, which they would have had to do anyway with a longer name. That said, this issue may come up yet again in September, though we have now published a few versions for Trial Implementation purposes (like what is currently published on profiles.ihe.net).

Note that @MaryLJ will work with @ToddCooper at an initial rewording draft. This can be included in the Github release 1.4.0 candidate release (for comment) and hopefully reviewed and approved at the September DCC meting.

ToddCooper commented 2 months ago

2024.08.28 Update -

  1. Edits related to DCC discussion + @d-gregorczyk TF-1 review comments are now bundled into THIS BUILD
  2. See "commit" notes above (detailed sections) for more information
  3. Renamed the option to Managed Discovery Option - OK?
  4. Renamed the DEV-46 Transaction to "Update Network Presence" (dropped Managed .... Metadata )- OK?
  5. Kept Retrieve Network Presence Metadata BUT could also drop "metadata" from DEV-47 for simplicity as well ... it is a bit redundant
  6. Added "Heartbeat" requirement in a very generic way to the option discussion - this should be sufficient but please give it a review
  7. All text should be reviewed - I have not done a "final" pass on that yet but would appreciate your fresh eyes @JavierEspina !!!
  8. Additional transaction Scope / summary updates may be required ... have asked Mary J. for clarification

Finally, @d-gregorczyk : The message sections in TF-2A are confusing to this old man, especially some in the DEV-47 section; could we do a review of that either Thursday or in the SDPi Friday discussion?

ToddCooper commented 2 months ago

2024.08.29 Update -

  1. All issues identified have been addressed [in the latest push] (https://github.com/IHE/DEV.SDPi/actions/runs/10620232135/artifacts/1871250898) ... see the detailed notes
  2. Additional requirements metadata have been added to the R1021 and passed through to the JSON export ... woohoo!
  3. Changed DEV-47 Bye() message arrow direction on sequence diagram

At this point, a final pre-integration review should be ready.

ToddCooper commented 2 months ago

@d-gregorczyk QUESTIONS:

  1. DEV-47 has an "Opt" section for CONSUMERs to receive Hello() and Bye() messages; however, this is not clearly discussed in the subsequent text; Doesn't the CONSUMER have to "subscribe" to these updates? The current text appears that it just happens perhaps based on matches from previous discovery queries ... ???
  2. "endpoint metadata" is mentioned a few times but not clearly described anywhere; does this include a PROVIDER's discovery scopes metadata? Other information or simply the UID? For example, does the ProbeMatch() provide a PROVIDER's discovery scope info also or just the UID and Transport Address?
ToddCooper commented 2 months ago

SDPi Friday 2024.08.30 Review - Group reviewed the changes related to the comments above.
Only changes were related to DEV-47

  1. Scope wording change
  2. Update the PDEV-47 PUML Bye( Provider UID )