Implement .stat() function #159

hacdias · 2018-01-30T09:46:37Z

To implement this, I take a look at Go's repo.stat implementation and I basically do the same on each one except on repoSize.

For numObjects I use the number of blocks on the Blockstore
For repoPath I just used this.path
For version I use this.version.get
For storageMax I use the config Datastore.StorageMax or the maximum integer number.
For repoSize I sum the number of bytes of the blocks and their keys from the Blockstore + the same for the Datastore and the Keystore. This doesn't count with api and ´config` files' size but it won't differ much from the real size.

I also added a simple test.

hacdias · 2018-01-30T10:35:04Z

@pgte @dignifiedquire @diasdavid I think this is done and the tests are passing 😄 Could you take a look and if it looks good, merge and publish a new version?

Needed for ipfs/js-ipfs#1197

dignifiedquire · 2018-01-30T10:35:40Z

src/index.js

+   * @return {void}
+   */
+  stat (callback) {
+    waterfall([


why not do the stats in parallel, and aggregate them to an object at the end?

Although there are some that use the same variable. Wouldn't that cause some problem eventually?

If you require some of them to be series, you can still make the all series, but perhaps use async/series with the object argument to make this less explicit.

pgte

Please see comments.

pgte · 2018-01-30T10:31:40Z

src/blockstore.js

+    query (query, callback) {
+      pull(
+        store.query(query),
+        pull.collect((err, list) => {


I think you can pass callback directly to pull.collect here.

Nice catch! Thanks 😄

pgte · 2018-01-30T10:44:46Z

src/index.js

+   * @return {void}
+   */
+  stat (callback) {
+    waterfall([


Here you're using waterfall to pass the accumulator, but since, by what I gather, each step is creating it's own entry, perhaps you could use async/parallel with the object arguments, collecting them all automatically in the final callback.

Multiple steps use the repoSize. If it was parallel I think I could end up messed up.

So perhaps you could calculate that first, and then proceed to make the rest parallel?

pgte · 2018-01-30T10:45:55Z

src/index.js

+
+function getSize (queryFn, callback) {
+  pull(
+    queryFn.query({}),


Try to avoid buffering: I think you can reduce this iteratively instead of buffering the entire list.

My comment refers to line below where you do pull.collect.

Sorry, but I didn't get this. How can I do this iteratively?

Since the query is a pull-stream, you can use pull.reduce like this:

pull( source, pull.reduce((val) => compute(val), 0, callback))

Oh, didn't know that. Thanks

hacdias · 2018-01-30T11:18:33Z

@dignifiedquire @pgte changes made!

dignifiedquire · 2018-01-30T11:27:51Z

src/index.js

+    options = Object.assign({}, {human: false}, options)
+
+    parallel([
+      (cb) => cb(null, this.path),


no need for this, just use it directly at the end

dignifiedquire · 2018-01-30T11:29:16Z

src/index.js

+      (cb) => this.version.get(cb),
+      (cb) => waterfall([
+        (cb) => cb(null, {
+          repoSize: new Big(0)


no need, just use the value directly

dignifiedquire · 2018-01-30T11:30:37Z

src/index.js

+          stats.repoSize = stats.repoSize.plus(size)
+          cb(err, stats)
+        }),
+        (stats, cb) => {


the below doesn't need to be a callback

dignifiedquire · 2018-01-30T11:31:06Z

src/index.js

+        (cb) => cb(null, {
+          repoSize: new Big(0)
+        }),
+        (stats, cb) => this.blocks.query({}, (err, list) => {


the size fetching below can all happen in parallel, just need to sum at the end

hacdias · 2018-01-30T13:48:40Z

@dignifiedquire @pgte hopefully everything is nicer now 😃

dignifiedquire

two more things :)

dignifiedquire · 2018-01-30T13:51:17Z

test/repo-test.js

+        expect(stats).to.have.property('repoPath')
+        expect(stats).to.have.property('repoSize')
+        expect(stats).to.have.property('storageMax')
+        done()


just realized it now, these don’t test that they are the right numbers, so you should at least check thatj they are greater 0 or similar

@dignifiedquire done! had to move the tests to another file so they run after the other tests add some data to the repo 😄

dignifiedquire · 2018-01-30T13:54:03Z

src/index.js

+        })
+      }),
+      (cb) => getSize(this.datastore, (err, size) => {
+        cb(err, new Big(size))


this can be simplified as getSize already returns a bigint

dignifiedquire · 2018-01-30T14:19:51Z

src/index.js

+        .plus(results[4])
+
+      if (options.human) {
+        size = size.div(1048576)


why not use something like https://www.npmjs.com/package/pretty-bytes instead?

The spec says MiB so it must be in base 2 and not 10. Unless we could find another package. But why importing another package if we can just divide by that number?

Ah okay, the package only makes sense if the output should be dynamically adjusted depending on the size.

dignifiedquire

all my complaints were addressed, looking good

hacdias · 2018-01-30T14:24:02Z

Cool! Thanks @dignifiedquire 😄

pgte · 2018-01-30T14:32:03Z

src/index.js

+      }),
+      (cb) => getSize(this.datastore, cb),
+      (cb) => getSize(this.keys, cb)
+    ], (err, results) => {


Perhaps it would be easier to use the object notation of async/parallel, something like this:

parallel({ a: (cb) => cb(null, 1), b: (cb) => cb(null, 2), c: (cb) => cb(null, 3) }, (err, results) => { if (err) { handleError(err) } else { console.log(results) // { a: 1, b: 2, c: 3} } })

Yeah, it would probably get a bit more clear. I'll do that.

pgte · 2018-01-30T14:32:51Z

@hacdias except for the async/parallel remark, LGTM!

hacdias · 2018-01-30T14:40:16Z

Done and the tests are passing. Could any of you merge this and release a new version when possible? 😃

daviddias · 2018-01-30T19:29:56Z

v0.18.7 released :)

hacdias · 2018-01-30T19:30:50Z

Cool! Thanks

ghost assigned hacdias Jan 30, 2018

ghost added the status/in-progress In progress label Jan 30, 2018

hacdias mentioned this pull request Jan 30, 2018

Implement Stats API ipfs/js-ipfs#1197

Closed

hacdias changed the title ~~WIP: Implement .stat() function~~ Implement .stat() function Jan 30, 2018

hacdias force-pushed the stat branch from b826e5b to 423b177 Compare January 30, 2018 10:29

hacdias requested review from pgte, dignifiedquire and daviddias January 30, 2018 10:30

dignifiedquire reviewed Jan 30, 2018

View reviewed changes

pgte suggested changes Jan 30, 2018

View reviewed changes

hacdias force-pushed the stat branch from d7caea7 to 8732da6 Compare January 30, 2018 11:18

dignifiedquire reviewed Jan 30, 2018

View reviewed changes

hacdias force-pushed the stat branch from 8732da6 to f6971ec Compare January 30, 2018 13:48

dignifiedquire suggested changes Jan 30, 2018

View reviewed changes

hacdias force-pushed the stat branch from f6971ec to 753dfa7 Compare January 30, 2018 14:11

dignifiedquire reviewed Jan 30, 2018

View reviewed changes

dignifiedquire previously approved these changes Jan 30, 2018

View reviewed changes

pgte reviewed Jan 30, 2018

View reviewed changes

feat: implement .stat function

3865142

hacdias dismissed dignifiedquire’s stale review via 3865142 January 30, 2018 14:37

hacdias force-pushed the stat branch from 753dfa7 to 3865142 Compare January 30, 2018 14:37

hacdias mentioned this pull request Jan 30, 2018

feat: stats API (stats.bitswap and stats.repo) ipfs/js-ipfs#1198

Merged

34 tasks

daviddias approved these changes Jan 30, 2018

View reviewed changes

daviddias merged commit bd522ee into master Jan 30, 2018

daviddias deleted the stat branch January 30, 2018 19:27

ghost removed the status/in-progress In progress label Jan 30, 2018

Implement .stat() function #159

Implement .stat() function #159

Conversation

hacdias commented Jan 30, 2018 • edited

hacdias commented Jan 30, 2018

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

pgte 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

hacdias commented Jan 30, 2018

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

hacdias commented Jan 30, 2018

dignifiedquire 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

dignifiedquire left a comment

Choose a reason for hiding this comment

hacdias commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgte commented Jan 30, 2018

hacdias commented Jan 30, 2018

daviddias commented Jan 30, 2018

hacdias commented Jan 30, 2018

hacdias commented Jan 30, 2018 •

edited