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

fix: refresh pins and stats at better rates #606

Merged
merged 1 commit into from
Feb 6, 2018
Merged

fix: refresh pins and stats at better rates #606

merged 1 commit into from
Feb 6, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 28, 2018

Fixes #600.

Ping @Cole128

@makew0rld
Copy link

@hacdias Looks good 😄 , although will indirect pins show up? And how do I install it for my mac since it hasn't been packaged as a dmg yet?

@hacdias
Copy link
Member Author

hacdias commented Jan 28, 2018

@Cole128 indirect pins won't show up. The only thing I added was the refreshing at every one sec.

About the .dmg:

image

When one of those little guys gets a check mark saying the build passed, you can click on Details --> Artifacts --> And there should be your dmg 😄

@makew0rld
Copy link

@hacdias Great! Once I install it and test it out I'll let you know whether it works.

@codecov-io
Copy link

codecov-io commented Jan 28, 2018

Codecov Report

Merging #606 into master will decrease coverage by 0.1%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
- Coverage   15.21%   15.11%   -0.11%     
==========================================
  Files          57       57              
  Lines         874      880       +6     
==========================================
  Hits          133      133              
- Misses        741      747       +6
Impacted Files Coverage Δ
src/controls/main/stats.js 0% <0%> (ø) ⬆️
src/screens/menubar.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c27f22d...e2c7ce2. Read the comment docs.

@makew0rld
Copy link

@hacdias I can't open the dmg, it says image not recognized.

@hacdias
Copy link
Member Author

hacdias commented Jan 28, 2018

Could you try on the other link? I don't have a macOS and I can't really help with that but please try the other link. Otherwise, tomorrow I can try to make it rebuild the image.

@makew0rld
Copy link

@hacdias I tried both.

@victorb
Copy link
Member

victorb commented Jan 28, 2018

@Cole128 could you please comment with the exact link you downloaded the .dmg from and also what macOS version you're running?

@hacdias
Copy link
Member Author

hacdias commented Jan 29, 2018

@Cole128 Could you try this one?


Just as a sidenote: builds are failing due to sinonjs/nise#27

@ghost
Copy link

ghost commented Jan 29, 2018

Calling pin/ls once per second can cause quite the load on go-ipfs and the harddrive. Is it really neccessary to have this happen every second?

If live updates are neccessary, we could instead emit pin-added and pin-removed events (we might already be doing that), which ipfs-desktop could listen to.

@ghost
Copy link

ghost commented Jan 29, 2018

we could instead emit pin-added and pin-removed events

To clarify, I mean the ipfs log command.

@hacdias
Copy link
Member Author

hacdias commented Jan 29, 2018

@lgierth didn't know about that!! Thanks. I'll check that out and see what if there are any events related to the pins.

@ghost
Copy link

ghost commented Jan 29, 2018

About the proposed code in this PR, it looks like it'll fire another pin/ls command no matter if the previous one has returned already. This means you can end up with multiple in-flight pin/ls commands, causing additional load. You should queue the next command only once the current one has returned. That way slow responses are treated as back-pressure.

@hacdias
Copy link
Member Author

hacdias commented Jan 29, 2018

@lgierth yeah, I've noticed that before lunch and I'm planning to change it to only call the command again one second after having the other's response.

How should I listen to those events? Using ipfs.log.tail([callback])? It seems a bit impractical since it doesn't allow me to handle specific event types.

@ghost
Copy link

ghost commented Jan 29, 2018

Mh it looks like the ipfs log command is quite odd -- it seems like ipfs log tail lets deals with events, but ipfs log ls and ipfs log level deal with log output. (Events and log output are two different things).

Maybe a filter argument to ipfs log tail is neccessary, or just moving the events stuff into its own command.

@hacdias hacdias changed the title fix: refresh pins every 1 second fix: refresh pins every 5 seconds and stats every 3 seconds Jan 29, 2018
@hacdias hacdias force-pushed the update-pin branch 2 times, most recently from 597cc7f to 51181f5 Compare January 29, 2018 14:51
@hacdias hacdias changed the title fix: refresh pins every 5 seconds and stats every 3 seconds fix: refresh pins and stats at better rates Jan 29, 2018
@hacdias
Copy link
Member Author

hacdias commented Jan 29, 2018

@Cole128 here's a new dmg!

@makew0rld
Copy link

makew0rld commented Feb 1, 2018

@hacdias I still get the error: Image not recognized.
Do any of you guys have a mac to test it out on?

@hacdias
Copy link
Member Author

hacdias commented Feb 1, 2018

@Cole128 unfortunately, I don't. Maybe @lgierth?

@hacdias
Copy link
Member Author

hacdias commented Feb 6, 2018

I'll merge this for now and then we can try to check out what's happening with the DMG.

@hacdias hacdias merged commit 628dea1 into master Feb 6, 2018
@hacdias hacdias deleted the update-pin branch February 6, 2018 10:47
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.

4 participants