From 566a86f5d4a3e7e64d98dcada09f44c4e8eaf9e6 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Fri, 9 Jan 2015 19:04:13 +0000 Subject: [PATCH] Address PR comments and add in more user feedback --- core/commands/ping.go | 50 ++++++++++++++++++++---------- routing/dht/dht.go | 6 ++-- routing/dht/routing.go | 7 +++-- routing/mock/centralized_client.go | 5 +-- routing/routing.go | 3 +- 5 files changed, 47 insertions(+), 24 deletions(-) diff --git a/core/commands/ping.go b/core/commands/ping.go index cfd28b118..0943ffdcd 100644 --- a/core/commands/ping.go +++ b/core/commands/ping.go @@ -2,7 +2,6 @@ package commands import ( "bytes" - "errors" "fmt" "io" "time" @@ -19,6 +18,7 @@ const kPingTimeout = 10 * time.Second type PingResult struct { Success bool Time time.Duration + Text string } var PingCmd = &cmds.Command{ @@ -33,7 +33,10 @@ send pings, wait for pongs, and print out round-trip latency information. `, }, Arguments: []cmds.Argument{ - cmds.StringArg("count", false, true, "Number of pings to perform"), + cmds.StringArg("peer ID", true, true, "ID of peer to be pinged"), + }, + Options: []cmds.Option{ + cmds.IntOption("count", "n"), }, Marshalers: cmds.MarshalerMap{ cmds.Text: func(res cmds.Response) (io.Reader, error) { @@ -49,7 +52,9 @@ send pings, wait for pongs, and print out round-trip latency information. } buf := new(bytes.Buffer) - if obj.Success { + if len(obj.Text) > 0 { + buf = bytes.NewBufferString(obj.Text + "\n") + } else if obj.Success { fmt.Fprintf(buf, "Pong took %.2fms\n", obj.Time.Seconds()*1000) } else { fmt.Fprintf(buf, "Pong failed\n") @@ -75,9 +80,11 @@ send pings, wait for pongs, and print out round-trip latency information. } if len(req.Arguments()) == 0 { - return nil, errors.New("no peer specified!") + return nil, cmds.ClientError("no peer specified!") } + outChan := make(chan interface{}, 5) + // Set up number of pings numPings := 10 val, found, err := req.Option("count").Int() @@ -94,33 +101,44 @@ send pings, wait for pongs, and print out round-trip latency information. return nil, err } - // Make sure we can find the node in question - ctx, _ := context.WithTimeout(context.Background(), kPingTimeout) - p, err := n.Routing.FindPeer(ctx, peerID) - if err != nil { - return nil, err - } - - outChan := make(chan interface{}) - go func() { defer close(outChan) + + // Make sure we can find the node in question + outChan <- &PingResult{ + Text: fmt.Sprintf("Looking up peer %s", peerID.Pretty()), + } + ctx, _ := context.WithTimeout(context.Background(), kPingTimeout) + p, err := n.Routing.FindPeer(ctx, peerID) + if err != nil { + outChan <- &PingResult{Text: "Peer lookup error!"} + outChan <- &PingResult{Text: err.Error()} + return + } + outChan <- &PingResult{ + Text: fmt.Sprintf("Peer found, starting pings."), + } + + var total time.Duration for i := 0; i < numPings; i++ { ctx, _ = context.WithTimeout(context.Background(), kPingTimeout) - before := time.Now() - err := n.Routing.Ping(ctx, p.ID) + took, err := n.Routing.Ping(ctx, p.ID) if err != nil { log.Errorf("Ping error: %s", err) outChan <- &PingResult{} break } - took := time.Now().Sub(before) outChan <- &PingResult{ Success: true, Time: took, } + total += took time.Sleep(time.Second) } + averagems := total.Seconds() * 1000 / float64(numPings) + outChan <- &PingResult{ + Text: fmt.Sprintf("Average latency: %.2fms", averagems), + } }() return outChan, nil diff --git a/routing/dht/dht.go b/routing/dht/dht.go index d6a6d8770..78fef653b 100644 --- a/routing/dht/dht.go +++ b/routing/dht/dht.go @@ -103,8 +103,8 @@ func (dht *IpfsDHT) Connect(ctx context.Context, npeer peer.ID) error { // Ping new peer to register in their routing table // NOTE: this should be done better... - if err := dht.Ping(ctx, npeer); err != nil { - return fmt.Errorf("failed to ping newly connected peer: %s\n", err) + if _, err := dht.Ping(ctx, npeer); err != nil { + return fmt.Errorf("failed to ping newly connected peer: %s", err) } log.Event(ctx, "connect", dht.self, npeer) dht.Update(ctx, npeer) @@ -329,7 +329,7 @@ func (dht *IpfsDHT) PingRoutine(t time.Duration) { peers := dht.routingTable.NearestPeers(kb.ConvertKey(u.Key(id)), 5) for _, p := range peers { ctx, _ := context.WithTimeout(dht.Context(), time.Second*5) - err := dht.Ping(ctx, p) + _, err := dht.Ping(ctx, p) if err != nil { log.Errorf("Ping error: %s", err) } diff --git a/routing/dht/routing.go b/routing/dht/routing.go index 5978a9a80..cd929c255 100644 --- a/routing/dht/routing.go +++ b/routing/dht/routing.go @@ -3,6 +3,7 @@ package dht import ( "math" "sync" + "time" context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context" @@ -434,12 +435,14 @@ func (dht *IpfsDHT) FindPeersConnectedToPeer(ctx context.Context, id peer.ID) (< } // Ping a peer, log the time it took -func (dht *IpfsDHT) Ping(ctx context.Context, p peer.ID) error { +func (dht *IpfsDHT) Ping(ctx context.Context, p peer.ID) (time.Duration, error) { // Thoughts: maybe this should accept an ID and do a peer lookup? log.Debugf("ping %s start", p) + before := time.Now() pmes := pb.NewMessage(pb.Message_PING, "", 0) _, err := dht.sendRequest(ctx, p, pmes) log.Debugf("ping %s end (err = %s)", p, err) - return err + + return time.Now().Sub(before), err } diff --git a/routing/mock/centralized_client.go b/routing/mock/centralized_client.go index da8e39692..4a9b63b01 100644 --- a/routing/mock/centralized_client.go +++ b/routing/mock/centralized_client.go @@ -2,6 +2,7 @@ package mockrouting import ( "errors" + "time" context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context" ds "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore" @@ -79,8 +80,8 @@ func (c *client) Provide(_ context.Context, key u.Key) error { return c.server.Announce(info, key) } -func (c *client) Ping(ctx context.Context, p peer.ID) error { - return nil +func (c *client) Ping(ctx context.Context, p peer.ID) (time.Duration, error) { + return 0, nil } var _ routing.IpfsRouting = &client{} diff --git a/routing/routing.go b/routing/routing.go index 934a00c41..8238aa45c 100644 --- a/routing/routing.go +++ b/routing/routing.go @@ -3,6 +3,7 @@ package routing import ( "errors" + "time" context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context" @@ -38,5 +39,5 @@ type IpfsRouting interface { FindPeer(context.Context, peer.ID) (peer.PeerInfo, error) // Ping a peer, log the time it took - Ping(context.Context, peer.ID) error + Ping(context.Context, peer.ID) (time.Duration, error) }