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

blockservice: add BlockedFetched/Added/Removed events #4649

Merged
merged 1 commit into from
Mar 23, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 4, 2018

... and remove a few log lines that don't seem terribly useful.

This will be useful for ipfs-repeater. The Bitswap.BlockGetRequest start/end events don't really cut it, they don't tell me that the block has actually been fetched and stored, and that the blockservice is able to provide them.

I put in TODO() contexts there, doing context wiring for Add/Delete felt out of scope, but I can do that too.

... and remove a few log lines that don't seem terribly useful.

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost added topic/tools Topic tools topic/blockstore Topic blockstore labels Feb 4, 2018
@ghost ghost self-assigned this Feb 4, 2018
@ghost ghost added the status/in-progress In progress label Feb 4, 2018
@Stebalien Stebalien requested a review from frrist February 4, 2018 14:17
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

I agree that wiring in the contexts is out of scope for this PR, however I think we will want timing data on these events in the future, so that will need to be done at some point.

@@ -169,6 +171,7 @@ func (s *blockService) AddBlocks(bs []blocks.Block) error {
}

for _, o := range toput {
log.Event(context.TODO(), "BlockService.BlockAdded", o.Cid())
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I like naming the events based on the method name, e.g. BlockService.AddBlocks.

Copy link
Member

Choose a reason for hiding this comment

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

I think in this case it makes sense to have custom named events as they are mostly for functional reasons. It would also mean having 4 different events instead of 2.

@Kubuxu Kubuxu removed the status/in-progress In progress label Mar 9, 2018
@Kubuxu Kubuxu added the RFM label Mar 9, 2018
@Kubuxu Kubuxu added this to the go-ipfs 0.4.15 milestone Mar 9, 2018
@whyrusleeping whyrusleeping merged commit e1208b7 into master Mar 23, 2018
@whyrusleeping whyrusleeping deleted the feat/blockservice-events branch March 23, 2018 06:05
@whyrusleeping
Copy link
Member

merge number two is off! Thanks @lgierth :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFM topic/blockstore Topic blockstore topic/tools Topic tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants