Issue #446 Adding peername to PinInfo #531

kishansagathiya · 2018-09-08T05:35:14Z

This commit adds peername to PinInfo and GlobalPinInfo so that we have
a nicer and more meaningfull output for queries like
ipfs-cluster-ctl status

So far this is a naive attempt. Quite a few things remaining here to add and to change.
Fixes #446

coveralls · 2018-09-08T05:44:41Z

Coverage increased (+0.03%) to 65.172% when pulling c62c922 on kishansagathiya:issue_446 into 275a8b2 on ipfs:master.

lanzafame

LGTM, there are just stray comments.

lanzafame · 2018-09-10T01:44:56Z

ipfs-cluster-ctl/main.go

@@ -601,6 +601,7 @@ The filter only takes effect when listing all pins. The possible values are:
 			},
 		},
 		{
+			// This could be one


@kishansagathiya Are these comments still required?

kishansagathiya · 2018-09-12T04:35:59Z

Examples of relevant command runs

[kishansagathiya@localhost ipfs-cluster]$ ipfs-cluster-ctl status
QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S :
    > Peer cluster2 : PINNED | 2018-09-12T04:18:52Z
    > Peer cluser1 : PINNED | 2018-09-12T04:18:52Z
[kishansagathiya@localhost ipfs-cluster]$ ipfs-cluster-ctl status QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S
QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S :
    > Peer cluster2 : PINNED | 2018-09-12T04:18:52Z
    > Peer cluser1 : PINNED | 2018-09-12T04:18:52Z
[kishansagathiya@localhost ipfs-cluster]$ ipfs-cluster-ctl sync
[kishansagathiya@localhost ipfs-cluster]$ ipfs-cluster-ctl sync QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S
QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S :
    > Peer cluster2 : PINNED | 2018-09-12T04:18:52Z
    > Peer cluser1 : PINNED | 2018-09-12T04:18:52Z
[kishansagathiya@localhost ipfs-cluster]$ ipfs-cluster-ctl recover
An error occurred:
  Code: 400
  Message: only requests with parameter local=true are supported
[kishansagathiya@localhost ipfs-cluster]$ ipfs-cluster-ctl recover QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S
QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S :
    > Peer cluster2 : PINNED | 2018-09-12T04:18:52Z
    > Peer cluser1 : PINNED | 2018-09-12T04:18:52Z
[kishansagathiya@localhost ipfs-cluster]$ git log

@hsanjuan @lanzafame
May be the output could be formatted for pipes to align with each other. Yet to write tests for this, but feel free to review so far.

kishansagathiya · 2018-09-12T06:34:42Z

rpc_api.go

@@ -134,6 +134,7 @@ func (rpcapi *RPCAPI) Status(ctx context.Context, in api.PinSerial, out *api.Glo
 func (rpcapi *RPCAPI) StatusLocal(ctx context.Context, in api.PinSerial, out *api.PinInfoSerial) error {
 	c := in.ToPin().Cid
 	pinfo := rpcapi.c.StatusLocal(c)
+	pinfo.PeerName = rpcapi.c.config.Peername


I would get rid off this. It should work without this.

kishansagathiya · 2018-09-12T14:52:48Z

@lanzafame travis checks have passed, but somehow it still shows pending.

lanzafame · 2018-09-13T01:45:50Z

@kishansagathiya I restarted the job on travis and this time round it ~~failed~~ timed out. Not sure why it didn't move out of pending.

Forgot to mention the addition of sharness test

lanzafame · 2018-09-13T02:03:38Z

Final thing, since this directly affects the output of the cli tool, it would be great to add a sharness test, see the sharness directory. Have a look around the different test files for examples of testing the output of a command and the actual test would go in t0025-ctl-status-report-commands.sh. Thanks

kishansagathiya · 2018-09-15T04:12:52Z

@lanzafame Going through current sharness tests, I don't think proposed change would require us to modify or add to sharness tests.
Because the proposed change does not really change the output, but it makes it meaningful. But I wonder what can be added to these(sharness) tests regardless of this change.

One more thing, earlier output was well formatted since the hash had a fixed size. I was thinking we should put some size limit to peerName so that we can keep this output also well formatted.

lanzafame · 2018-09-17T02:50:56Z

Because the proposed change does not really change the output, but it makes it meaningful. But I wonder what can be added to these(sharness) tests regardless of this change.

Good point. Nvm the sharness 👍

One more thing, earlier output was well formatted since the hash had a fixed size. I was thinking we should put some size limit to peerName so that we can keep this output also well formatted.

A naive approach would be to use a \t (tab) character between the peerName and the : in the formatting string, i.e.

- fmt.Printf("    > Peer %s : %s", v.PeerName, strings.ToUpper(v.Status))
+ fmt.Printf("    > Peer %s\t: %s", v.PeerName, strings.ToUpper(v.Status))

The other option is to use the padding syntax of fmt.Printf:

- fmt.Printf("    > Peer %s : %s", v.PeerName, strings.ToUpper(v.Status))
+ fmt.Printf("    > Peer %-10s: %s", v.PeerName, strings.ToUpper(v.Status))

The padding value, 10, is just an example, not sure what the ideal would be in terms of best looking output.

I will leave the choice between to the two approaches up to you. For now, I would rather not limit the size of the string and just go with ugly output if someone use a really long peer name.

GitCop · 2018-09-18T02:13:05Z

There were the following issues with your Pull Request

Commit: d0b1d51
Invalid signoff. Commit message must end with
License: MIT
Signed-off-by:

Code contribution guidelines for IPFS Cluster are available at https://cluster.ipfs.io/developer/contribute/#code-contribution-guidelines

This message was auto-generated by https://gitcop.com

GitCop · 2018-09-18T02:13:52Z

There were the following issues with your Pull Request

Commit: 18576ab
Invalid signoff. Commit message must end with
License: MIT
Signed-off-by:

Code contribution guidelines for IPFS Cluster are available at https://cluster.ipfs.io/developer/contribute/#code-contribution-guidelines

This message was auto-generated by https://gitcop.com

kishansagathiya · 2018-09-18T02:52:46Z

@lanzafame One travis run is failing. I tried to replicate failure locally, but I couldn't. I think if we re-run travis check, it will pass.

hsanjuan

Thanks @kishansagathiya . This looks very good. Mostly cosmetic changes needed.

hsanjuan · 2018-09-18T20:53:15Z

api/types.go

+	Status   TrackerStatus
+	TS       time.Time
+	Error    string
+	PeerName string


Pedantic but could you move up PeerName under Peer?

hsanjuan · 2018-09-18T20:53:26Z

api/types.go

+	Status   string `json:"status"`
+	TS       string `json:"timestamp"`
+	Error    string `json:"error"`
+	PeerName string `json:"peername"`


Here and in the other places below.

hsanjuan · 2018-09-18T20:56:04Z

cluster.go

+				Status:   api.TrackerStatusClusterError,
+				TS:       time.Now(),
+				Error:    e.Error(),
+				PeerName: c.config.Peername,


I think we should not set PeerName here. You are setting the local peer's name, but the PinInfo object may belong to any other peer.

So maybe set it to members[i] instead?

hsanjuan · 2018-09-18T21:00:40Z

ipfs-cluster-ctl/formatters.go

@@ -147,7 +147,7 @@ func textFormatPrintGPInfo(obj *api.GlobalPinInfoSerial) {

 	for _, k := range peers {
 		v := obj.PeerMap[k]
-		fmt.Printf("    > Peer %s : %s", k, strings.ToUpper(v.Status))
+		fmt.Printf("    > Peer %-10s : %s", v.PeerName, strings.ToUpper(v.Status))


If v.PeerName is set it should show the PeerName, otherwise it should show the peer ID. Depends on how comment above is resolved.

We can remove the Peer part. I think it's understood and saves some space. We can increase padding to 15. I'd like to see an example of the output though.

Sample output

[kishansagathiya@localhost ipfs-cluster]$ ipfs-cluster-ctl status QmU6PQs58j4UieXg8qte9qdvckmts7sWiVkeHov86ZVW1S : > cluster2 : PINNED | 2018-09-24T17:20:56Z > cluser1 : PINNED | 2018-09-24T17:20:56Z

hsanjuan · 2018-09-18T21:04:15Z

pintracker/optracker/operationtracker.go

+			Status:   api.TrackerStatusUnpinned,
+			TS:       time.Now(),
+			Error:    "",
+			PeerName: "",


I believe opt.peerName is usable here?

my bad! operation is nil and not opt!

This commit adds peername to PinInfo and GlobalPinInfo so that we have a nicer and more meaningfull output for queries like `ipfs-cluster-ctl status` License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

This commit adds peername to PinInfo and GlobalPinInfo so that we have a nicer and more meaningfull output for `ipfs-cluster-ctl` queries like `status`, `sync` and `recover` License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Removed comments and code used for debugging License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Removed unnecessary peername assignment Modified tests according to the changes made to add peername to PinInfo License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Formatted output License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Fixing errors and better code placement License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

hsanjuan · 2018-09-25T19:49:58Z

cluster.go

-				Error:  e.Error(),
+				Cid:      h,
+				Peer:     members[i],
+				PeerName: string(members[i]),


This should be members[i].String() or members[i].Pretty() (leaning towards the first) . Unfortunately casting a peer.ID to string directly is a bad thing to do (been there, done that).

Makes sense. I was able to find a nice article explaining why.
https://stackoverflow.com/questions/4167304/why-should-casting-be-avoided

Actually, the reason here is that a peer ID is internally a string but that string is just holding a byte slice (a multihash actually) and the string representation we use for peer IDs is the B58 encoded version of that byte slice. https://godoc.org/github.com/libp2p/go-libp2p-peer#IDB58Encode . The reason why a Peer ID is a string and not a []byte probably comes to the slightly smaller footprint and immutability of strings (https://syslog.ravelin.com/byte-vs-string-in-go-d645b67ca7ff). In any case, the right way to get a string is to IDB58Encode it.

Avoid typecasting License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

hsanjuan · 2018-09-26T11:26:56Z

really happy to have this

ghost assigned kishansagathiya Sep 8, 2018

ghost added the status/in-progress In progress label Sep 8, 2018

lanzafame suggested changes Sep 10, 2018

View reviewed changes

kishansagathiya commented Sep 12, 2018

View reviewed changes

kishansagathiya force-pushed the issue_446 branch from 0c32b4f to 115e42b Compare September 12, 2018 14:31

kishansagathiya added need/review Needs a review and removed status/in-progress In progress labels Sep 12, 2018

lanzafame previously approved these changes Sep 13, 2018

View reviewed changes

ghost added the status/in-progress In progress label Sep 18, 2018

kishansagathiya force-pushed the issue_446 branch from d0b1d51 to 18576ab Compare September 18, 2018 02:13

kishansagathiya force-pushed the issue_446 branch from 18576ab to 93a8301 Compare September 18, 2018 02:15

lanzafame approved these changes Sep 18, 2018

View reviewed changes

lanzafame added RFM Ready for Merge and removed status/in-progress In progress need/review Needs a review labels Sep 18, 2018

hsanjuan requested changes Sep 18, 2018

View reviewed changes

lanzafame removed the RFM Ready for Merge label Sep 19, 2018

kishansagathiya added 2 commits September 24, 2018 23:04

Issue ipfs-cluster#446 Adding peername to PinInfo

ff48fb3

This commit adds peername to PinInfo and GlobalPinInfo so that we have a nicer and more meaningfull output for queries like `ipfs-cluster-ctl status` License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

kishansagathiya added 3 commits September 24, 2018 23:04

Issue ipfs-cluster#446 Adding peername to PinInfo

773b4de

Removed comments and code used for debugging License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Issue ipfs-cluster#446 Adding peername to PinInfo

b76b65e

Removed unnecessary peername assignment Modified tests according to the changes made to add peername to PinInfo License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Issue ipfs-cluster#446 Adding peername to PinInfo

61c03e2

Formatted output License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

kishansagathiya force-pushed the issue_446 branch from 93a8301 to 0580fd5 Compare September 24, 2018 17:43

ghost added the status/in-progress In progress label Sep 24, 2018

Issue ipfs-cluster#446 Adding peername to PinInfo

2cd4420

Fixing errors and better code placement License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

kishansagathiya force-pushed the issue_446 branch from 0580fd5 to 2cd4420 Compare September 24, 2018 17:51

hsanjuan requested changes Sep 25, 2018

View reviewed changes

Issue ipfs-cluster#446 Adding peername to PinInfo

c62c922

Avoid typecasting License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

hsanjuan approved these changes Sep 26, 2018

View reviewed changes

hsanjuan merged commit e941d6b into ipfs-cluster:master Sep 26, 2018

ghost removed the status/in-progress In progress label Sep 26, 2018

hsanjuan mentioned this pull request Sep 26, 2018

Release 0.6.0 #522

Closed

kishansagathiya deleted the issue_446 branch November 6, 2019 15:38

kishansagathiya restored the issue_446 branch November 6, 2019 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #446 Adding peername to PinInfo #531

Issue #446 Adding peername to PinInfo #531

kishansagathiya commented Sep 8, 2018

coveralls commented Sep 8, 2018 •

edited

lanzafame left a comment

lanzafame Sep 10, 2018

kishansagathiya commented Sep 12, 2018 •

edited

kishansagathiya Sep 12, 2018

kishansagathiya commented Sep 12, 2018 •

edited

lanzafame commented Sep 13, 2018 •

edited

lanzafame commented Sep 13, 2018

kishansagathiya commented Sep 15, 2018 •

edited

lanzafame commented Sep 17, 2018

GitCop commented Sep 18, 2018

GitCop commented Sep 18, 2018

kishansagathiya commented Sep 18, 2018

hsanjuan left a comment

hsanjuan Sep 18, 2018

hsanjuan Sep 18, 2018

hsanjuan Sep 18, 2018

hsanjuan Sep 18, 2018

hsanjuan Sep 18, 2018

hsanjuan Sep 18, 2018

kishansagathiya Sep 24, 2018

hsanjuan Sep 18, 2018

kishansagathiya Sep 19, 2018

hsanjuan Sep 25, 2018

kishansagathiya Sep 26, 2018

hsanjuan Sep 26, 2018

hsanjuan commented Sep 26, 2018

Issue #446 Adding peername to PinInfo #531

Issue #446 Adding peername to PinInfo #531

Conversation

kishansagathiya commented Sep 8, 2018

coveralls commented Sep 8, 2018 • edited

lanzafame left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya commented Sep 12, 2018 • edited

Choose a reason for hiding this comment

kishansagathiya commented Sep 12, 2018 • edited

lanzafame commented Sep 13, 2018 • edited

lanzafame commented Sep 13, 2018

kishansagathiya commented Sep 15, 2018 • edited

lanzafame commented Sep 17, 2018

GitCop commented Sep 18, 2018

GitCop commented Sep 18, 2018

kishansagathiya commented Sep 18, 2018

hsanjuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsanjuan commented Sep 26, 2018

coveralls commented Sep 8, 2018 •

edited

kishansagathiya commented Sep 12, 2018 •

edited

kishansagathiya commented Sep 12, 2018 •

edited

lanzafame commented Sep 13, 2018 •

edited

kishansagathiya commented Sep 15, 2018 •

edited