You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
For historical reasons, and to support backward compatibility with old worker versions, we have quite a few classes that are very similar representing requests and responses when processing individual accessibility analysis tasks. I noticed when working on #884 that this is a significant obstacle or source of confusion when implementing and testing new kinds of results.
On one hand we have separate classes for the external API and the backend-to-worker communication. On the other hand, we also have separate classes for regional and single point tasks. And some of these classes have layers of superclasses that don't necessarily have any benefit.
There are opportunities to merge five request classes into one or two, and three response classes into one or two, depending on whether separate models are kept for external and internal APIs.
This would be a major change as it would disrupt backward compatibility with workers, and maybe require migration of results or database contents. As such it's something we could think through and document, but would have to delay until we are ready for well-orchestrated major version change.
Request Side
The external API uses a single model for all requests, single point or regional, which is AnalysisRequest. For communication with workers, its populateTask method is used to produce an AnalysisWorkerTask, which has two concrete subtypes: for single point tasks, we create a TravelTimeSurfaceTask in BrokerController, and for regional tasks we create a RegionalTask in RegionalAnalysisController. Also, for historical reasons, AnalysisWorkerTask extends ProfileRequest. It may be possible to merge all five of these classes into a single class, perhaps with some fields factored out into auxiliary classes that are not always included in the request. Such refactoring might also be the occasion to simplify the many small variations that are possible in these requests, for example by always referring to origin and destination point sets by ID and allowing workers to fetch and cache the details of those point sets over an API or from a database, or specifying origin and destination grid sizes separately (the same request fields can currently represent origins or destinations depending on the case).
There may be advantages to merging these into two classes rather than one, where the only distinction is external API vs. internal communication with workers.
Response Side
All accessibility calculations yield an instance of OneOriginResult, a unified internal result type for single point and regional analyses. However, for returning over the API the results are copied into an instance of AnalysisWorker.GridJsonBlock or RegionalWorkResult respectively. These two classes are quite similar in purpose, and there is duplication of logic where they are created from the same source type. This increases maintenance burden and mental load when diagnosing problems, extending or modifying the software. AnalysisWorker.GridJsonBlock ends up being returned via the external API, while RegionalWorkResult only travels between worker and backend where it's collated into regional results, but it seems like the constraints on the representations are similar.
Making a single API model object for use when serializing OneOriginResult and passing it to both the backend and UI would likely break the ability to use old worker versions, as the new normalized representation would differ from the past representation(s).
Aside: Refactoring this might be the occasion to explore other ways of returning multiple data blocks for single point requests: we currently tack a JSON serialization of AnalysisWorker.GridJsonBlock on the end of a binary grid, which makes it tricky to examine and debug the responses. It's not clear that all results should be sent as a single format (e.g. textual representation of numbers) as gridded analysis results are very similar to images, so image formats (or binary formats similar to image formats) are inherently well suited to carry and compress the data, and can be decoded by optimized native code in most browsers. Returning both one or more binary "images" and non-spatial JSON metadata for a single request implies either some kind of multipart response or multiple requests (meaning the server has to be stateful). It might actually be advantageous in other ways to have the server statefully maintain results but that's a real design change.
The text was updated successfully, but these errors were encountered:
For historical reasons, and to support backward compatibility with old worker versions, we have quite a few classes that are very similar representing requests and responses when processing individual accessibility analysis tasks. I noticed when working on #884 that this is a significant obstacle or source of confusion when implementing and testing new kinds of results.
On one hand we have separate classes for the external API and the backend-to-worker communication. On the other hand, we also have separate classes for regional and single point tasks. And some of these classes have layers of superclasses that don't necessarily have any benefit.
There are opportunities to merge five request classes into one or two, and three response classes into one or two, depending on whether separate models are kept for external and internal APIs.
This would be a major change as it would disrupt backward compatibility with workers, and maybe require migration of results or database contents. As such it's something we could think through and document, but would have to delay until we are ready for well-orchestrated major version change.
Request Side
The external API uses a single model for all requests, single point or regional, which is
AnalysisRequest
. For communication with workers, itspopulateTask
method is used to produce anAnalysisWorkerTask
, which has two concrete subtypes: for single point tasks, we create aTravelTimeSurfaceTask
inBrokerController
, and for regional tasks we create aRegionalTask
inRegionalAnalysisController
. Also, for historical reasons,AnalysisWorkerTask extends ProfileRequest
. It may be possible to merge all five of these classes into a single class, perhaps with some fields factored out into auxiliary classes that are not always included in the request. Such refactoring might also be the occasion to simplify the many small variations that are possible in these requests, for example by always referring to origin and destination point sets by ID and allowing workers to fetch and cache the details of those point sets over an API or from a database, or specifying origin and destination grid sizes separately (the same request fields can currently represent origins or destinations depending on the case).There may be advantages to merging these into two classes rather than one, where the only distinction is external API vs. internal communication with workers.
Response Side
All accessibility calculations yield an instance of
OneOriginResult
, a unified internal result type for single point and regional analyses. However, for returning over the API the results are copied into an instance ofAnalysisWorker.GridJsonBlock
orRegionalWorkResult
respectively. These two classes are quite similar in purpose, and there is duplication of logic where they are created from the same source type. This increases maintenance burden and mental load when diagnosing problems, extending or modifying the software.AnalysisWorker.GridJsonBlock
ends up being returned via the external API, whileRegionalWorkResult
only travels between worker and backend where it's collated into regional results, but it seems like the constraints on the representations are similar.Making a single API model object for use when serializing
OneOriginResult
and passing it to both the backend and UI would likely break the ability to use old worker versions, as the new normalized representation would differ from the past representation(s).Aside: Refactoring this might be the occasion to explore other ways of returning multiple data blocks for single point requests: we currently tack a JSON serialization of
AnalysisWorker.GridJsonBlock
on the end of a binary grid, which makes it tricky to examine and debug the responses. It's not clear that all results should be sent as a single format (e.g. textual representation of numbers) as gridded analysis results are very similar to images, so image formats (or binary formats similar to image formats) are inherently well suited to carry and compress the data, and can be decoded by optimized native code in most browsers. Returning both one or more binary "images" and non-spatial JSON metadata for a single request implies either some kind of multipart response or multiple requests (meaning the server has to be stateful). It might actually be advantageous in other ways to have the server statefully maintain results but that's a real design change.The text was updated successfully, but these errors were encountered: