[
https://issues.apache.org/jira/browse/BEAM-13399?focusedWorklogId=719615&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-719615
]
ASF GitHub Bot logged work on BEAM-13399:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 02/Feb/22 19:50
Start Date: 02/Feb/22 19:50
Worklog Time Spent: 10m
Work Description: lostluck commented on a change in pull request #16671:
URL: https://github.com/apache/beam/pull/16671#discussion_r797869705
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expand.go
##########
@@ -20,9 +20,11 @@ package xlangx
import (
"context"
+ // "github.com/apache/beam/sdks/v2/go/pkg/beam/core"
Review comment:
rm line
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expand.go
##########
@@ -140,3 +142,67 @@ func QueryExpansionService(ctx context.Context, p
*HandlerParams) (*jobpb.Expans
}
return res, nil
}
+
+func startAutomatedExpansionService(gradleTarget string) (func() error,
string, error) {
+ jarPath, err := expansionx.GetBeamJar(gradleTarget, "2.35.0")
Review comment:
I don't love the hard coding of 2.35.0 but it will probably do for now.
Could we tag this with a JIRA comment so we don't lose track of the hard code?
I've filed https://issues.apache.org/jira/browse/BEAM-13805 for it.
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expand.go
##########
@@ -140,3 +142,67 @@ func QueryExpansionService(ctx context.Context, p
*HandlerParams) (*jobpb.Expans
}
return res, nil
}
+
+func startAutomatedExpansionService(gradleTarget string) (func() error,
string, error) {
Review comment:
Consider calling this `startJavaExpansionService` instead.
Automated is sort of meaningless at this level, so it's not worth
mentioning, but calling out Java is important since the implementation is all
Jars all the time. It also avoids the issue of "Default Java".
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/registry.go
##########
@@ -257,9 +262,21 @@ func Require(expansionAddr string) string {
return hardOverrideNamespace + Separator + expansionAddr
}
+// UseAutomatedExpansionService takes a gradle target and creates a
+// tagged string to indicate that it should be used to start up an
+// automated expansion service for a gross-language expansion.
+//
+// Intended for use by cross language wrappers to permit spinning
+// up an expansion service for a user if no expansion service address
+// is provided.
+func UseAutomatedExpansionService(gradleTarget string) string {
Review comment:
I think I'm set on having a Java specific keyword for this. Lets call
this UseAutomatedJavaExpansionService, which makes the connection to Java
explicit.
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expand.go
##########
@@ -140,3 +142,67 @@ func QueryExpansionService(ctx context.Context, p
*HandlerParams) (*jobpb.Expans
}
return res, nil
}
+
+func startAutomatedExpansionService(gradleTarget string) (func() error,
string, error) {
+ jarPath, err := expansionx.GetBeamJar(gradleTarget, "2.35.0")
+ if err != nil {
+ return nil, "", err
+ }
+ serviceRunner, err := expansionx.NewExpansionServiceRunner(jarPath, "")
Review comment:
We may want to rename or alias this to `NewJavaExpansionServiceRunner`
since all it does is Java jars.
##########
File path: sdks/go/pkg/beam/transforms/sql/sql.go
##########
@@ -114,6 +116,9 @@ func Transform(s beam.Scope, query string, opts ...Option)
beam.PCollection {
if o.expansionAddr != "" {
expansionAddr = xlangx.Require(o.expansionAddr)
}
+ if expansionAddr == sqlx.DefaultExpansionAddr {
+ expansionAddr =
xlangx.UseAutomatedExpansionService(serviceGradleTarget)
Review comment:
Consider moving this line and the constant to the sqlx package instead,
and assigning it to DefaultExpansionAddr
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/registry.go
##########
@@ -244,6 +248,7 @@ const (
// Separator is the canonical separator between a namespace and
optional configuration.
Separator = ":"
hardOverrideNamespace = "hardoverride"
+ autoNamespace = "auto"
Review comment:
Since all we do is Java: "autojava"
##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/registry.go
##########
@@ -220,7 +220,11 @@ func (r *registry) getHandlerFunc(urn, expansionAddr
string) (HandlerFunc, strin
// By the time this is called, we want *some* kind of HandlerFunc at
all,
// So first we check for the hard override.
ns, config := parseAddr(expansionAddr)
- if ns == hardOverrideNamespace {
+ if ns == autoNamespace {
+ // Leave expansionAddr unmodified so the autoNamespace keyword
sticks.
+ // We strip it manually in the HandlerFunc.
+ return QueryAutomatedExpansionService, expansionAddr
Review comment:
My concern with this, is that it prevents URN overrides.
What if a user wants to override a specific URN to start an automated
expansion service. Why can't we register the auto keyword into the handlers map?
--
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-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 719615)
Time Spent: 19h 40m (was: 19.5h)
> [Go SDK] Add automated expansion service start up feature
> ---------------------------------------------------------
>
> Key: BEAM-13399
> URL: https://issues.apache.org/jira/browse/BEAM-13399
> Project: Beam
> Issue Type: Improvement
> Components: cross-language, sdk-go
> Reporter: Jack McCluskey
> Assignee: Jack McCluskey
> Priority: P2
> Time Spent: 19h 40m
> Remaining Estimate: 0h
>
> Add Go SDK support for automated expansion service start-up if one is not
> provided, following the example of the Python implementation.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
