Round out diagnostic parameters#157
Merged
Merged
Conversation
b89ba29 to
30c6a18
Compare
Add a libp2p over http test. Add max request in progress configurable parameter. Add block diagnostics. Add readme explaining parameters
30c6a18 to
678dc3e
Compare
raulk
approved these changes
Mar 5, 2021
raulk
reviewed
Mar 5, 2021
Comment on lines
+414
to
+420
| var resp *http.Response | ||
| var err error | ||
| if useLibP2p { | ||
| resp, err = client.Get(fmt.Sprintf("libp2p://%s/%s", p.peerAddr.ID.String(), c.String())) | ||
| } else { | ||
| resp, err = client.Get(fmt.Sprintf("http://%s:8080/%s", p.ip.String(), c.String())) | ||
| } |
Member
There was a problem hiding this comment.
This will make the HTTP case run in parallel with the Graphsync nodes that haven't finished yet, thus making the results unreliable.
We could synchronize on a barrier, then call runtime.GC() to try to free resources. But I generally advise against running (a) the subject of test and (b) the control in the same process, as results won't be reliable. It's best to run these completely isolated from one another.
@aarshkshah1992 will be doing a large refactor in this test plan, turning it into a generic data transfer plan that:
- runs only one transport: graphsync, tcp, http, http-over-libp2p, raw-libp2p.
- allows you to select a muxer and a secure channel to instantiate the host with, for libp2p transports.
Collaborator
Author
There was a problem hiding this comment.
@raulk a couple things:
- There already is a generic transfer plan -- Beyond Bitswap -- I'm not sure it's the right thing for your use case, but you should at least give it a look before writing from scratch, as it actually supports much more complex data transfers (real system directories, etc)
- If you want to write a large refactor that's fine, but I think a generic transfer plan should live outside the graphsync repo
- I don't believe the parallel cases make that much of a difference in the over all conclusion. I've posted in thread as to why I think this and also wrote a quick hack to run the HTTP/ HTTP over libp2p seperately, which produced the same end result. I think that conclusively proves the problem is not in graphsync, and I'd like to ideally move the work out of the repo to reflect that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goals
Add some final parameters and diagnostics to the graphsync testplan, and document exactly what all the parameters do
Implementation