slawlor / ractor

Rust actor framework
MIT License
1.38k stars 69 forks source link

Add Erlang-inspired process group (`pg`) scopes #177

Closed leonqadirie closed 10 months ago

leonqadirie commented 11 months ago

Might resolve #31.

I went the proposed route with split functions and (scope, group) as an identifying tuple. Couldn't find the mentioned protobuf OTW cluster protocol changes.

As this is my first-ever PR with 'real' code I suggest a more thorough look. My main concerns besides the API design are:

Thanks!

-- Edit: Regarding the code coverage: this seems to mostly stem from duplicated yet also previously untested logic due to the split function approach. The exception is the new which_scopes_and_groups() for which I can provide a test if desired - just a bit trickier when running tests concurrently. :)

codecov[bot] commented 11 months ago

Codecov Report

Attention: 102 lines in your changes are missing coverage. Please review.

Comparison is base (d3572b9) 79.19% compared to head (7b1ee20) 79.48%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #177 +/- ## ========================================== + Coverage 79.19% 79.48% +0.28% ========================================== Files 50 50 Lines 9148 9671 +523 ========================================== + Hits 7245 7687 +442 - Misses 1903 1984 +81 ``` | [Files](https://app.codecov.io/gh/slawlor/ractor/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor) | Coverage Δ | | |---|---|---| | [ractor\_cluster/src/node/node\_session/tests.rs](https://app.codecov.io/gh/slawlor/ractor/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor#diff-cmFjdG9yX2NsdXN0ZXIvc3JjL25vZGUvbm9kZV9zZXNzaW9uL3Rlc3RzLnJz) | `93.44% <100.00%> (+0.02%)` | :arrow_up: | | [ractor/src/actor/messages.rs](https://app.codecov.io/gh/slawlor/ractor/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor#diff-cmFjdG9yL3NyYy9hY3Rvci9tZXNzYWdlcy5ycw==) | `63.38% <0.00%> (-4.81%)` | :arrow_down: | | [ractor\_cluster/src/node/node\_session/mod.rs](https://app.codecov.io/gh/slawlor/ractor/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor#diff-cmFjdG9yX2NsdXN0ZXIvc3JjL25vZGUvbm9kZV9zZXNzaW9uL21vZC5ycw==) | `30.51% <0.00%> (-0.44%)` | :arrow_down: | | [ractor/src/pg/tests.rs](https://app.codecov.io/gh/slawlor/ractor/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor#diff-cmFjdG9yL3NyYy9wZy90ZXN0cy5ycw==) | `93.53% <93.91%> (+0.42%)` | :arrow_up: | | [ractor/src/pg/mod.rs](https://app.codecov.io/gh/slawlor/ractor/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor#diff-cmFjdG9yL3NyYy9wZy9tb2QucnM=) | `73.12% <70.31%> (+0.79%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/slawlor/ractor/pull/177/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Sean+Lawlor)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

slawlor commented 11 months ago

Thanks for the contribution! I'll try and take a look as soon as I can but it might be a couple of days. 😁

leonqadirie commented 11 months ago

I think we can reduce a lot of code duplication by having the non-scoped methods call the scoped ones just with the default scope as an argument. That also will reduce it to 1 implementation of each call (join/leave/etc)

Well, duh. Thanks for the pointers, learned a bit or two. 😅 I did get occasional CI failures for the async-std tests, but suspect they are not related to my code.

slawlor commented 11 months ago

I did get occasional CI failures for the async-std tests, but suspect they are not related to my code.

Yeah I'm working on stabilizing these actually. I have another branch I'm going to split out some changes for this runtime to stabilize the flaky tests. I'm working on a wasm runtime but that might be a dead end lol

slawlor commented 11 months ago

Apologies but this might take some time to get to another review. We just had a new baby and I'm a little sleep deprived at the moment lol

leonqadirie commented 11 months ago

First of all, congratulations! And no worries, take all the time you need. :)

slawlor commented 11 months ago

I think it's really close! but there's some cleanups and optimizations that could be done. Should be quick & minimal. Thanks again!

leonqadirie commented 11 months ago

Cool and thanks for stabilizing the async-std tests!

A bit unsure of how to proceed/which cleanups and optimizations you are referring to specifically. Prior reviews seem to be addressed in logic; besides this one, that is:

This is where having another index might be helpful, i.e. Scope -> Group name in a separate mapping. But that's an optimization that can be added in the future.

-- Addendum: 2 reviews are not marked as outdated as they were mapped to unchanged comments instead of the function bodies.

slawlor commented 11 months ago

I think I missed a button to.mark.the review done. I'm happy to leave the double index but to the future but the 6 comments in my latest review should be addressed prior to merging. If anything doesn't make sense, let me know. And feel free to mark the old review comments as done to clear up the pending comments

leonqadirie commented 10 months ago

Thanks for addressing the comments and bearing with me! This looks great, and thanks for the additions :)

Nice! Feels good to have my first 'real' PR merged. :) Thanks for the feedback and please always feel free to be (constructively) pedantic with my code, I already learned a ton here.

Will address the test and then maybe take a look into what kind of index optimisation you had in mind.