-
Notifications
You must be signed in to change notification settings - Fork 171
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
trickle-server: Don't auto-create channels on first publish. #3251
Conversation
Channels can still be created locally on the server. This is needed in case a payment fails then we need to close the channel but we don't want the publisher to re-open them.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3251 +/- ##
===================================================
- Coverage 34.71608% 34.70842% -0.00766%
===================================================
Files 136 136
Lines 36208 36216 +8
===================================================
Hits 12570 12570
- Misses 22926 22934 +8
Partials 712 712
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR @j0sh Looks good, but I have two questions.
-
I believe we should delay the first payment check and allow all to start and check the first payment after 10, maybe 30s. In other words we take the optimistic approach that everything is fine not to delay the rest. What do you think? The alternative would be to wait with setting up everything until the first payment comes.
-
When the payment check fails, then I close the stream and it prevents the streaming from happening. All good. But I wonder about cleaning up other parts.
- Tear down runner => I guess @victorges implements some timeout, so it should happen automatically
- Any other objects to clean up on either Orchestrator or Gateway?
Thanks for the review @leszko
Possibly. We can send the first payment in along with the initial request like the other pipelines do it (eg via
The runner will receive an EOS (end of stream) flag and stop subscribing (PR). I think the rest of the pipeline is also terminated after that, but I haven't tested recently.
Right now, the bookkeeping on the orchestrator is minimal (just publish and subscribe streams IIRC) but I am sure we will have more to clean up later. The gateway also cleans itself up after a moment or so, but I do have some improvements to make on the publisher side so tear-down is a little more controllable, rather than the current backpressure-led error chain. |
Channels can still be created locally on the server.
This is needed in case a payment fails then we need to close the channel but we don't want the publisher to re-open them.
We introduce a flag instead of just hard-coding the behavior because we depend on auto-creating in some other tools including the server on the demo box.