[GitHub] [beam] lostluck commented on a diff in pull request #17216: Simplify specifying additional dependencies in Go SDK in XLang IOs

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

lostluck commented on code in PR #17216:
URL: https://github.com/apache/beam/pull/17216#discussion_r843327776


##########
sdks/go/pkg/beam/xlang.go:
##########
@@ -172,8 +172,7 @@ func TryCrossLanguage(
 	payload []byte,
 	expansionAddr string,
 	namedInputs map[string]PCollection,
-	namedOutputTypes map[string]FullType,
-) (map[string]PCollection, error) {
+	namedOutputTypes map[string]FullType) (map[string]PCollection, error) {

Review Comment:
   Don't worry about this formatting nit. it's because there's no trailing comma in the last parameter. The signatures are the same. But since we don't need to change the file anyway, adding the comma back, and the newline will keep gofmt from reverting it, allowing this file to no longer have a diff.



##########
sdks/go/pkg/beam/core/runtime/xlangx/registry.go:
##########
@@ -276,6 +278,16 @@ func UseAutomatedJavaExpansionService(gradleTarget string) string {
 	return autoJavaNamespace + Separator + gradleTarget
 }
 
+// AddClasspaths takes an expansion address string and creates a tagged string
+// suffixed with classpath separator and classpaths provided. This indicates that
+// the classpaths should be added to expansion service JAR before auto start-up.
+//
+// Intended for use by cross language wrappers to add classpaths to the manifest of
+// an expansion service in case of automated start-up of JAVA expansion service.
+func AddClasspaths(expansionAddr string, classpaths []string) string {

Review Comment:
   @jrmccluskey What do you think of this approach?
   
   I don't like that it requires the user to call both UseAutomatedJavaExpansionService and this, and could lead to weird malformations of the expansion address if they forget to call the former.
   
   If adding the classpaths is something we can do independently of the autostartup, then this is correct, but I don't think that's true. I'd prefer a single function that probably calls to UseAutomatedJavaExpansionService that then does the extra construction if needed.  (Possibly with it's own options.... options on options...)  Or is that too much?



-- 
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