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

Add version of Pump() with status callbacks #4

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

KyleMaas
Copy link
Contributor

@KyleMaas KyleMaas commented Jan 16, 2023

This adds a version of the Pump() function which takes callback functions so the caller knows where we're at in the processing cycle.

This came up while seeking a solution for ssbc/go-ssb#251. Since the Luigi Pump()s were all being run in goroutines, we had no way to know if the indexes had caught up with the main log to know whether or not they're in sync, which caused all kinds of problems. This gives us a very easy way to fix that.

Copy link
Member

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@cryptix cryptix left a comment

Choose a reason for hiding this comment

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

I like this, the only thing i'm not sure about anymore if the PushSource interface upgrade is used in go-ssb. I know we added it for some reason but... 🙈

In either case I'd probably remove it from this new function since it sidesteps the callbacks.

@KyleMaas
Copy link
Contributor Author

@cryptix Thanks for the feedback! Not sure I can answer that question - I just copied it from the other version, so if it needs to be removed, it probably needs to be removed from both.

@cryptix
Copy link
Member

cryptix commented Jan 19, 2023

I don't have the time to do a full scan for this but a cursory look just turned up two, probably also outdated, utilities in go-ssb that actually use PushSource. So it might be fine to replace and will probably help to reduce luigi's surface area for a future replacment anyway. I would have expected margaret or go-ssb's indexes to use this at least. Or it could have been for go-muxrpc, which used luigi before I ditched it there in v2.

context: Not sure how familiar you are with Go's stdlib interfaces but Pump is akin to io.Copy and PushSource was intended as something like WriteTo. Back then we were way to ignorant about the use of interface{} and the overhead it introduces on the runtime and the developers... Sadly i never got around to re-write margaret without using luigi. I started an experiment for this over here. ((holz)riese is the german word for flume)

@KyleMaas
Copy link
Contributor Author

So are we good to merge this, then, and try and tackle removing PushSource in a subsequent pull request against both projects?

@decentral1se
Copy link
Member

PushSource follow ups are #5 / ssbc/go-ssb#304

@KyleMaas
Copy link
Contributor Author

Awesome. Thanks!

@KyleMaas KyleMaas deleted the add-wait-callbacks branch January 19, 2023 19:55
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.

3 participants