Fix #652: Don't create mappings unless they're actually needed #656
Conversation
This was leading to deadlock in perfectly reasonable cases and still could lead to deadlock in less-reasonable cases.
isc-tleavitt
requested review from
isc-kiyer,
isc-eneil,
isc-shuliu,
isc-jili and
isc-jlechtne
this is just to force CI to run...
| Set db = ##class(%SYS.Namespace).GetPackageDest(,name) | ||
| } ElseIf (type = "CLS") { | ||
| Set db = ##class(%SYS.Namespace).GetPackageDest(,$Piece(name,".",1,*-1)) | ||
| } Else { |
There was a problem hiding this comment.
Do we need a case for globals as well? Also, rather than grouping the specific use cases in the Document super class, would it make sense to have specific implementations in the respective sub classes?
There was a problem hiding this comment.
It was interesting looking back at the history of Beiming's work here - it started out as a separate implementations, then moved to a centralized (wrong in many ways) implementation.
In this case I focused on making it work for all of the existing callers, which all now pass an InternalName. Agreed that we should branch out to globals later; filed a separate issue for this: #658
There was a problem hiding this comment.
A reason to keep this centralized based on InternalName is that a custom resource processor might care about the mapping of something other than the document name itself (e.g., an associated/generated file).
There was a problem hiding this comment.
In that case, a custom processor can call the method for the corresponding file type though right instead of calling the one in Document? I think specifically just the call to Get*Dest/equivalent should be in separate sub-classes of Document so the overarching logic for can remain in Document since that's the only part that differs between different document types.
There was a problem hiding this comment.
That's fair - will cover refactoring in the followup task as well.
@isc-kiyer @isc-eneil and others - any concerns on this from a HS perspective?