Added filter option to `ipfs-cluster-ctl status` #627

kishansagathiya · 2018-12-20T08:26:07Z

When the --filter is passed, it will only fetch the peer information
where status of the pin matches with the filter value.
Valid filter values are tracker status types(i.e., "pinned",
"pin_error", "unpinning" etc), an alias of tracker status type (i.e.,
"queued" or "error"), comma separated list of tracker status type
and/or it aliases(i.e., "error,pinning")

On passing invalid filter value no status information will be shown

In particular, the filter would remove elements from []GlobalPinInfo
when none of the peers in GlobalPinInfo match the filter. If one peer
in the GlobalPinInfo matches the filter, the whole object is returned,
including the information for the other peers which may or not match it.

filter option works on statusAll("GET /pins"). For fetching pin status
for a CID("GET /pins/"), filter option would have no effect

Continueing from #478

Fixes #445

kishansagathiya · 2018-12-20T11:09:10Z

ipfs-cluster status --help print filter value instead of just filter

.// at end of the help message 
.
OPTIONS:
   --local         run operation only on the contacted peer
   --filter value  Comma seperated list of type tracker status and their aliases

Would you know why?

lanzafame · 2018-12-21T04:34:14Z

api/rest/client/methods.go

@@ -136,9 +136,13 @@ func (c *defaultClient) Status(ci cid.Cid, local bool) (api.GlobalPinInfo, error
 }

 // StatusAll gathers Status() for all tracked items.
-func (c *defaultClient) StatusAll(local bool) ([]api.GlobalPinInfo, error) {
+// If valid filter value is provided, it would fetch only those status information
+// where status is matching the filter value


missing full stop

lanzafame · 2018-12-21T04:34:26Z

api/rest/client/methods.go

+// If valid filter value is provided, it would fetch only those status information
+// where status is matching the filter value
+// Valid filter values are tracker status type, an alias of tracker status type
+// (queued or error), comma separated list of track status type and/or it aliases


missing full stop

api/rest/client/methods_test.go

lanzafame · 2018-12-21T04:44:31Z

api/rest/restapi.go

 	if local == "true" {
 		var pinInfos []types.PinInfoSerial
+
 		err := api.rpcClient.Call("",


While changing this function, can you update api.rpcClient.Call to api.rpcClient.CallContext and pass in the Request context r.Context().

Also can you fix the multilining of the function call so that params aren't on the same line as parens.

lanzafame · 2018-12-21T04:44:45Z

api/rest/restapi.go

 	} else {
-		var pinInfos []types.GlobalPinInfoSerial
 		err := api.rpcClient.Call("",


Same as above.

lanzafame · 2018-12-21T04:55:05Z

cmd/ipfs-cluster-ctl/main.go

+tracker status type and/or it aliases ("error,pinning")
+On passing invalid filter value no status information will be shown
+List of tracker status types
+https://github.com/ipfs/ipfs-cluster/blob/319c41cbf195b0453b8d1987991280d3121bac93/api/types.go#L66


No point using the commit url, as we will have to update this url every time we update the list. May as well just put all the filter values here and add a comment in api/types to remind people that if they add to the list of filters, they need to update this comment with the new values.

You can loop and pretty print that list here. An invalid filter should throw an error (catching it both in the client (to avoid making a request for nothing) and in the server side (for people that doesn't use our client)).

hsanjuan · 2018-12-21T12:48:10Z

ipfs-cluster status --help print filter value instead of just filter
.// at end of the help message 
.
OPTIONS:
   --local         run operation only on the contacted peer
   --filter value  Comma seperated list of type tracker status and their aliases
Would you know why?

Because the --filter argument takes a value. It's not just a boolean flag.

api/rest/client/methods.go

hsanjuan · 2018-12-21T12:59:10Z

api/rest/restapi.go

@@ -669,27 +669,74 @@ func (api *API) allocationHandler(w http.ResponseWriter, r *http.Request) {
 	}
 }

+func match(filters []string, status string) bool {
+	for _, filter := range filters {


We should convert to TrackerStatus (TrackerStatusFromString()) and compare with the original and not strings. The filter is a user-supplied string, so it should be checked for validity. We should probably move this function to api/types.go too. We will need to match API functionality in the Go API exposed by the main component (different PR).

Additionally, maybe worth to convert TrackerStatus from iota to 1 << iota and have TrackerStatusError = TrackerStatusPinError | TrackerStatusUnpinError | TrackerStatusClusterError and so on.. thoughts @lanzafame ? we did this with PinType and I think things were ok, it should not break anything since TrackerStatus is not persisted anywhere.

We should convert to TrackerStatus (TrackerStatusFromString()) and compare with the original and not strings.

Can't do this with aliases such as error or queued

We will need to match API functionality in the Go API exposed by the main component

Don't understand this.
Do you mean this code ? https://github.com/ipfs/ipfs-cluster/blob/17599dac639cf6fd70343c72c24b7a8f037c733f/rpc_api.go#L111-L123
and https://github.com/ipfs/ipfs-cluster/blob/17599dac639cf6fd70343c72c24b7a8f037c733f/cluster.go#L753-L763

@hsanjuan @lanzafame All comments addressed, except this one

I mean https://github.com/ipfs/ipfs-cluster/blob/master/api/types.go#L582.

It would look like (untested):

diff --git a/api/types.go b/api/types.go index 1234d09..bc3f426 100644 --- a/api/types.go +++ b/api/types.go @@ -34,7 +34,7 @@ var logger = logging.Logger("apitypes") // TrackerStatus values const ( // IPFSStatus should never take this value - TrackerStatusBug TrackerStatus = iota + TrackerStatusBug TrackerStatus = 1 << iota // The cluster node is offline or not responding TrackerStatusClusterError // An error occurred pinning @@ -60,6 +60,12 @@ const ( TrackerStatusSharded ) +// Composite TrackerStatus +const ( + TrackerStatusError = TrackerSTatusClusterError | TrackerStatusPinError | TrackerStatusUnpinError + TrackerStatusQueued = TrackerStatusPinQueued | TrackerStatusUnpinQueued +) + // TrackerStatus represents the status of a tracked Cid in the PinTracker type TrackerStatus int @@ -75,6 +81,8 @@ var trackerStatusString = map[TrackerStatus]string{ TrackerStatusRemote: "remote", TrackerStatusPinQueued: "pin_queued", TrackerStatusUnpinQueued: "unpin_queued", + TrackerStatusError: "error", + TrackerStatusQueued: "queued", }

hsanjuan · 2018-12-21T13:01:03Z

api/rest/restapi_test.go

+
+		// Test with filter
+		var resp3 []api.GlobalPinInfoSerial
+		makeGet(t, rest, url(rest)+"/pins?filter=queue", &resp3)


should be "queued" ?

hsanjuan · 2018-12-21T13:01:52Z

cmd/ipfs-cluster-ctl/main.go

+
+When the --filter is passed, it will only fetch the peer information
+where status of the pin matches with the filter value.
+Valid filter values are tracker status types("pinned", "pin_error", "unpinning" etc),


just list the values

hsanjuan · 2018-12-21T13:04:45Z

cmd/ipfs-cluster-ctl/main.go

 `,
 			ArgsUsage: "[CID]",
 			Flags: []cli.Flag{
 				localFlag(),
+				cli.StringFlag{
+					Name:  "filter",
+					Usage: "Comma seperated list of type tracker status and their aliases",


Comma-separated list of filters. The list of acceptable values are on the command help which is printed with the command a few lines over this.

kishansagathiya · 2018-12-26T05:55:57Z

What is https://github.com/ipfs/ipfs-cluster/blob/master/sharness/t0025-ctl-status-report-commands.sh for ? It has more than tests for just ctl status. This would help me decide if I should create a new file for adding sharness tests for ctl status

hsanjuan · 2019-01-02T09:43:49Z

api/types.go

@@ -93,6 +93,36 @@ func TrackerStatusFromString(str string) TrackerStatus {
 	return TrackerStatusBug
 }

+func IsFilterValid(filter string) bool {


Adding Queued and Error to TrackerStatus, this should be a oneliner like TrackerStatusFromString(filter) != TrackerStatusBug. Also, probably better to convert to TrackerStatus first and then call a func (st TrackerStatus) IsValid() bool

hsanjuan · 2019-01-02T09:47:20Z

cmd/ipfs-cluster-ctl/main.go

-List of tracker status types
-https://github.com/ipfs/ipfs-cluster/blob/319c41cbf195b0453b8d1987991280d3121bac93/api/types.go#L66
+Valid filter values are tracker status types(full list of tracker status
+types includes "bug", "cluster_error", "pin_error", "unpin_error", "pinned",


Please build this list programatically. You can add a helper function in api which loops the trackerStatusString and returns a list of valid status strings.

hsanjuan

Now that we have bit-flag TrackerStatuses, we can use bitwise comparsion to do matching:

// Match returns true if the tracker status matches the given filter.
// For example TrackerStatusPinError will match TrackerStatusPinError and TrackerStatusError
func (ts TrackerStatus) Match(filter TrackerStatus) bool {
    return ts & f > 0
}

api/types.go should not deal or operate with string versions of TrackerStatus.

api/rest will need:

A method to convert a query string in the form of "filter1,filter2,filter3" to TrackerStatus (worth removing whitespaces from the string before splitting). The filter is built the filter as the bitwise OR of the parsed values (ts1 || ts2 || ts3)
Use api.TrackerStatusFromString(pinInfo.Status).Match(filter) in the method filtering globalPinInfos.

This saves the most-inner loop which compares each Status to each filter value because now all the filter values can be represented as a single thing. Thus the complexity of this method is not exponentially worse with every extra filter value.

api/types.go

cmd/ipfs-cluster-ctl/main.go

License: MIT Signed-off-by: Paul Jewell <sona1111@zoho.com>

@lanzafame

-Fixed logic issue in match condition of 'filterStatus' function -Added and verified success of test provided by @lanzafame -Attempted to condense code and apply other cleanup provided by @lanzafame License: MIT Signed-off-by: Paul Jewell <sona1111@zoho.com>

-Changed some 'snake case' to 'camel case' in accordance with code climate License: MIT Signed-off-by: Paul Jewell <sona1111@zoho.com>

Added filter option to `ipfs-cluster-ctl status` When the --filter is passed, it will only fetch the peer information where status of the pin matches with the filter value. Valid filter values are tracker status types(i.e., "pinned", "pin_error", "unpinning" etc), an alias of tracker status type (i.e., "queued" or "error"), comma separated list of tracker status type and/or it aliases(i.e., "error,pinning") On passing invalid filter value no status information will be shown In particular, the filter would remove elements from []GlobalPinInfo when none of the peers in GlobalPinInfo match the filter. If one peer in the GlobalPinInfo matches the filter, the whole object is returned, including the information for the other peers which may or not match it. filter option works on statusAll("GET /pins"). For fetching pin status for a CID("GET /pins/<cid>"), filter option would have no effect Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Added clients tests Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Added a fail case where an invalid filter is passed in. Update `api.rpcClient.Call` to `api.rpcClient.CallContext` and pass in the Request context r.Context() so that context can be cancelled when the request is cancelled by caller Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Passing `make check` Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Improved matching of filters and tracker status Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

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

coveralls · 2019-01-05T09:34:06Z

Coverage increased (+0.1%) to 65.581% when pulling 964ced6 on issue_445 into 595db7a on master.

Optimized filter to tracker status matching by using bitwise comparisions License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan · 2019-01-07T18:50:51Z

@kishansagathiya I have ended wrapping this up. Please check my last commits. Sorry, it was difficult to figure out the details and realize of some small issues without actually messing with it myself.

hsanjuan · 2019-01-07T18:51:38Z

I have not tested this much though, would be grateful if you can check things still work as expected.

kishansagathiya

OK Tested locally,

However I have a small comment, let me know what you think about that. I will change it accordingly and fix tests.

api/rest/client/methods.go

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan · 2019-01-08T15:01:08Z

api/types_test.go

@@ -29,7 +29,7 @@ var testPeerID6, _ = peer.IDB58Decode("QmR8Vu6kZk7JvAN2rWVWgiduHatgBq2bb15Yyq8RR
 func TestTrackerFromString(t *testing.T) {
 	testcases := []string{"undefined", "cluster_error", "pin_error", "unpin_error", "pinned", "pinning", "unpinning", "unpinned", "remote"}
 	for i, tc := range testcases {
-		if TrackerStatusFromString(tc).String() != TrackerStatus(1<<uint(i)).String() {
+		if tc != TrackerStatus(1<<uint(i)).String() && tc != TrackerStatus(0).String() {


I had already fixed this

hsanjuan · 2019-01-08T15:08:07Z

@kishansagathiya please don't force push on 1) branches worked on by multiple people 2) In progress non-trivial reviews. It makes it difficult to follow up on changes.

hsanjuan · 2019-01-08T15:12:00Z

I'm going to force-push the original branch on top, the last commit should be reverted anyway.

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan · 2019-01-08T15:19:30Z

Sorry! I just realized I had not fixed that test after all. Should be better now. :/

kishansagathiya · 2019-01-08T16:16:39Z

@hsanjuan All checks have passed including travis pr check, somehow here it was showing pending(I have seen this happening before). Anyway I have restarted hopefully, that problem wont appear again.

kishansagathiya · 2019-01-08T17:15:37Z

@hsanjuan finally after two reruns, phew!

hsanjuan · 2019-01-08T17:26:28Z

Hmm yeah, every few weeks I spend a couple of days figuring out why tests have become flaky. Maybe it's time for a new round.

ghost assigned kishansagathiya Dec 20, 2018

ghost added the status/in-progress In progress label Dec 20, 2018

kishansagathiya force-pushed the issue_445 branch from 5716d8c to 0d2fb75 Compare December 20, 2018 08:38

kishansagathiya force-pushed the issue_445 branch from 0d2fb75 to 20b13f7 Compare December 20, 2018 11:14

kishansagathiya requested review from lanzafame and hsanjuan and removed request for lanzafame December 20, 2018 11:48

lanzafame suggested changes Dec 21, 2018

View reviewed changes

hsanjuan requested changes Dec 21, 2018

View reviewed changes

hsanjuan requested changes Jan 2, 2019

View reviewed changes

hsanjuan requested changes Jan 4, 2019

View reviewed changes

hsanjuan reviewed Jan 4, 2019

View reviewed changes

cmd/ipfs-cluster-ctl/main.go Outdated Show resolved Hide resolved

sona1111 and others added 10 commits January 5, 2019 14:44

Fix #445: Implemented status filter for ipfs-cluster-ctl

7c04dd1

License: MIT Signed-off-by: Paul Jewell <sona1111@zoho.com>

Fix #445: Reduced length of filter help string

02af6c1

License: MIT Signed-off-by: Paul Jewell <sona1111@zoho.com>

Fix #445:

bda9633

-Fixed logic issue in match condition of 'filterStatus' function -Added and verified success of test provided by @lanzafame -Attempted to condense code and apply other cleanup provided by @lanzafame License: MIT Signed-off-by: Paul Jewell <sona1111@zoho.com>

Fix #445:

ed20241

-Changed some 'snake case' to 'camel case' in accordance with code climate License: MIT Signed-off-by: Paul Jewell <sona1111@zoho.com>

Status filters for ipfs-cluster-ctl status

02e129f

Added clients tests Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Status filters for ipfs-cluster-ctl status

d0d9034

Passing `make check` Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Status filters for ipfs-cluster-ctl status

2c364dd

Improved matching of filters and tracker status Fixes #445 License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

Wrap help message for less than 120 characters

822c95a

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

kishansagathiya force-pushed the issue_445 branch from 0ace1a8 to 70db2a0 Compare January 5, 2019 09:15

Status filters for ipfs-cluster-ctl status

706558c

Optimized filter to tracker status matching by using bitwise comparisions License: MIT Signed-off-by: Kishan Mohanbhai Sagathiya <kishansagathiya@gmail.com>

kishansagathiya force-pushed the issue_445 branch from 70db2a0 to 706558c Compare January 5, 2019 10:15

Feat #445: Use TrackerStatus as filter. Simplify and small misc.

7de930b

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

ghost assigned hsanjuan Jan 7, 2019

Feat #445: Clarify about 0 filter value.

75702c2

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

kishansagathiya commented Jan 8, 2019

View reviewed changes

api/rest/client/methods.go Show resolved Hide resolved

hsanjuan added 2 commits January 8, 2019 12:28

Feat #445: Catch invalid filter strings in ipfs-cluster-ctl.

fa5906c

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

Issue #445: Fix test

2e692e8

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

kishansagathiya force-pushed the issue_445 branch from 434ee5c to ab29c79 Compare January 8, 2019 14:16

hsanjuan reviewed Jan 8, 2019

View reviewed changes

hsanjuan force-pushed the issue_445 branch from ab29c79 to 2e692e8 Compare January 8, 2019 15:12

Feat #445: Fix string test with TrackerStatus == 0

964ced6

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan approved these changes Jan 8, 2019

View reviewed changes

hsanjuan merged commit d80f3ee into master Jan 8, 2019

ghost removed the status/in-progress In progress label Jan 8, 2019

hsanjuan deleted the issue_445 branch January 8, 2019 17:26

hsanjuan mentioned this pull request Jan 8, 2019

Release 0.8.0 #620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added filter option to `ipfs-cluster-ctl status` #627

Added filter option to `ipfs-cluster-ctl status` #627

kishansagathiya commented Dec 20, 2018

kishansagathiya commented Dec 20, 2018 •

edited

lanzafame Dec 21, 2018

lanzafame Dec 21, 2018

lanzafame Dec 21, 2018

lanzafame Dec 21, 2018

lanzafame Dec 21, 2018

lanzafame Dec 21, 2018

hsanjuan Dec 21, 2018

hsanjuan commented Dec 21, 2018

hsanjuan Dec 21, 2018 •

edited

kishansagathiya Dec 26, 2018

kishansagathiya Dec 26, 2018

hsanjuan Jan 2, 2019

hsanjuan Dec 21, 2018

hsanjuan Dec 21, 2018

hsanjuan Dec 21, 2018

kishansagathiya commented Dec 26, 2018

hsanjuan Jan 2, 2019

hsanjuan Jan 2, 2019

hsanjuan left a comment

coveralls commented Jan 5, 2019 •

edited

hsanjuan commented Jan 7, 2019

hsanjuan commented Jan 7, 2019

kishansagathiya left a comment

hsanjuan Jan 8, 2019

hsanjuan commented Jan 8, 2019

hsanjuan commented Jan 8, 2019 •

edited

hsanjuan commented Jan 8, 2019

kishansagathiya commented Jan 8, 2019

kishansagathiya commented Jan 8, 2019

hsanjuan commented Jan 8, 2019

Added filter option to ipfs-cluster-ctl status #627

Added filter option to ipfs-cluster-ctl status #627

Conversation

kishansagathiya commented Dec 20, 2018

kishansagathiya commented Dec 20, 2018 • edited

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 Dec 21, 2018

hsanjuan Dec 21, 2018 • edited

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

kishansagathiya commented Dec 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsanjuan left a comment

Choose a reason for hiding this comment

coveralls commented Jan 5, 2019 • edited

hsanjuan commented Jan 7, 2019

hsanjuan commented Jan 7, 2019

kishansagathiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsanjuan commented Jan 8, 2019

hsanjuan commented Jan 8, 2019 • edited

hsanjuan commented Jan 8, 2019

kishansagathiya commented Jan 8, 2019

kishansagathiya commented Jan 8, 2019

hsanjuan commented Jan 8, 2019

Added filter option to `ipfs-cluster-ctl status` #627

Added filter option to `ipfs-cluster-ctl status` #627

kishansagathiya commented Dec 20, 2018 •

edited

hsanjuan Dec 21, 2018 •

edited

coveralls commented Jan 5, 2019 •

edited

hsanjuan commented Jan 8, 2019 •

edited