Require specifying ports when using multiple nodes#145
Conversation
|
This change is |
2e3aa8c to
becfd4b
Compare
testserver/testserver.go
Outdated
| fmt.Sprintf("--listen-addr=localhost:%d", 26257+i), | ||
| fmt.Sprintf("--http-addr=localhost:%d", 8080+i), | ||
| fmt.Sprintf("--listen-addr=localhost:%d", serverArgs.httpPorts[i]), | ||
| fmt.Sprintf("--http-addr=localhost:%d", 0), |
There was a problem hiding this comment.
for this to be backwards compatible, i think we want it so still default to 26257/8080 if no port is specified
There was a problem hiding this comment.
I updated it for 26257 listenaddr but I don't think we actually use the HTTP addr in this case, I think it should be okay to just update it use 0 unless we find that we really need to set it.
There was a problem hiding this comment.
Added a diff parameter to allow people to specify HTTP port as well.
becfd4b to
0ed81c0
Compare
rafiss
left a comment
There was a problem hiding this comment.
this looks fine, just small nits
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @ZhouXing19)
testserver/testserver.go line 218 at r3 (raw file):
storeOnDisk bool // to save database in disk storeMemSize float64 // the proportion of available memory allocated to test server httpPort int
do we still need httpPort in addition to httpPorts?
testserver/testserver.go line 484 at r3 (raw file):
hostPort := serverArgs.listenAddrPorts[0] for i, port := range serverArgs.listenAddrPorts { portArgsStr[i] = fmt.Sprint(port)
nit: seems like it would be more readable if this was portArgsStr[i] = fmt.Sprintf("localhost:%d", port)
then below change to joinArg := fmt.Sprintf("--join=%s", strings.Join(portArgsStr, ","))
testserver/testserver_test.go line 474 at r3 (raw file):
defer func() { if err := exec.Command("rm", "-rf", "./temp_binaries").Run(); err != nil { t.Log(err)
is it better to keep failing loudly here?
d1fe11c to
1da40a5
Compare
RichardJCai
left a comment
There was a problem hiding this comment.
Reviewable status:
testserver/testserver.go line 218 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
do we still need
httpPortin addition tohttpPorts?
Was just being lazy keeping this around for backwards compat, updated it such that ExposeConsoleOpt just sets httpPorts[0] to port for backwards compat.
testserver/testserver.go line 484 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: seems like it would be more readable if this was
portArgsStr[i] = fmt.Sprintf("localhost:%d", port)then below change to
joinArg := fmt.Sprintf("--join=%s", strings.Join(portArgsStr, ","))
Yeah sounds good
RichardJCai
left a comment
There was a problem hiding this comment.
Reviewable status:
testserver/testserver_test.go line 474 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
is it better to keep failing loudly here?
Done
rafiss
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status:
Previously it was using hardcoded ports, to allow parallel runs we want to be able to specify nodes.
It is on the user to make sure when a node is specified, that port cannot be stolen away during a node restart.