-
Notifications
You must be signed in to change notification settings - Fork 18
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
Rewrite the ChainActivityHandler
#597
Conversation
might have to give this a try later also for a second opinion, I also have my own take on reworking chain activity handler if anyone's interested, albeit it was made with a different rationale behind them, do let me know if you want PR of that patch |
I think I actually like my approach better, because it is more straight forward in what it does. But if you think differently I am happy to discuss how we want to change the handler (I mean it, I do not think that my code is just better 😅 ) Your code definitely has one giant problem though (if I am not mistaken): current |
you do raise an interesing point in terms of ensuring operation continuity, and yeah my patch does fall short on that part, but I think that could be work around: changed the name for the new though I have to say that I don't like the prospect of changing message signature nuking messenger systems very much if that's the case
as for this part I'm afraid that it'll devolve into mostly value differences, as I feel like "straight forward" for you and me is quite different here what I've done in my patch is roughly a reimplementation of the same logic that I managed to untangle from the old handler, and try to express it in a more obvious way (as in, fewer and more visible actions/branches that should be more easy to follow through, rather than whatever nigh incomprehensible nested actually, I'm quite sure the entire thing could be done with just plain loops, no recursion required, but for the time being I've allow it to recursive dispatch to allow interleaved message processing, as it could take some time for the chain processing to complete and I'm not sure of the performance impact when you have one (or possibly more) long chain message hogging the worker, maybe a bit of batching for building and consuming part could be done here |
yeah that would work I think
Yeah we definitely need to test that. If that is correct we really have to keep it in mind (especially me, since I probably nuked it multiple times in the past with those changes)
Probably true. However I feel like the job of the ChainActivityHandler is super simple: fetch the missing parents of x activity and redispatch x afterwards. My main goal was to get rid of the self dispatch, as I think it is just to easy to mess that up, be that in the initial implementation or in changes coming in the future. Again you are probably right a lot of it is preference, so maybe I should shut up and let melroy, e-five, debounced and others decide on what implementation they prefer 😅 |
e83b26d
to
60c9e32
Compare
- remove all redispatches of the `ChainActivityMessage`, just get all dependencies one by one in a recursive call - adjust the 3 main sources for it: `LikeHandler`, `DislikeHandler` and `AnnounceHandler` (basically like, dislike and boost something that is not in our db) - adjust the `ApActivityRepository` for better readability and remove the union select with 4 individual ones
60c9e32
to
bbadc20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far so good, everything seems to be working fine.
ChainActivityMessage
, just get all dependencies one by one in a recursive callLikeHandler
,DislikeHandler
andAnnounceHandler
(basically like, dislike and boost something that is not in our db)ApActivityRepository
for better readability and remove the union select with 4 individual onesFor testing:
I tried it with 20 nested comments and purged the entire post. After liking the first comment everything got recreated
I really hope that this fixes one of our worst circular message offenders