-
Notifications
You must be signed in to change notification settings - Fork 0
Add ADR for surfacing Primo CDI records in results #219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Why these changes are being introduced: The proposed unified search interface would display records from Primo CDI (via Primo Search API) and Alma (via TIMDEX API) in the same results list. TIMDEX UI does not currently have a means to combine results from multiple APIs in this way. Relevant ticket(s): N/A How this addresses that need: This adds an ADR that outlines a proposed solution to this problem, by introducing a search orchestration layer that will handle API calls and results normalization. Side effects of this change: There are additional decisions to be made around the architecture of the search orchestrator, such as how to manage relevance normalization. These decisions are noted in the ADR and will be explored in future ADRs.
Pull Request Test Coverage Report for Build 16807824291Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in a position right now to indicate a vote or approval/change request on this text - I think we're in a discussion phase prior to concluding anything, so that's what I'm focusing on for now...
- While I agree with the prospect of building an orchestrator and proceeding with a simple interleaving logic for merging the two sets of results, I think formally I'd like this document to describe multiple options prior to stating the decision we're going with. The one clear alternative I see right now would be to harvest CDI records into TIMDEX, although I think others are implicitly referenced (continuing to display the two results separately without merging, building an external orchestrator in Python, building an internal orchestrator in Rails).
- The sequence diagram feels like it could be cleaner in its placement of the "User submits search query" phase - maybe that comes in from a User participant to the frontend, which then delegates the search to both TACOS and the orchestrator?
I'm also not sure whether any of the interactions in the diagram are blocking each other - it feels like the basic phases are meant to be:
-
User submits search query to TIMDEX UI
-
TIMDEX UI starts processing, doing internal things
-
TIMDEX UI issues the search to three targets: TACOS, TIMDEX API, and the CDI (these later two may be managed by the Search Orchestrator, while some other model in the application handled TACOS?)
-
As responses are received from the three external systems, some (TACOS) are displayed immediately, in parallel to the other streams. The Search Orchestrator, however, waits to respond until it has received responses from both TIMDEX API and the CDI. It interleaves those responses on its own before returning them for rendering (which is also a difference to TACOS, whose response is not modified in any way)
-
As the responses are processed in step 4, they are added to the DOM and shown to the user.
I think this is pretty much the same sequence you're proposing - so I think I'm ultimately wanting to have a diagram that makes clear what the dependencies are, and what might be happening in parallel. This might be my misunderstanding how to read a sequence diagram, however.
- I'm not sure whether the TIMDEX UI is the most appropriate place to consider how to facilitate computation access to CDI records. While it is definitely a concern for the Libraries, I think it makes sense to discuss that use case elsewhere? The choice to build an orchestrator or adopt a separate approach feels completely separate from that use case (although I might be missing something, so I'm not yet at a point of blocking it being here)
I think those are the thoughts which are most prominently occurring to me at the moment. I may have other comments in the next day or so, prior to our discussing this as a team.
Thanks for putting this together! I appreciate the work it took to assemble.
@matt-bernhardt Thank you so much for the prompt and thoughtful feedback! These are my initial thoughts as I mull it over this morning.
Love this idea, especially because the inclusion of an 'alternative options' list is consistent with how we write more complex ADRs. I'll restructure the document to formalize that.
Great point. As currently diagrammed, it looks like the user is submitting the query directly to the orchestrator. What I meant to convey was, as you said, the query handoff from UI to orchestrator.
Yeah, I see what you mean. I tried to indicate asynchronous interactions using dotted lines, but generally speaking, I wasn't sure about the clarity of the sequence diagram, so it's helpful to have that instinct affirmed. I wonder if a flow diagram would be more readable/useful?
When I wrote that, I was thinking less about TIMDEX UI and more about the whole TIMDEX ecosystem. Ostensibly, TIMDEX API is intended to facilitate computational access, and integrating CDI records into TIMDEX feels like an opportunity to consider if we can provide such access to CDI. However, I agree that computational access to CDI is out of scope for this ADR. As I consider it further, I might argue that it's out of scope of TIMDEX altogether, since TIMDEX API is primarily focused on our local collections. (Though, that has changed somewhat since GDT.) |
Why these changes are being introduced:
The proposed unified search interface would display records from Primo CDI (via Primo Search API) and Alma (via TIMDEX API) in the same results list. TIMDEX UI does not currently have a means to combine results from multiple APIs in this way.
Relevant ticket(s):
N/A
How this addresses that need:
This adds an ADR that outlines a proposed solution to this problem, by introducing a search orchestration layer that will handle API calls and results normalization.
Side effects of this change:
There are additional decisions to be made around the architecture of the search orchestrator, such as how to manage relevance normalization. These decisions are noted in the ADR and will be explored in future ADRs.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
There's some hand-waving regarding implementation details. From my perspective, it seemed adequate to delay these decisions until we start working on the orchestrator, but I would be happy to modify the ADR to include more detail if that would be useful.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing