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

WiP restructure of Space_Managers and responsibility of cache,stage and commit #171

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

nphias
Copy link
Collaborator

@nphias nphias commented Nov 4, 2024

The start of the work to re-architect around HolonSpaceManagers
in general managers maintain stateful data and service provide transient / stateless functions

HolonSpaceManagers

  • manage their own holon cache
  • manage a staging nursery and the commit cycle of holons they own.
    The Nursery is a manager internally controlled by the SpaceManager that takes care of basic staging logic

The following services assist the SpaceManager:

  • HolonService is a stateless service to bridge storage logic via holon and holon_node domain entities
  • CommitService is an intermediate helper for commit logic

Functionality of the deprecated CommitManager and CacheManager have been moved to the SpaceManager and CommitService (they still remain as unincluded modules for reference from this PR)
A Local SpaceManger is always created by the initialisation process along with its respective LocalSpace holon.
A reference to the Local SpaceManager is stored in the context so it can be accessed anyway from the context.
The space holon and space_ref belong to the HolonSpaceManager but are sourced from the session and serialised to the session via spacemanager methods

we might assume that related holons could be in other spaces so the context is needed to differenctiate.
We can also determine which space a holon belongs to during the commit process because a “parent” space property is a field in the Holon struct.

@nphias nphias self-assigned this Nov 4, 2024
@nphias nphias linked an issue Nov 4, 2024 that may be closed by this pull request
29 tasks
@nphias nphias changed the title WiP restructure of holonspace and responsibility of cahe,commit and c… WiP restructure of Space_Managers and responsibility of cache,stage and commit Nov 19, 2024
@nphias
Copy link
Collaborator Author

nphias commented Nov 25, 2024

outstanding issues:
//#[case::simple_stage_new_version_test(simple_stage_new_version_fixture())]
is failing on a final equality assertion. not logical errors or borrow panics were found
for now i commented it. as it needs confirmation on steps

further work:

  • The commit process should really write lock the stage to prevent any changes during the commit
    reading via the stage of other holons during commit is already known and allowed

@nphias nphias marked this pull request as ready for review November 25, 2024 16:34
@nphias nphias force-pushed the 165-moving-cachemanager-into-holonspace branch from 5027c76 to d46a1d4 Compare November 26, 2024 23:08
@evomimic
Copy link
Owner

evomimic commented Nov 27, 2024

Dependencies:

  • StagingArea is simply a transfer format between client and guest. It should not depend on HolonSpaceManager or Nursery.

    • Drop the from_space_manager and from_stage_to_nursery methods.
    • Add and implement from_staging_area to Nursery (but NOT as part of NurseryBehavior trait).
  • Having DanceRequest implement init_context_from_state is awkward because it requires DanceRequest to have detailed knowledge of HolonSpaceManager internals. It becomes even more awkward when init_context_from_state is called BEFORE ensure_local_holon_space.

Suggestion:

  1. Move responsibility for init_context_from_state to HolonSpaceManager. The implementation of this method can call ensure_local_holon_space.
  2. Have Dancer::dance just create an empty HSM and then invoke init_context_from_state on it.
  3. Move responsibility to Nursery as a kind of constructor: init_nursery_from_state(state:SessionState)->Self
  4. Have HSM call the Nursery's init_nursery_from_state method to initialize its nursery.

Struct Definitions

  • HolonSpaceManager struct shouldn't need BOTH a space AND a space_ref
  • Perhaps fields in HolonsContext should be Rc<RefCell, instead of just RefCell? This allows different components to share and mutate the components of the Context.
  • Fields in Nursery should be private

There are few things I requested in the Issue commentary that I notice you weren't implemented.

  • define and implement the HolonSpaceBehavior trait,
  • Move operations on the HolonSpace object itself to a holon_space.rs file with HolonSpace DTO logic. (Move these from Holon to HolonSpace)

Simplifications

  • Now that we have elevated the HolonStagingBehavior to HolonSpaceManager, staged_reference needn't know or access the internals of HolonSpaceManager or nursery. For example, get_rc_holon can just borrow the space manager and then call get_holon_by_index

Minor Style Concerns:

  • Variable names should be camel_case (e.g., spaceholon -> space_holon)
  • Fluent methods should be chained on separate lines -- for example,

Copy link
Owner

@evomimic evomimic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There remains some areas to be improved, but no showstopper issues in this code and all tests pass. I'm approving this PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code in staged_reference.rs module violates the encapsulation of the staged holons within the Nursery... don't expose the staged holons, just ask the SpaceManager for the holon by index.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to introduce a StagedIndex type (as is currently being done in the staged_reference.rs module), it should be defined in the module that actually owns the keyed_index... i.e., it should be defined in the nursery.rs module and the definition of the Nursery's keyed_index should use that type.

@evomimic evomimic merged commit 224494a into main Nov 27, 2024
1 check passed
@evomimic evomimic deleted the 165-moving-cachemanager-into-holonspace branch November 27, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving Cache and Staging responsibility into HolonSpace
2 participants