floodsub api #377

victorb · 2016-09-13T11:43:33Z

First iteration on the publish/subscribe that is being introduced in go-ipfs

Missing things

~~Readme information about IPFS_EXEC having to be pointed to go-ipfs running with pub/sub when running tests~~ no necessary if we get go-ipfs-npm to use new binary instead
Information about how to use it
go-ipfs-npm having access to the pubsub, so it'll be downloaded when running
Add --discover flag
Rebase once reviewing is done
Rebase from master again

dignifiedquire · 2016-09-13T11:48:20Z

src/api/pubsub.js

+      }
+
+      var rs = new Stream()
+      rs.readable = true


you can use through2 instead, that way you can pipe the respons into your transform

In the end, we've decided to keep using Stream for now, and maybe investigate using through2 in the future instead.

haadcode · 2016-09-13T12:08:55Z

Looking good @victorbjelkholm! Couple of small notes to simplify the code.

Would you want to expand the tests to include a test for peerA to peerB to make sure two peers are talking? This doesn't really fall under unit tests but imo would be good to get into a habit of writing simple integration tests, too. That way our test suites can spot when deps break. Not needed for this PR to be merged imo, but would be nice addition.

dignifiedquire · 2016-09-13T12:31:36Z

src/api/pubsub.js

+      rs.cancel = () => {
+        request.abort()
+        activeSubscriptions = removeSubscription(activeSubscriptions, topic)
+      }


this is now defined twice

Yeah, my thinking is that if you're canceling the subscription before you got one message, we need to cancel the subscription and update the list of subscriptions.

Correct, but the outer definition is enough, you are only duplicating the same code in the inner one.

Ah, of course. Thanks

victorb · 2016-09-13T13:41:27Z

@haadcode

Thanks for reviewing!

Would you want to expand the tests to include a test for peerA to peerB to make sure two peers are talking?

Test for pub/sub between nodes added 👍

victorb · 2016-09-14T12:21:35Z

@haadcode @dignifiedquire @diasdavid finished all outstanding tasks, only thing missing is a rebase.

Can you confirm everything looks good?

dignifiedquire · 2016-09-14T12:59:05Z

README.md

+ipfsApi.pubsub.subscribe('my-topic', (err, subscription) => {
+	if (err) {
+		console.log('Something went wrong setting up the subscription')
+		throw err


Please use 2 space indents

Sure, got no linting for .md files ;)

dignifiedquire · 2016-09-14T14:24:44Z

If CI is happy, I am happy

daviddias · 2016-09-14T14:28:11Z

src/api/pubsub.js

+  return Object.assign({}, message, {
+    from: bs58.encode(message.from),
+    data: Base64.decode(message.data),
+    seqno: Base64.decode(message.seqno)


@whyrusleeping you are base64 encoding all the messages?

nvm, it is just when it comes through the http-api

base64 encoding happens on buffers in json

victorb · 2016-09-14T16:15:41Z

@dignifiedquire @diasdavid Tests passes in CircleCI, Travis? Not so much... Everything is passing in local as well, so not sure what's going on. If you could please take a look, I would be forever grateful:

  1) .log .log.tail:

     Uncaught AssertionError: expected undefined to exist

      at test/interface-ipfs-core/log.spec.js:34:23

      at src/api/log.js:9:590

      at src/request-api.js:9:2732

      at options.beforeRedirect.finish (node_modules/wreck/lib/index.js:229:20)

      at wrapped (node_modules/hoek/lib/index.js:871:20)

      at ClientRequest.options.beforeRedirect.onResponse (node_modules/wreck/lib/index.js:180:20)

      at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:433:21)

      at HTTPParser.parserOnHeadersComplete (_http_common.js:103:23)

      at Socket.socketOnData (_http_client.js:322:20)

      at readableAddChunk (_stream_readable.js:153:18)

      at Socket.Readable.push (_stream_readable.js:111:10)

      at TCP.onread (net.js:536:20)

https://travis-ci.org/ipfs/js-ipfs-api/jobs/159910037

This commit adds publish/subscribe methods to js-ipfs-api. These changes requires special version of go-ipfs currently, with pub/sub activated.

victorb · 2016-09-15T12:02:24Z

For future reference: travis is trash. I'm working from a branch in my repo and in a PR to this repo. I accidently pushed to upstream repo once, deleted the branch shortly after, and now Travis is trying to use the upstream branch instead of from my fork. So the Travis status here is from js-ipfs-api/feat-pubsub but this PR is actually from VictorBjelkholm/feat-pubsub. Just to keep in mind

victorb · 2016-09-15T12:09:39Z

@diasdavid @dignifiedquire Finally, everything is passing and CR comments have been addressed.

dignifiedquire · 2016-09-15T12:23:30Z

@victorbjelkholm do we want to merge this into master or into a separate branch? Given that usually master corresponds with what's available in a go-ipfs release I would suggest to put it into a pubsub branch and release it under a tag, rather than an official release.

victorb · 2016-09-15T12:28:36Z

@dignifiedquire good question. I think separate branch is good enough, and then release it under a tag, as you suggested.

Question is, how do we keep it up to date with master? Manual merges?

The branch already live in ipfs/js-ipfs-api under the name feat-pubsub, maybe we can just close this and make a release from there?

dignifiedquire · 2016-09-15T12:55:53Z

The branch already live in ipfs/js-ipfs-api under the name feat-pubsub, maybe we can just close this and make a release from there?

Sounds good to me.

Lets wait for @diasdavid to wake up and ask him if he has any objections

daviddias · 2016-09-15T19:48:28Z

Let's do branch (feat/floodsub) only and keep that branch in this repo.

I still need review the code, but I can do it under the new branch, will CR once I get to integrate floodsub it in js-ipfs and think through the API as a whole for both lands (already had some discussions with @whyrusleeping on how to make sure we don't make the API very HTTP biased, as the HTTP should be invisible). This probably won't happen until after DEVCON.

victorb · 2016-09-17T16:29:34Z

@diasdavid Sounds good, I'll leave this PR as-is for now then.

daviddias · 2016-10-17T15:14:48Z

can we namespace the api under floodsub? I'm worried that we might have to make a breaking API change in the future when we get tree forming, if we use pubsub from the start.

victorb · 2016-10-17T15:29:37Z

@diasdavid

can we namespace the api under floodsub?

Might be confusing if we mix (comparing to go-ipfs where it's under pubsub rather than floodsub). What you think?

daviddias · 2016-10-17T16:32:10Z

@victorbjelkholm true (I'm still trying to convince @whyrusleeping to namespace it under floodsub too :D)

victorb · 2016-10-17T16:58:07Z

@diasdavid ah, haha, I see... Well, if you want me to move, I'll do that, I would agree that it could make it easier for the future.

dignifiedquire · 2016-10-20T17:36:29Z

I have a rebased branch on my machine which I will push tomorrow.

haadcode · 2016-11-18T07:00:16Z

I'm refactoring this as the implementation here was always considered temp. The work happens here: https://github.com/haadcode/js-ipfs-api/blob/fix/pubsub/src/api/pubsub.js.

There's still a lot to be done, but got it up to a fairly good point yesterday. I added a really nice test to make sure messages flow nicely: https://github.com/haadcode/js-ipfs-api/blob/fix/pubsub/test/ipfs-api/pubsub.spec.js#L157.

Will keep you updated.

daviddias · 2016-12-06T18:08:47Z

@haadcode Is this the PR that I should be looking at and testing against the tests at interface-ipfs-core?

daviddias · 2016-12-06T18:34:17Z

@haadcode should we close this PR in favor of one coming from https://github.com/haadcode/js-ipfs-api ? //cc @victorbjelkholm

haadcode · 2016-12-06T18:54:57Z

Yeah, we can close this.

daviddias · 2016-12-06T18:57:56Z

Follow #470

victorb added the in progress label Sep 13, 2016

dignifiedquire reviewed Sep 13, 2016
View reviewed changes

victorb force-pushed the feat-pubsub branch from f2fef38 to df2c2a1 Compare September 14, 2016 12:05

dignifiedquire reviewed Sep 14, 2016
View reviewed changes

daviddias reviewed Sep 14, 2016
View reviewed changes

victorb force-pushed the feat-pubsub branch 2 times, most recently from 59f8d9f to 6e51424 Compare September 15, 2016 07:54

feat(pubsub): Add publish/subscribe

7d7e4c1

This commit adds publish/subscribe methods to js-ipfs-api. These changes requires special version of go-ipfs currently, with pub/sub activated.

victorb force-pushed the feat-pubsub branch from 6e51424 to 7d7e4c1 Compare September 15, 2016 11:01

victorb removed the in progress label Sep 17, 2016

daviddias added the js-ipfs-backlog label Sep 26, 2016

daviddias changed the title ~~Publish/Subscribe~~ floodsub api Oct 17, 2016

This was referenced Nov 8, 2016

Sprint: Nov 7 -> Nov 13 ipfs/team-mgmt#246

Closed

track: floodsub ipfs/js-ipfs#580

Closed

victorb mentioned this pull request Nov 15, 2016

Add possibility to get all peers of a pubsub topic to js-ipfs-api #423

Closed

daviddias added ready and removed js-ipfs-backlog labels Dec 5, 2016

daviddias closed this Dec 6, 2016

daviddias removed the ready label Dec 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

floodsub api #377

floodsub api #377

victorb commented Sep 13, 2016 •

edited

Loading

dignifiedquire Sep 13, 2016

victorb Sep 13, 2016

haadcode commented Sep 13, 2016

dignifiedquire Sep 13, 2016

victorb Sep 13, 2016 •

edited

Loading

dignifiedquire Sep 14, 2016

victorb Sep 14, 2016

victorb commented Sep 13, 2016 •

edited

Loading

victorb commented Sep 14, 2016

dignifiedquire Sep 14, 2016

victorb Sep 14, 2016

dignifiedquire Sep 14, 2016

dignifiedquire commented Sep 14, 2016

daviddias Sep 14, 2016

daviddias Sep 14, 2016

jbenet Sep 15, 2016

victorb commented Sep 14, 2016

victorb commented Sep 15, 2016

victorb commented Sep 15, 2016 •

edited

Loading

dignifiedquire commented Sep 15, 2016

victorb commented Sep 15, 2016

dignifiedquire commented Sep 15, 2016

daviddias commented Sep 15, 2016 •

edited

Loading

victorb commented Sep 17, 2016

daviddias commented Oct 17, 2016

victorb commented Oct 17, 2016

daviddias commented Oct 17, 2016

victorb commented Oct 17, 2016

dignifiedquire commented Oct 20, 2016

haadcode commented Nov 18, 2016

daviddias commented Dec 6, 2016

daviddias commented Dec 6, 2016

haadcode commented Dec 6, 2016

daviddias commented Dec 6, 2016

floodsub api #377

floodsub api #377

Conversation

victorb commented Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode commented Sep 13, 2016

Choose a reason for hiding this comment

victorb Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorb commented Sep 13, 2016 • edited Loading

victorb commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorb commented Sep 14, 2016

victorb commented Sep 15, 2016

victorb commented Sep 15, 2016 • edited Loading

dignifiedquire commented Sep 15, 2016

victorb commented Sep 15, 2016

dignifiedquire commented Sep 15, 2016

daviddias commented Sep 15, 2016 • edited Loading

victorb commented Sep 17, 2016

daviddias commented Oct 17, 2016

victorb commented Oct 17, 2016

daviddias commented Oct 17, 2016

victorb commented Oct 17, 2016

dignifiedquire commented Oct 20, 2016

haadcode commented Nov 18, 2016

daviddias commented Dec 6, 2016

daviddias commented Dec 6, 2016

haadcode commented Dec 6, 2016

daviddias commented Dec 6, 2016

Follow #470

victorb commented Sep 13, 2016 •

edited

Loading

victorb Sep 13, 2016 •

edited

Loading

victorb commented Sep 13, 2016 •

edited

Loading

victorb commented Sep 15, 2016 •

edited

Loading

daviddias commented Sep 15, 2016 •

edited

Loading