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

Events expansion for the progress tracker to provide more detailed information about pin status #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reinerRubin
Copy link

@reinerRubin reinerRubin commented May 28, 2019

@michaelavila

I have tried to implement a small events system for the progress tracker. I wanted to expose a channel as a more natural interface for events. But we can not take the risk of blocking a "pin" goroutine. So there is some kind of "infinity" channel inside.

idk, it seems overcomplicated, but exposing a slice of events seemed also terrible.

It's a draft version just to show my idea. I am going to add more tests (and fix race), make it more clear and rename some things. Plus to try integrate this to go-ipfs itself. Feel free to reject this approach, I am not very attached to this idea.

ipfs/kubo#6042

@michaelavila
Copy link

@reinerRubin Hey, that was fast! Thanks! 🎉

Also, thanks for putting up a draft version! It helps a lot.

Others can weigh in, but I think your intuition about being "complicated" for a first version is on the right track. I can imagine that we will need a robust progress tracker in time, but for now I'd try and figure out a simpler change.

What if we start by just reporting the CIDs that we start pinning? I think, in that case, we need to add just a little bit of information to the Increment() call and modify the bits of the command code that render results to the user. Getting even a simple change like what I'm suggesting merged will require working through a few details (e.g. how to store and surface even simple events). If we can get that stuff worked out, it should make further work on progress tracking better. Does that make sense?

@reinerRubin
Copy link
Author

reinerRubin commented May 28, 2019

But if I get it right, progress is the consequence of asking the tracker how it is going by ticker. So the tracker has to have some state. As I see tracker can store a slice of planned to pin CIDs and return this slice on demand with flushing his current state. So the pin command can iterate through and emit (AddPinOutput). It was my first idea but I was not sure it's ok enough (plus I am not fun of polling by timer).

If it's ok, then I can do this version (it would be like channel but much simpler).

@michaelavila
Copy link

michaelavila commented May 29, 2019

@reinerRubin Try it out! My only suggestion is to push often so you can get more feedback. Maybe figure out how you will surface these events to the user (like you said originally)?

@reinerRubin reinerRubin force-pushed the feature/6042-expand-progress-tracker-with-events branch from 3787392 to 8dcc6d9 Compare June 2, 2019 20:05
@reinerRubin
Copy link
Author

@michaelavila, I have tried the approach above and written a test to simulate things that are expected to be in ipfs command.

@michaelavila
Copy link

michaelavila commented Jun 3, 2019

@reinerRubin want to take a crack at surfacing these events to the user? I know that one goal is to surface these events during the ipfs pin add command.

@reinerRubin
Copy link
Author

reinerRubin commented Jun 3, 2019

@michaelavila, yeah. I am going to prepare the second PR to the ipfs (pin add command) itself. How must it be organized? Should I make PR with "hacked" vendor and then set actual version after merging this one?

// About your comment. I am not sure about "PlanToPin" maybe "Pinning" would fit better.

@michaelavila
Copy link

How must it be organized? Should I make PR with "hacked" vendor and then set actual version after merging this one?

You can use the replace directive in the go.mod file of the go-ipfs repository to reference a specific commit of go-merkledag.

That will work for the time being. Eventually, when you get the go-merkledag work merged we can tag a new version of it and update the version in the go.mod file in go-ipfs. Until then, just reference a commit directly.

Let me know if I should clarify anything or if you run into trouble.

@reinerRubin
Copy link
Author

Ok, got it.

@reinerRubin reinerRubin force-pushed the feature/6042-expand-progress-tracker-with-events branch from 8dcc6d9 to 3ff70c5 Compare June 5, 2019 08:34
@reinerRubin reinerRubin changed the title (WIP) Events expansion for the progress tracker to provide more detailed information about pin status Events expansion for the progress tracker to provide more detailed information about pin status Jun 5, 2019
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.

2 participants