Skip to content
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

[enhancement] Improve Interfaces Between Components #18

Closed
1 task done
wilyle opened this issue Jul 19, 2023 · 0 comments · Fixed by #106
Closed
1 task done

[enhancement] Improve Interfaces Between Components #18

wilyle opened this issue Jul 19, 2023 · 0 comments · Fixed by #106
Assignees
Labels
enhancement New feature or request

Comments

@wilyle
Copy link
Contributor

wilyle commented Jul 19, 2023

Description

Most components are using a "shared state" model (usually Arc<Mutex<T>>) to communicate data. This interface is clunky and may not even be the best approach. Consider modifying these interfaces to improve readability, maintainability. and/or performance.

The main suggestion would be to wrap usage of Arc<Mutex<T>> behind another struct where appropriate. For example, instead of using Arc<Mutex<HashMap<String, DigitalTwinMapEntry>>> in the cartographer and emitter, create a DigitalTwinMap struct which exposes methods such as get(id: String) and set(id: String, value: DigitalTwinMapEntry). The DigitalTwinMap could manage locking the resource as well, meaning that lock statements are not needed by the caller. Callers would likely just need to use Arc<DigitalTwinMap> rather than the more complex type. This wrapper can likely be made generic to avoid code duplication, and the DigitalTwinMap might extend or alias a struct such as SharedMap<TKey, TValue>.

Other things to consider:

  • Refactor to have reader/writer roles. Most interfaces between components go one way where one component is updating the data and another is reading it. In this case, the "more correct" way to implement this would be with reader or writer access as appropriate.
  • Refactor to avoid having to copy and maintain separate state where sensible. For example, the emitter copies data from the digital twin map into its own emissions map which has additional metadata. However, each entry in the emitter's map has a corresponding entry in the shared state with the cartographer. If the DigitalTwinMapEntry data type contained fields for this data then the copy would not be necessary, saving both CPU and memory usage. This should be balanced with the impact of truly using shared state like this since the threads accessing the data might be blocked more often, causing delays in updating this shared state.
  • Refactor to avoid using Arc<Mutex<T>> as the interface between components. Possible options include a channel-based approach or a client-server-style approach. Note that most alternatives (including the possibilities mentioned) would require additional resource consumption, so this should be balanced against the benefits of such an approach.

Acceptance criteria

  • Components do not use Arc<Mutex<T>> directly to share data (since there are multiple ways to realize this as noted in the description, this is the only acceptance criterion included in this issue)
@wilyle wilyle added the enhancement New feature or request label Jul 19, 2023
@wilyle wilyle self-assigned this Jul 27, 2023
wilyle added a commit that referenced this issue Sep 6, 2023
Reimplements the cartographer and emitter. Changes include:
- Switch from `Box<dyn Trait>` to generics for pluggable components
- Use a single shared signal store rather than separate shared components between each interface combo
- Move the `DigitalTwinAdapter::find_by_id` call to the cartographer, thereby eliminating the need for digital twin adapters to run a separate thread
- Update the API for providers and the provider proxy to use struct-like enum variants and return `Result` instead of panicking
- Utilize `mockall` for emitter tests instead of the in-memory mock
- Prevent errors on one signal in the cartographer and emitter from taking down the entire app
- Misc minor cleanup

This partially addresses #18, but more work is needed to get provider proxies integrated
wilyle added a commit that referenced this issue Jan 9, 2024
Merge contracts and common crates to simplify directory structure. This also helps deal with a circular dependency that arose in finishing #18
wilyle added a commit that referenced this issue Jan 10, 2024
Closes #18 by migrating proxies to the signal store model for sharing data.

Also moves some macros related to axum to a shared place since I otherwise needed to copy one of them for the http proxy

Co-authored-by: jorchiu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant