jrmccluskey commented on a change in pull request #16470:
URL: https://github.com/apache/beam/pull/16470#discussion_r781387880
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/process.go
##########
@@ -30,20 +31,39 @@ type ExpansionServiceRunner struct {
serviceCommand *exec.Cmd
}
+func findOpenPort() (int, error) {
+ listener, err := net.Listen("tcp", ":0")
+ if err != nil {
+ return 0, err
+ }
+ defer listener.Close()
+ return listener.Addr().(*net.TCPAddr).Port, nil
+}
+
// NewExpansionServiceRunner builds an ExpansionServiceRunner struct for a
given gradle target and
// Beam version and returns a pointer to it.
-func NewExpansionServiceRunner(jarPath, servicePort string)
*ExpansionServiceRunner {
+func NewExpansionServiceRunner(jarPath, servicePort string)
(*ExpansionServiceRunner, error) {
if servicePort == "" {
- servicePort = "8097"
+ port, err := findOpenPort()
+ if err != nil {
+ return nil, fmt.Errorf("failed to find open port for
service, got %v", err)
+ }
+ servicePort = fmt.Sprintf("%d", port)
}
serviceCommand := exec.Command("java", "-jar", jarPath, servicePort)
- return &ExpansionServiceRunner{jarPath: jarPath, servicePort:
servicePort, serviceCommand: serviceCommand}
+ return &ExpansionServiceRunner{jarPath: jarPath, servicePort:
servicePort, serviceCommand: serviceCommand}, nil
}
func (e *ExpansionServiceRunner) String() string {
return fmt.Sprintf("JAR: %v, Port: %v, Process: %v", e.jarPath,
e.servicePort, e.serviceCommand.Process)
}
+// GetPort returns the formatted port the ExpansionServiceRunner is set to
start the expansion
+// service on.
+func (e *ExpansionServiceRunner) GetPort() string {
Review comment:
Interesting point regarding getters in Go. Changed.
Although wouldn't an exported field be directly accessible anyway? My
intuition here is that unexported fields have a lot of use as private fields in
a getter/setter structure and that an explicit Get/Set username isn't a bad
thing. This is more a comment on general Go style, not this particular instance
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/process.go
##########
@@ -30,20 +31,39 @@ type ExpansionServiceRunner struct {
serviceCommand *exec.Cmd
}
+func findOpenPort() (int, error) {
+ listener, err := net.Listen("tcp", ":0")
+ if err != nil {
+ return 0, err
+ }
+ defer listener.Close()
+ return listener.Addr().(*net.TCPAddr).Port, nil
+}
+
// NewExpansionServiceRunner builds an ExpansionServiceRunner struct for a
given gradle target and
// Beam version and returns a pointer to it.
Review comment:
Added, totally fair addition
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/process_test.go
##########
@@ -22,15 +22,28 @@ import (
"testing"
)
+func TestFindOpenPort(t *testing.T) {
+ port, err := findOpenPort()
+ if err != nil {
+ t.Fatalf("failed to find open port, got %v", port)
Review comment:
Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
