[GitHub] [beam] lostluck commented on a change in pull request #16470: [BEAM-13534] Add automated port polling to expansion service runner if port isn't provided

[GitBox <gi...@apache.org>]

lostluck commented on a change in pull request #16470:
URL: https://github.com/apache/beam/pull/16470#discussion_r781358735



##########
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:
       Consider adding documentation for the behavior of "Passing an empty string as servicePort will request an open port to be assigned to the servce." 

##########
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:
       typo: port is being printed instead of err.

##########
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:
       s/GetPort/Endpoint
   
   What's being returned isn't just the port, it's the address + port pair. If an ipaddress were used, it would be called a "SocketAddress" instead. Since it's not an IP address, the best term is Endpoint.
   
   Finally, idiomatic Go drops the "Get" prefix unless the name would conflict with an exported field or method.
   
   

##########
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)
+	}
+	if port < 1 || port > 65535 {
+		t.Errorf("port out of TCP range [1, 66535], got %d", port)
+	}
+}
+
 func TestNewExpansionServiceRunner(t *testing.T) {
 	testPath := "path/to/jar"
 	testPort := "8097"
-	serviceRunner := NewExpansionServiceRunner(testPath, testPort)
+	serviceRunner, err := NewExpansionServiceRunner(testPath, testPort)
+	if err != nil {
+		t.Fatalf("NewExpansionServiceRunner failed, got %v", err)
+	}
 	if serviceRunner.jarPath != testPath {
 		t.Errorf("JAR path mismatch: wanted %v, got %v", testPath, serviceRunner.jarPath)
 	}
 	if serviceRunner.servicePort != testPort {
-		t.Errorf("service port mismatch: wanted %v, got %v", testPort, serviceRunner.servicePort)
+		t.Errorf("service port mismatch: wanted %v, got %v", testPort, testPort)

Review comment:
       This is an example of where using the got, want convention helps avoid copypasta mistakes like duplicating what we want, and what we got.
   
   Doing something like
   
   ```
   if got, want := serviceRunner.servicePort, testPort; got != want {
   		t.Errorf("service port mismatch: got %v, want %v", got, want)
   }
   ```
   
   Similar elsewhere. I noticed this last time after I merged it, so now's a good time to fix the older test cases.
   

##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/process_test.go
##########
@@ -44,19 +57,38 @@ func TestNewExpansionServiceRunner(t *testing.T) {
 	}
 }
 
+func TestGetPort(t *testing.T) {
+	testPort := "8097"
+	serviceRunner, err := NewExpansionServiceRunner("", testPort)
+	if err != nil {
+		t.Fatalf("NewExpansionServiceRunner failed, got %v", err)
+	}
+	observedPort := serviceRunner.GetPort()
+	expPort := "localhost:" + testPort
+	if observedPort != expPort {
+		t.Errorf("GetPort() returned mismatched value: wanted %v, got %v", observedPort, expPort)

Review comment:
       Nit: got & want for observedPort and expPort respectively are sufficient. There's nothing to confuse them with, and using the got + want convention will make it easier on other readers for consistency.




-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org