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

Jump to bottom

refactor(bitswap, blockservice) Give bitswap responsibility for local datastore Gets/Puts #388

Closed
wants to merge 89 commits into from
Closed

refactor(bitswap, blockservice) Give bitswap responsibility for local datastore Gets/Puts #388

wants to merge 89 commits into from

Conversation

btc
Copy link
Contributor

@btc btc commented Nov 27, 2014

  1. Despite these facts, bitswap has been saddled with an awkward responsibility of adding blocks received from the network to the datastore. We're making an assumption that callers want this behavior, but is it really true? Blockservice is responsible for mediating client requests. It optimizes performance by consulting the datastore before consulting bitswap. blockservice should add received blocks to the datastore. - @maybebtc

  2. Dividing the responsibility is awkward. do note though that bitswap in the future may retrieve + store blocks for the sake of trading. (blocks not requested by the client). So it will need write access to a datastore/blockstore. Maybe the correct thing to do is sink the full responsibilities into bitswap ("consult the datastore before making network requests," and "write to the datastore when the local client calls HasBlock"). - @jbenet

  3. Here's a PR that gives bitswap more responsibility.

@whyrusleeping @jbenet

We may want to wait until after #386 is merged. All's the same to me though.

whyrusleeping and others added 30 commits November 18, 2014 21:31
@whyrusleeping
beginnings of a bitswap refactor
8fb7a0f
@whyrusleeping
dont panic on empty wantlist
dce36e3
test(bitswap)
e6c5316 
@whyrusleeping This appears to be a timing issue. The asynchronous
nature of the new structure provides has the bitswap waiting on the
context a bit more. This isn't a problem at all, but in this test, it
makes the functions return in an inconveniently timely manner.

TODO don't let the test depend on time.

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
events(bitswap) try the new event logger in the bitswap GetBlock method
6176c62 
@jbenet
@whyrusleeping

Let me know if you want to direct the eventlog output to _both_ the file
and stderr. Right now it goes to file. Perhaps this is just a minor bip
in the larger discussion around log levels.

#292

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(bitswap) handle error
699f1d5 
@whyrusleeping

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(bitswap) consistent event names
4fd10bd 
@whyrusleeping @jbenet

since the logger is created with package scope, don't need to specify
the package name in event messages

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(log) ->f
e576518 
@whyrusleeping

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
use event logger here too?
d891ea9 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
clarify MessageReceived contract
ac05b90 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
naming
6157d5f 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
constify to make it clear what _can_ and _can't_ change over time
ceff380 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
some renaming
ab724e2 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
simplify
9e15ead 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
misc(bitswap) renaming
938e4c6 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@whyrusleeping
added a new test for a dhthell scenario that was failing
17ef046
@whyrusleeping
Merge branch 'bithack' of github.com:jbenet/go-ipfs into bithack
f071975 
Conflicts:
	exchange/bitswap/bitswap.go
@whyrusleeping
move some variables into strategy
db77b55
@whyrusleeping
fix tests halting
5017568
@whyrusleeping
remove buffer timing in bitswap in favor of manual batching
9ebd221
@whyrusleeping
randomize rebroadcast target
352832e
fix(bitswap/notifications) don't force sender to block on receiver
57942fc 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
style(bitswap/notifications) make it more obvious
a11f5c2 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
feat(bitswap/notifications) Subscribe to multiple keys
9b3c040 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
tests(bitswap) share constructor between tests
acf8692 
@whyrusleeping i hope this makes it a bit easier to work with tests

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(merkledag) missing arg
06c4109 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor(bitswap) move wantlist to loop receive
fdd6663 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
feat(bitswap) implement GetBlocks
59ee84d 
@whyrusleeping @jbenet

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(bitswap) stop the ticker when the run loop exits
22a0c9f 
@whyrusleeping

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
tests(bitswap) share code between the two large tests
8c99f60 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@whyrusleeping
start working getBlocks up the call chain
4d89c44
whyrusleeping and others added 15 commits November 24, 2014 08:30
@whyrusleeping
fix issues in merkledag
5eae29e
@whyrusleeping
add a test in merkledag to exercise GetBlocks
8f034a2
refactor(util) move block generator
ffb10e9 
@whyrusleeping @jbenet

Putting the block generator in a util dir until blocks.

Can't put it in util/testutil because the util/testutil/dag-generator
imports blockservice and blockservice uses the generator.

Tough problem. This'll do for now.

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(notifications) prevent deadlock when context cancelled early
11bb58e 
+ test(notifications)

cc @whyrusleeping @jbenet

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(bitswap) pass derived context to called functions
ada5714 
@whyrusleeping @jbenet

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor(bitswap) perform Publish in HasBlock
5720a04 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@whyrusleeping
some bitswap cleanup
0085103
@whyrusleeping
document bitswap more
1d18aa6
feat(bitswap) make offline exchange query datastore
c45e5e4 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
style(blockservice) s/Remote/Exchange
2481c72 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
feat(blockstore) check Has before put
ade719d 
since this is the blockstore, all writes are blocks and all block writes
are expensive.

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix(bitswap) move responsibility for Put into exchange
3e05348 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
feat(blockservice) let bitswap be responsible for local Get
01f0e78 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor(blockservice) in GetBlocks let exchange perform local query
4e780f3 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor(bitswap) consolidate HasBlock
6a8674a 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@btc btc self-assigned this Nov 27, 2014
whyrusleeping
whyrusleeping reviewed Dec 1, 2014
View reviewed changes
blockservice/blockservice.go
err = s.Remote.HasBlock(ctx, b)
}
return k, err
err := s.Exchange.HasBlock(context.TODO(), b)
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to pass a context in

@whyrusleeping
Copy link
Member

I dont know how much I like this PR. Yes, i agree the responsibility split for bitswap is weird, but i dont think the solution should be stripping the BlockService of all responsibility.

@btc
Copy link
Contributor Author

btc commented Dec 1, 2014

The logical result of this course of action is for Blockservice to become an interface that bitswap implements. This PR demonstrates there isn't a need for the blockservice at all. However, we've discussed moving toward a notification center architecture. Keeping the blockservice around provides some flexibility should we choose to explore that route.

With that said, here's an alternative:

Perhaps we just want to wrap the blockstore with a layer that detects recent successful writes and elides duplicates. Then, both blockservice and bitswap can have responsibility for writing and we don't have to pay the large IO write penalty. Instead, we would incur a smaller mutex penalty on Puts as well as a small memory cost. Under the heuristic that duplicate writes occur close (in time) to each other, an LRU cache would perform well.

@whyrusleeping
Copy link
Member

hrm... the idea of making blockservice an interface for bitswap seems fine to me. I guess my gut just says "thats too much responsibility for one object!"

@btc
Copy link
Contributor Author

btc commented Dec 1, 2014

I guess my gut just says "thats too much responsibility for one object!"

I share this concern. If (just if) we were to go this route, my take on it is that the bitswap.bitswap struct is gutted out and broken down into essential components.

  1. The implementation of HasBlock gets moved into the event loop.
  2. The implementation of ReceiveMessage gets moved into the event loop. (Now all mutable state is modified in the event loop)
  3. GetBlock, GetBlocks, HasBlock, and ReceiveMessage communicate with the loop worker over these channels.
BlockRequests chan<- []Key
Messages chan<- BitSwapMessage
Blocks chan<- Block

\4. Then the event loop is basically a frame around the strategy.

That said...

My honest intuition is that we don't have concrete constraints driving us toward a particular architecture yet. All of this is speculative. Thus, a smaller solution that allows for more flexibility is probably wiser.

Caching Blockstore writes is stupid simple and gets the job done. It's just a Blockstore wrapper that modifies Put and delegates all other methods to another Blockstore. As it doesn't force us to commit to any particular decision, it allows future information to dictate our move.

@btc
Copy link
Contributor Author

btc commented Dec 1, 2014

Let's not worry about this change yet. Once bithack is merged, I'll open a PR for a simple write cache.

bithack...bitswap-blockservice-diplomacy

@btc btc closed this Dec 1, 2014
@btc btc deleted the power+responsibility,peter branch December 18, 2014 15:30
@btc btc removed their assignment Mar 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@btc @whyrusleeping @jbenet