Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix publishing #250

Merged
merged 24 commits into from
Jul 8, 2020
Merged

fix publishing #250

merged 24 commits into from
Jul 8, 2020

Conversation

dryajov
Copy link
Contributor

@dryajov dryajov commented Jul 4, 2020

This is a WIP to fix some publishing issues, it duplicates some of the work in #244 so we might consider cherry picking either into this branch or into the #244 branch. I'm leaving it here for now as a reference ( @sinkingsugar )

@sinkingsugar
Copy link
Contributor

sinkingsugar commented Jul 5, 2020

I like it.. I wanted to do this refactor but it would have taken me much more to figure it out.. don't worry about my further changes.. I just added some locks to heartbeat.. which worked 99% of the times.. still triggered nil peer tho

I will just back-off a little bit once we can settle this properly.

@@ -60,7 +60,7 @@ suite "FloodSub":
await nodes[1].subscribe("foobar", handler)
await waitSub(nodes[0], nodes[1], "foobar")

await nodes[0].publish("foobar", "Hello!".toBytes())
discard await nodes[0].publish("foobar", "Hello!".toBytes())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check: await... == 1 (or whatever it should be?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check: 1 == await ... maybe easier to read

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's from my branch btw 😄 , the thing is that this does not apply to flood, it will always be 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw I find it useful (nbc side for example I'm using it) to have this int with gossip. but we can remove it once things stabilize if you guys don't think it's necessary.. it's a debug tool tho

@dryajov dryajov force-pushed the remove-pb-queue-1 branch from a0262a4 to 3fd1b77 Compare July 6, 2020 21:03
@dryajov dryajov force-pushed the remove-pb-queue-1 branch from 3fd1b77 to 4b2616f Compare July 7, 2020 17:06
@dryajov dryajov marked this pull request as ready for review July 8, 2020 00:32
@dryajov dryajov merged commit a52763c into master Jul 8, 2020
@dryajov dryajov deleted the remove-pb-queue-1 branch July 8, 2020 00:33

libp2p_gossipsub_peers_per_topic_gossipsub
.set(g.gossipsub.getOrDefault(topic).len.int64, labelValues = [topic])
if allPeers.len == 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allpeers can't be zero in here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants