Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

pss: Limit the number of workers forwarding messages in outbox #1860

Merged
merged 11 commits into from
Oct 15, 2019
Merged

pss: Limit the number of workers forwarding messages in outbox #1860

merged 11 commits into from
Oct 15, 2019

Conversation

kortatu
Copy link
Contributor

@kortatu kortatu commented Oct 4, 2019

Whenever a message is taken from the outbox a new go routine is fired. There used to be no limit of parallel goroutines (only the size of the outbox queue that was 100000). Now a new channel of size numWorkers has been implemented to limit parallel processing.
This results in a new limit when adding a message to the outbox. If the numbers of parallel workers has been reached the routine trying to enqueue it will block.
Additionally, the size of the queue has been decreased to 10000.
This PR solves issue #1802, and point 2 in #1855. Is built on top of pending PR #1744 (refactor pss outbox).

@kortatu kortatu added bug pss builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. labels Oct 4, 2019
@kortatu kortatu self-assigned this Oct 4, 2019
@janos
Copy link
Member

janos commented Oct 4, 2019

Should #1744 be closed in favour to this PR? As this PR is based on it.

@kortatu kortatu mentioned this pull request Oct 4, 2019
Closed
@kortatu
Copy link
Contributor Author

kortatu commented Oct 4, 2019

Should #1744 be closed in favour to this PR? As this PR is based on it.

Yes, that PR could be closed if this is reviewed instead ot it.

@janos
Copy link
Member

janos commented Oct 4, 2019

And #1861 is based from the same branch and shares most of the changes. Which one of three PRs is correct to review?

@kortatu
Copy link
Contributor Author

kortatu commented Oct 4, 2019 •
edited
Loading

And #1861 is based from the same branch and shares most of the changes. Which one of three PRs is correct to review?

This one, and then #1861. We can close #1744 (or merge #1744 and continue with this only reviewing the actual changes this PR relates to)

@janos
Copy link
Member

janos commented Oct 4, 2019

In that case of dependent PRs, maybe it is better to remove ready for review label from #1861 until this PR is merged.

@kortatu
Copy link
Contributor Author

kortatu commented Oct 4, 2019

In that case of dependent PRs, maybe it is better to remove ready for review label from #1861 until this PR is merged.

Ok, done

@kortatu kortatu changed the title pss: Limit the number of workers forwarding message in outbox pss: Limit the number of workers forwarding messages in outbox Oct 4, 2019
@janos
Copy link
Member

janos commented Oct 4, 2019

Given that #1860 and #1861 are based on #1744, wouldn't it be easier to review additional changes if new PRs are targeted to #1744 branch instead to master? That would not cause the code showing it diff in all PRs, and also preserve the original PR and its comments in one PR.

@kortatu
Copy link
Contributor Author

kortatu commented Oct 4, 2019

Given that #1860 and #1861 are based on #1744, wouldn't it be easier to review additional changes if new PRs are targeted to #1744 branch instead to master? That would not cause the code showing it diff in all PRs, and also preserve the original PR and its comments in one PR.

#1744 branch is not in etheresphere/swarm but in epiclabs-io/swarm, so the PR would need to be created in a different repository and you could not review it.

janos
janos reviewed Oct 4, 2019
View reviewed changes
pss/outbox/outbox.go Outdated Show resolved Hide resolved
pss/outbox/message.go Show resolved Hide resolved
pss/outbox/outbox.go Show resolved Hide resolved
pss/outbox/outbox.go Outdated Show resolved Hide resolved
pss/outbox/message.go Show resolved Hide resolved
pss/outbox/outbox_test.go Outdated Show resolved Hide resolved
@janos janos mentioned this pull request Oct 7, 2019
Closed
janos
janos reviewed Oct 7, 2019
View reviewed changes
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. I think that the code is fine, the comments can be formatted a bit better (full sentences, with a space after //), which would be very nice as already is in most of the codebase.

We should sync about next steps as there is another track with @nonsense @acud trying to tackle the same problems in parallel. I see that some work has already been done over the weekend on #1867.

pss/outbox/outbox.go Outdated
Forward forwardFunction //Function that executes the actual forwarding
}

//Outbox type
Copy link
Member

Choose a reason for hiding this comment

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

This comment can be improved.

janos
janos approved these changes Oct 7, 2019
View reviewed changes
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Thanks @kortatu. I would wait for merging this PR until we sync with @nonsense and @acud.

@kortatu kortatu added ready for review and removed builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. labels Oct 9, 2019
@nonsense nonsense self-requested a review October 15, 2019 07:35
@janos janos mentioned this pull request Oct 15, 2019
Closed
@kortatu kortatu merged commit 3c1c30c into ethersphere:master Oct 15, 2019
@kortatu kortatu deleted the issue-1802 branch October 15, 2019 09:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nonsense nonsense nonsense approved these changes

@janos janos janos approved these changes

Assignees

@kortatu kortatu

Labels
bug pss ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kortatu @janos @nonsense @aperonam