Refactor/singleton aggregate#46
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR restructures the es_kernel application with OTP lifecycle management, introducing a top-level supervisor hierarchy for dynamic aggregate spawning and a unified public dispatch API. It refactors the singleton manager to use domain-based composite-key routing, removes lifecycle callbacks from store contracts, and updates all dependent code to align with the new architecture. Changes
Sequence DiagramsequenceDiagram
participant Client
participant es_kernel as es_kernel (dispatch)
participant mgr as es_kernel_mgr_aggregate
participant sup as es_kernel_aggregate_sup
participant agg as es_kernel_aggregate
participant store as Store
Client->>es_kernel: dispatch(Command)
es_kernel->>mgr: dispatch(ServerRef, Command)
activate mgr
rect rgba(100, 150, 200, 0.2)
note over mgr: Extract domain & stream_id<br/>from command map
alt Aggregate exists
mgr->>agg: execute(Aggregate, Command)
else Aggregate not found
mgr->>sup: start_aggregate(Aggregate, StoreContext, Id, Opts)
activate sup
sup->>agg: start_link(Aggregate, StoreContext, Id, Opts)
activate agg
agg->>store: load_latest snapshot
agg->>store: fold events from snapshot
agg-->>sup: {ok, Pid}
deactivate agg
deactivate sup
mgr->>agg: execute(Aggregate, Command)
end
activate agg
agg->>agg: apply command to state
agg->>store: append events
agg-->>agg: emit result
deactivate agg
end
mgr-->>es_kernel: ok | {error, Reason}
deactivate mgr
es_kernel-->>Client: ok | {error, Reason}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Areas requiring close attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
- Coverage 81.56% 79.37% -2.19%
==========================================
Files 12 16 +4
Lines 320 320
==========================================
- Hits 261 254 -7
- Misses 59 66 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ccamel
force-pushed
the
refactor/singleton-aggregate
branch
from
301a116 to
67e02ae
Compare
ccamel
force-pushed
the
refactor/singleton-aggregate
branch
from
67e02ae to
60c7c70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/es_kernel/src/es_kernel_aggregate.erl (1)
190-227: Passivation timer is not updated on error paths (can cause premature passivation)In both
handle_call/3andhandle_cast/2you do:NewTimerRef = install_passivation(State#state.timeout, State#state.timer_ref), case process_command(State, Command) of {ok, {State1, Sequence1}} -> ..., {reply, ok, State#state{..., timer_ref = NewTimerRef}}; {error, Reason} -> {reply, {error, Reason}, State} end.On
{error, Reason}/{error, _}you don’t writeNewTimerRefback into the state, which means:
- The old
timer_refwas just cancelled.- A new timer was scheduled but its ref is now lost.
- Subsequent successful commands will cancel the old (already cancelled) ref, not the active one, so earlier timers can still fire and passivate the aggregate even after fresh activity.
That can lead to “random” passivation some time after the first failed command, even if commands kept coming in afterwards.
I’d update the error branches to store
NewTimerRefas well, so errors still count as activity for passivation purposes:handle_call(Command, _From, State) -> NewTimerRef = install_passivation(State#state.timeout, State#state.timer_ref), case process_command(State, Command) of {ok, {State1, Sequence1}} -> maybe_save_snapshot(State#state{state = State1, sequence = Sequence1}), {reply, ok, State#state{ state = State1, sequence = Sequence1, timer_ref = NewTimerRef }}; {error, Reason} -> - {reply, {error, Reason}, State} + {reply, {error, Reason}, State#state{timer_ref = NewTimerRef}} end. handle_cast(Command, State) -> NewTimerRef = install_passivation(State#state.timeout, State#state.timer_ref), case process_command(State, Command) of {ok, {State1, Sequence1}} -> maybe_save_snapshot(State#state{state = State1, sequence = Sequence1}), {noreply, State#state{ state = State1, sequence = Sequence1, timer_ref = NewTimerRef }}; {error, _} -> - {noreply, State} + {noreply, State#state{timer_ref = NewTimerRef}} end.That keeps the passivation behavior consistent regardless of whether the last command succeeded or failed.
README.md (2)
129-150: Event store behaviour snippet still advertises start/0 and stop/0In the “Event store” section you still show:
% Initializes the event store -callback start() -> {ok, initialized | already_initialized} | {error, term()}. % Shuts down the event store. -callback stop() -> {ok} | {error, term()}.But the actual
es_contract_event_storebehaviour no longer includesstart/0andstop/0— lifecycle is now intentionally outside the behaviour contract and handled at the application level (or via concrete backends).Might be good to:
- Drop these two callbacks from the README snippet, or
- Move them under a “typical backend module may also expose…” note, making it clear they’re not part of the formal behaviour.
That’ll keep new backend authors from assuming
start/0/stop/0are required when they’re not.
175-182: Aggregate start_link examples still use the old Store argumentBoth aggregate examples currently show:
es_kernel_aggregate:start_link( Module, Store, Id, #{snapshot_interval => 10} ).and
es_kernel_aggregate:start_link( bank_account_aggregate, es_store_ets, <<"account-123">>, #{ timeout => 5000, snapshot_interval => 10 } ).But
es_kernel_aggregate:start_link/4now expects aStoreContext :: es_kernel_store:store_context(), i.e. a{EventStore, SnapshotStore}tuple, not a single store module — and internally it callses_kernel_store:append/3,load_latest/2, etc. against that context.To match the current code, these examples should be updated to something like:
StoreCtx = es_kernel_app:get_store_context(), es_kernel_aggregate:start_link( Module, StoreCtx, Id, #{snapshot_interval => 10} ).and similarly for the bank account example.
As written, anyone copy-pasting the snippet with
es_store_etsalone will hit a badmatch insidees_kernel_store.Also applies to: 287-296
🧹 Nitpick comments (14)
apps/es_kernel/src/es_contract_event_store.erl (1)
5-8: Clarify error semantics forappend/2to match preferred styleThe behaviour docs look good, and calling out that lifecycle is out-of-band is a nice simplification. One small suggestion: right now
append/2is specced as returning onlyok, while the docs say it “may throw an exception if persistence fails”. If you intend backends to prefer{error, Reason}-style failures over exceptions (which tends to play nicer with callers and observability), it could be worth making that explicit here and/or mentioning that throwing should be reserved for programmer errors, not persistence failures. Based on learnings, ...Also applies to: 21-22, 35-40
apps/es_kernel/src/es_kernel.app.src (1)
4-5: OTP app layout looks good; just double‑check registered namesThe app resource now looks like a conventional OTP app:
es_kernel_appas the callback module, top‑level + aggregate supervisors, and the aggregate manager all wired up, with{event_store, snapshot_store}matching howstore_context()is described elsewhere. Only thing I’d suggest is double‑checking that each ofes_kernel_sup,es_kernel_aggregate_sup, andes_kernel_mgr_aggregateactually registers a process under that exact atom so theregisteredlist stays truthful for observers/tools.Also applies to: 7-10, 20-26
apps/es_kernel/src/es_contract_snapshot_store.erl (1)
5-23: Nice clarification of snapshot semantics; just make sure callers handle{warning, Reason}consistentlyThe updated docs and callback spec read really well — they make it clear snapshots are optional optimizations, keep lifecycle concerns out of the behaviour, and require fully-formed snapshot records (domain, stream_id, sequence, etc.), which lines up nicely with the existing “stores shouldn’t fill in missing fields” design. Based on learnings, that’s exactly what we want.
Only small thing I’d double‑check is that all snapshot store implementations and call sites now consistently treat
{warning, Reason}as the “non-fatal failure” path (e.g. log + continue) and don’t still expect{error, Reason}or crash semantics for persistence problems. If that’s already true across the codebase, then this contract looks good to me.Also applies to: 33-45
apps/es_kernel/src/es_kernel.erl (1)
1-30: Thin public dispatch API looks good; just keep error behaviour and tests aligned with the specI like the move to a single
es_kernel:dispatch/1entrypoint that just forwards into the singleton manager — keeps the public surface nice and small.Given the spec says
ok | {error, Reason}, it’d be good to make surees_kernel_mgr_aggregate:dispatch/2never leaks raw exits/throws (e.g.noprocif the manager isn’t started) and always normalizes failures into{error, Reason}so callers can trust this contract.Also, since Codecov flags this file as uncovered, a tiny test that calls
es_kernel:dispatch/1in both the happy path and an error path (e.g. bad command/domain) would lock the behaviour in and bump coverage.apps/es_kernel/src/es_kernel_app.erl (1)
11-60: Application callback looks solid; consider a small test aroundget_store_context/0defaultsThis app module is very clean: delegating
start/2straight toes_kernel_sup:start_link()is the usual OTP pattern, and centralizing the{EventStore, SnapshotStore}resolution inget_store_context/0keeps configuration nicely in one place and matches the “stores are pluggable backends” design.As a small follow‑up, I’d suggest adding a couple of tests that exercise
get_store_context/0with and withoutevent_store/snapshot_storeset in the app env, just to lock in the defaulting toes_store_etsand avoid regressions when new store types get added.examples/demo_bank.script (1)
13-26: Demo wiring matches the new OTP/kernel setup nicelyThe revised script reads really well: you configure the kernel to use ETS, start the ETS store explicitly, then boot the
es_kernelapplication and send commands via the newes_kernel:dispatch/1helper. That’s a nice, linear story that mirrors the supervision design.Nothing blocking here from my side — if you ever want to extend the demo, you could optionally pattern‑match on
Dispatch/2results to pretty‑print success vs error cases, but as-is it’s already a clear and useful walkthrough.Also applies to: 30-41, 43-51
apps/es_kernel/test/es_kernel_store_tests.erl (2)
40-57: Normalize store lifecycle helpers (and consider return semantics)The
start_store/1andstop_store/1helpers nicely centralize the lifecycle logic for{EventStore, SnapshotStore}tuples, and they make the ETS/Mnesia vs composite-store cases much easier to reason about.Two small thoughts:
start_store/1currently ignores the result ofEventStore:start()and, whenEventStore =:= SnapshotStore, always returnsok. That’s fine for these tests, but it does mean you’ll never notice{error, Reason}fromEventStore:start/0. If you ever want tests to assert on startup failures, you might want to pattern‑match or at least propagate non‑okresults.- Symmetrically,
stop_store/1only returns theEventStore:stop()result. That’s probably enough here, but if the snapshot store fails to stop you won’t see it.If you’re happy treating lifecycle failures as “test setup explosions” rather than explicit assertions, this is totally fine as-is; otherwise, a small refactor to pattern‑match on
{ok, _} | {error, _}in both helpers would make failures more visible.
391-422: Snapshot error test comment is now slightly out of sync with behaviorIn the
snapshot_save_error/1tests:
- ETS branch: you explicitly
start_store/1,stop_store/1, then verifyes_kernel_store:store/2returns{warning, _}on a stopped store. That nicely matches the design goal of preferring{warning, Reason}over crashes for snapshot failures. Based on learnings, this is exactly the shape you want for store error handling.- Mnesia branch: the comment still talks about “the store persists even after stop() is called” and “error path tested implicitly”, but you no longer call a
stop()function here — you juststart_store/1and then store a snapshot successfully.Might be worth tightening the Mnesia comment so it just states that snapshots are expected to succeed there (and that snapshot error handling is covered by other tests), to avoid confusion for future readers.
apps/es_kernel/test/es_kernel_mgr_aggregate_tests.erl (1)
14-47: Test setup/teardown matches new architecture, with small robustness nitsThe new
setup/0/teardown/1flow lines up nicely with the refactor:
- You configure
es_kernel’sevent_storeandsnapshot_storeviaapplication:set_env/3.- Start the configured store modules explicitly (for ETS table creation etc.).
- Ensure
es_kernel_aggregate_supis running, and that any stalees_kernel_mgr_aggregatefrom prior runs is killed before each test.Two minor things you might consider (mostly for future test stability):
- The
timer:sleep(10)after killing the manager/supervisor is a bit arbitrary; in practice 10ms is usually fine, but if tests ever get flaky under load, bumping that or switching to a small wait loop that pollswhereis/1could make it more robust.- You’re returning
StoreContextfromsetup/0but tests mostly calles_kernel_app:get_store_context()again instead of using the fixture. Not wrong at all, just slightly redundant.Functionally, though, this harness matches the new OTP layout well.
Also applies to: 48-75
apps/es_kernel/test/es_kernel_aggregate_tests.erl (3)
17-37: StoreContext-based setup/teardown is wired up nicelyThe move to
es_kernel_app:get_store_context/0and starting/stopping{EventStore, SnapshotStore}(including the “same module” case) looks consistent with the new app-level configuration and keeps the tests backend-agnostic.If you ever see tests get slow or flaky around store lifecycle, one small tweak could be to centralize the
application:load/set_envin a higher-level suite setup and leave this foreach setup just for starting/stopping the resolved modules, but what you have is perfectly serviceable for now.
41-57: Nice assertion + cmd helper; just be aware of the hidden env dependencyUsing
?assertStatewithStoreCtx = es_kernel_app:get_store_context()and thecmd/3helper keeps the tests very readable and lines up with the domain+stream command contract.The only thing to keep in mind is that
?assertStatenow implicitly depends on whateveres_kernelenv is currently set; if you ever introduce tests with differing store contexts, passingStoreCtxinto the macro (or assertion helper) instead of re-reading the env would make that coupling explicit. For the current suite, this is totally fine and clean.
59-239: Tests around execute/2, snapshots, passivation, and now_fun look solid (with a tiny perf nit)The switch to
es_kernel_aggregate:execute/2pluscmd/3aligns well with the new command model, and all the snapshot/rehydration andnow_funexpectations look consistent with the aggregate’s semantics and the StoreContext plumbing.One small thing you might consider later:
aggregate_passivation/0waits2000ms after usingTimeout = 1000. Dropping both to something likeTimeout = 100andtimer:sleep(200)(or driving time vianow_funif the implementation allows) can tighten test runtime without really affecting the intent. Not urgent, but worth keeping in mind as the suite grows.apps/es_kernel/src/es_kernel_mgr_aggregate.erl (2)
150-162: DOWN handling via maps:filter is correct; reverse index is a future micro-optimizationThe
'DOWN'handling that filterspidsby value and removes the matching key is correct under the invariant of one aggregate per{Aggregate, Id}, and keeps the state clean when children die.If this manager ever needs to track large numbers of aggregates and DOWN traffic becomes noticeable, you could consider maintaining a secondary
pid -> Keymap to avoid scanning all entries on each DOWN, but that’s an optimization you can happily postpone.
73-89: Confirmed: dead aggregates crash the manager—consider resilient routing if churn is expectedThe concern is real:
gen_server:call/2exits the caller with{noproc, {gen_server, call, ArgList}}when the target process is dead. Sinceexecute/2directly wrapsgen_server:call(line 92), any call to a dead aggregate will crash the manager process at lines 198 or 205 inensure_and_dispatch/4before the'DOWN'handler runs.The suggested
catchpattern (lines 204–206) would let the manager survive, drop the stale pid, and return{error, aggregate_down}to the caller—trade-off being fail-fast for robustness under churn. This is a valid design choice for a singleton router; if that tradeoff suits your ops model, the pattern works well.If you keep the current fail-fast behavior, document it as intentional; if you'd prefer resilience, the suggested guard is straightforward. Either way, the routing logic and explicit
ok | {error, Reason}API boundary stay clean.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
README.md(6 hunks)apps/es_kernel/src/es_contract_event_store.erl(2 hunks)apps/es_kernel/src/es_contract_snapshot_store.erl(2 hunks)apps/es_kernel/src/es_kernel.app.src(2 hunks)apps/es_kernel/src/es_kernel.erl(1 hunks)apps/es_kernel/src/es_kernel_aggregate.erl(4 hunks)apps/es_kernel/src/es_kernel_aggregate_sup.erl(1 hunks)apps/es_kernel/src/es_kernel_app.erl(1 hunks)apps/es_kernel/src/es_kernel_mgr_aggregate.erl(9 hunks)apps/es_kernel/src/es_kernel_mgr_behaviour.erl(0 hunks)apps/es_kernel/src/es_kernel_store.erl(1 hunks)apps/es_kernel/src/es_kernel_sup.erl(1 hunks)apps/es_kernel/test/es_kernel_aggregate_tests.erl(8 hunks)apps/es_kernel/test/es_kernel_mgr_aggregate_tests.erl(3 hunks)apps/es_kernel/test/es_kernel_store_tests.erl(11 hunks)apps/es_xp/src/bank_account_aggregate.erl(1 hunks)config/sys.config(1 hunks)examples/demo_bank.script(1 hunks)
💤 Files with no reviewable changes (1)
- apps/es_kernel/src/es_kernel_mgr_behaviour.erl
🧰 Additional context used
📓 Path-based instructions (2)
**/*.erl
⚙️ CodeRabbit configuration file
**/*.erl: Review the Erlang source files with a focus on the project's architecture and best practices:
- Actor Model & Concurrency: Ensure that OTP behaviors (e.g.,
gen_server,gen_statem,gen_event) are correctly implemented and used effectively.- Event Sourcing: Validate that event persistence and retrieval via Mnesia follow best practices, ensuring proper data consistency and versioning.
- Domain-Driven Design (DDD): Check if the event-driven architecture properly separates commands, events, and state transitions.
- Code Quality and Maintainability: Ensure idiomatic Erlang code, proper pattern matching, effective use of records and maps, and clean module organization.
- Performance & Scalability: Identify bottlenecks related to message passing, ETS/Mnesia transactions, and process spawning. Suggest optimizations where applicable.
Files:
apps/es_xp/src/bank_account_aggregate.erlapps/es_kernel/src/es_kernel.erlapps/es_kernel/src/es_kernel_app.erlapps/es_kernel/src/es_kernel_aggregate_sup.erlapps/es_kernel/src/es_kernel_sup.erlapps/es_kernel/test/es_kernel_store_tests.erlapps/es_kernel/src/es_kernel_aggregate.erlapps/es_kernel/src/es_kernel_store.erlapps/es_kernel/src/es_contract_snapshot_store.erlapps/es_kernel/src/es_kernel_mgr_aggregate.erlapps/es_kernel/test/es_kernel_mgr_aggregate_tests.erlapps/es_kernel/src/es_contract_event_store.erlapps/es_kernel/test/es_kernel_aggregate_tests.erl
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Review markdown documentation with these guidelines:
- Structure: Ensure logical organization and consistency in headings.
- Technical Accuracy: Verify correctness in Erlang/Mnesia-related concepts and example code.
- Clarity: Ensure content is concise, with minimal jargon, making it accessible to developers.
- Completeness: Check if all key functionalities are covered and properly documented.
Files:
README.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: ccamel
Repo: ccamel/erlang-event-sourcing-xp PR: 23
File: apps/event_sourcing_core/src/event_sourcing_core_aggregate.erl:360-377
Timestamp: 2025-10-24T18:37:48.183Z
Learning: In apps/event_sourcing_core, prefer explicit error returns (ok | {error, Reason}) over blanket try/catch blocks for store operations. This provides better visibility into errors without masking bugs in store implementations.
📚 Learning: 2025-10-24T18:37:48.183Z
Learnt from: ccamel
Repo: ccamel/erlang-event-sourcing-xp PR: 23
File: apps/event_sourcing_core/src/event_sourcing_core_aggregate.erl:360-377
Timestamp: 2025-10-24T18:37:48.183Z
Learning: In apps/event_sourcing_core, prefer explicit error returns (ok | {error, Reason}) over blanket try/catch blocks for store operations. This provides better visibility into errors without masking bugs in store implementations.
Applied to files:
apps/es_xp/src/bank_account_aggregate.erlREADME.mdapps/es_kernel/src/es_kernel_app.erlexamples/demo_bank.scriptapps/es_kernel/test/es_kernel_store_tests.erlapps/es_kernel/src/es_kernel_store.erlapps/es_kernel/src/es_contract_snapshot_store.erlapps/es_kernel/test/es_kernel_mgr_aggregate_tests.erlapps/es_kernel/src/es_contract_event_store.erlapps/es_kernel/test/es_kernel_aggregate_tests.erl
📚 Learning: 2025-10-24T19:50:30.466Z
Learnt from: ccamel
Repo: ccamel/erlang-event-sourcing-xp PR: 23
File: apps/event_sourcing_core/src/event_sourcing_core_store_ets.erl:120-140
Timestamp: 2025-10-24T19:50:30.466Z
Learning: In the event_sourcing_core system, events and snapshots should follow a consistent API pattern: both should be created with domain information included and then passed as complete records for storage, rather than having fields populated at the storage layer.
Applied to files:
apps/es_kernel/src/es_kernel_store.erlapps/es_kernel/src/es_contract_snapshot_store.erlapps/es_kernel/src/es_contract_event_store.erl
🪛 LanguageTool
README.md
[style] ~121-~121: Consider using “who” when you are referring to a person instead of an object.
Context: ...el_aggregate_sup: a dynamic supervisor that spawns es_kernel_aggregate` processes ...
(THAT_WHO)
🔇 Additional comments (7)
apps/es_kernel/src/es_kernel_store.erl (1)
3-11: Nice consolidation of store context and record-centric APIThe updated docs around
store_context()and the split{EventStore, SnapshotStore}make the boundary really clear, and the helpers (new_event/…,new_snapshot/5) keep all domain fields (including timestamps in metadata) assembled before hitting the backend. Delegatingstore/2andload_latest/2directly to the modules without wrapping in try/catch also lines up well with the explicit{ok | {error, Reason}}style from the rest of the stack. Based on learnings, ...Also applies to: 40-42, 116-152, 243-272
config/sys.config (1)
2-5: Config wiring matches the new kernel envThe new
es_kernelsection insys.configlines up nicely with the application env ines_kernel.app.srcand the{EventStore, SnapshotStore}store context. This should make swapping backends later straightforward without touching code. Looks good to me.apps/es_kernel/src/es_kernel_sup.erl (1)
1-68: Supervisor structure looks solid and idiomaticThe top-level
es_kernel_suplayout is clean:
- Map-based
SupFlagswithone_for_oneand sane intensity/period.- Dynamic
es_kernel_aggregate_supand singletones_kernel_mgr_aggregateare wired correctly as child specs.StoreContextpulled once ininit/1and passed into the manager is a nice, simple injection point.Only thing to keep in mind: if
es_kernel’s env is misconfigured,es_kernel_app:get_store_context/0will blow up during supervisor init and prevent the app from starting. That might actually be what you want (fail fast), but if you ever need softer failure you could wrap that call and return{stop, Reason}instead.Otherwise this looks good to me.
apps/es_kernel/src/es_kernel_aggregate.erl (1)
77-133: Nice separation of command execution and rehydrationThe new
execute/2andrehydrate/3split looks good:
execute/2is a very clear wrapper aroundgen_server:call/2, which is what the manager needs.init/1wiring{State1, Sequence1} = rehydrate(...)withTimeout,SnapshotInterval,now_fun, andtimer_refmakes the aggregate startup story much easier to follow.rehydrate/3doing “snapshot if available, thenfold/5over the tail range” is exactly the pattern you want for event-sourced rehydration.This keeps domain logic (
Aggregate:init/0,Aggregate:apply_event/2) nicely separated from infra concerns and leans on explicit store APIs without try/catch, which matches the preferred error-handling style. Based on learnings, this is a solid direction.Also applies to: 147-179
apps/es_kernel/test/es_kernel_mgr_aggregate_tests.erl (1)
79-101: Helpers and expectations are consistent with composite-key routingThe updated helpers and tests look well-aligned with the new manager semantics:
agg_id/3now keys into the manager state with{Aggregate, Id}, which matches the composite key{domain, stream_id}approach.cmd/3builds properes_contract_command:new/6values, and alldispatch/2calls now go through this instead of raw tuples — that should keep tests resilient to any future tweaks to the command shape.?assertStatechecking for{state, bank_account_aggregate, StoreCtx, Id, ...}lines up with the aggregate’s updated#state{}record (aggregate, store_ctx, id, state, sequence, ...).start_mgr/1taking a timeout and wiringStoreContext+ opts intoes_kernel_mgr_aggregate:start_link/2mirrors how the real manager will be booted underes_kernel_sup.Overall, these tests are exercising exactly the behaviors you care about (routing, passivation, invalid-command handling) against the new manager/aggregate wiring.
Also applies to: 102-155, 173-179
README.md (1)
24-32: High-level kernel/manager docs are nicely in syncThe updated README around:
- Kernel OTP app + supervision (
es_kernel_sup,es_kernel_aggregate_sup,es_kernel_mgr_aggregate),- The demo that configures ETS via
application:set_env/3, startses_store_ets, then bootses_kerneland dispatches viaes_kernel:dispatch/1,- And the aggregate manager diagrams/flow that show composite-key routing and
es_kernel_aggregate:execute/2…all look spot on relative to the new architecture. This is a really clear narrative for how everything hangs together.
Also applies to: 46-91, 116-124, 301-320
apps/es_kernel/src/es_kernel_mgr_aggregate.erl (1)
28-36: State modelling and start_link/2 match the new singleton manager designThe
#state{store, opts, pids}record with{Aggregate, StreamId}keys plusstart_link(StoreContext, Opts)registering the process ases_kernel_mgr_aggregateline up nicely with the revised OTP app + dynamic supervisor architecture. The open map type forOpts(timeout, now_fun) also gives you room to forward extra options to aggregates without tightening the spec too much.Also applies to: 41-61, 97-110
ccamel
force-pushed
the
refactor/singleton-aggregate
branch
from
c5a50e5 to
40f4d61
Compare
ccamel
force-pushed
the
refactor/singleton-aggregate
branch
from
40f4d61 to
21f5df0
Compare
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
2 participants
This refactoring builds on the previous changes (#42, #45). 👷♂️
Now time to turn the kernel into a proper OTP application with a clear supervision tree and a single entrypoint for commands. Instead of spreading routing and lifecycle concerns across behaviours and helpers, a singleton aggregate manager now sits in front of a dynamic supervisor, spins up aggregates on demand, rehydrates them from the store, and keeps its own registry clean.
Also, persistence is now plugged in via the application environment, so the kernel no longer knows the backend and just relies on the event/snapshot contracts...
Documentation, tests and examples have been updated accordingly.