-
Notifications
You must be signed in to change notification settings - Fork 178
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
Split devtool Orchestrator scripts with/without external transcoder. Prevent run scripts from using same CLI port. #2299
Conversation
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.
@thomshutt Thanks for the PR.
I wonder if this change actually solves the issue of executing run_transcoder.sh
. Here are my thoughts:
- The O/T topology (when the orchestrator is run together with transcoder in one process) works anyway, just execute:
./run_broadcaster*.sh
./run_orchestrator*.sh
There is no port conflict, everything works fine.
- The split OT topology (which your PR is supposed to fix) didn't work for me after your change. Maybe I did something wrong...
I think to make the split OT topology it'd require some more changes. Even if we fix the CLI port, the following execution does not work.
./run_broadcaster*.sh
./run_orchestrator*.sh
./run_transcoder*.sh
The reason for this is that the Orchestrator is anyway run with -transcoder
parameter, so it does not accept other transcoders.
My suggestion is to:
- either fix scripts for the split O/T topology
- either just accept the split O/T does not work and maybe create a GH issue to fix it
@leszko Thanks for reviewing! That makes sense, I'll remove the |
TBH I'd keep the possibility for the O/T topology and maybe add an option to generate the split O/T (separate transcoder)? I think O/T is good, because it's the simplest way to test something on-chain. So I use it very often. |
👍 Fair enough, I'll go with that then |
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 for the change. I like it. And also great that you added run*.sh
to .gitignore
. Added some comments.
@@ -330,13 +330,10 @@ func ethSetup(ethAcctAddr, keystoreDir string, isBroadcaster bool) { | |||
func createTranscoderRunScript(ethAcctAddr, dataDir, serviceHost string) { |
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.
Not related to your PR, but we could clean up a little bit the generated scripts, now some of the commands are in new lines, some of them are in the same, I'd make it consistent. Wdyt?
Generated run_broadcaster.sh
(some parameters are in new lines, some are in the same):
#!/bin/bash
./livepeer -v 99 -ethController 0x77a0865438f2efd65667362d4a8937537ca7a5ef -datadir ./.lpdev2/broadcaster_0x06AAaf48472D6a64f33C7e281b5dB0fE419878F6 \
-ethAcctAddr 0x06AAaf48472D6a64f33C7e281b5dB0fE419878F6 \
-ethUrl http://localhost:8545/ \
-ethPassword "" \
-network=devenv \
-blockPollingInterval 1 \
-monitor=false -currentManifest=true -cliAddr 127.0.0.1:7935 -httpAddr 127.0.0.1:8935 -broadcaster=true -rtmpAddr 127.0.0.1:1935
Generated run_trascoder.sh
(everything is in one line):
#!/bin/bash
./livepeer -v 99 -datadir ./.lpdev2/transcoder_0x065195849B39Bf5D99b4EbD28F61C75C6EB8771a -orchSecret secre -orchAddr 127.0.0.1:8936 -cliAddr 127.0.0.1:7937 -transcoder
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.
@thomshutt Could you check this as well?
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.
LGTM
Before merging please:
-
Look also into this comment.
-
Squash, rename, and rebase your commits according to what's described in the Contribution Guide: Commits
@@ -330,13 +330,10 @@ func ethSetup(ethAcctAddr, keystoreDir string, isBroadcaster bool) { | |||
func createTranscoderRunScript(ethAcctAddr, dataDir, serviceHost string) { |
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.
@thomshutt Could you check this as well?
…der and standardize how scripts are formatted
a86cf1d
to
cbaf350
Compare
Thanks for all your help here @leszko 🎉 |
What does this pull request do? Explain your changes. (required)
It's currently not possible to run the
run_broadcaster
andrun_transcoder
scripts generated by devtool simultaneously, becauserun_broadcaster
specifies a CLI Port of7935
and run_transcoder doesn't specify one (and so go-livepeer uses7935
by default).Specific updates (required)
How did you test each of these updates (required)
Does this pull request close any open issues?
No
Checklist:
make
runs successfully./test.sh
pass