[GitHub] [beam] damccorm commented on a change in pull request #17110: [BEAM-13695] Add option to add-open flags for Java container

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

damccorm commented on a change in pull request #17110:
URL: https://github.com/apache/beam/pull/17110#discussion_r832611975



##########
File path: sdks/java/container/boot.go
##########
@@ -193,7 +195,35 @@ func main() {
 	} else {
 		args = append(args, jammAgentArgs)
 	}
+	// Apply meta options
+	const metaDir = "/opt/apache/beam/options"
+	metaOptions, err := LoadMetaOptions(metaDir)
+	javaOptions := BuildOptions(metaOptions)
+	// (1) Add custom jvm arguments: "-server -Xmx1324 -XXfoo .."
+	args = append(args, javaOptions.JavaArguments...)

Review comment:
       Is there any risk of duplicate args here? What would happen in that case/should we do some validation/warn or overwrite?

##########
File path: sdks/java/container/boot.go
##########
@@ -211,3 +241,120 @@ func heapSizeLimit(info *fnpb.ProvisionInfo) uint64 {
 	}
 	return 1 << 30
 }
+
+// Options represents java VM invocation options in a simple,
+// semi-structured way.
+type Options struct {
+	JavaArguments []string          `json:"java_arguments,omitempty"`
+	Properties    map[string]string `json:"properties,omitempty"`
+	Classpath     []string          `json:"classpath,omitempty"`
+}
+
+// MetaOption represents a jvm environment transformation or setup
+// that the launcher employs. The aim is to keep the service-side and
+// user-side required configuration simple and minimal, yet allow
+// numerous execution tweaks. Most tweaks are enabled by default and
+// require no input. Some setups, such as Cloud Debugging, are opt-in.
+//
+// Meta-options are usually included with the image and use supporting
+// files, usually jars. A few are intrinsic because they are require
+// additional input or complex computations, such as Cloud Debugging
+// and Cloud Profiling. Meta-options can be enabled or disabled by
+// name. For the most part, the meta-option names are not guaranteed
+// to be backwards compatible or stable. They are rather knobs that
+// can be tuned if some well-intended transformation cause trouble for
+// a customer. For tweaks, the expectation is that the default is
+// almost always correct.
+//
+// Meta-options are simple additive manipulations applied in priority
+// order (applied low to high) to allow jvm customization by adding
+// files, notably enabling customization by later docker layers. The
+// override semantics is prepend for lists and simple overwrite
+// otherwise. A common use case is adding a jar to the beginning of
+// the classpath, such as the shuffle or windmill jni jar, or adding
+// an agent.
+type MetaOption struct {
+	Name        string  `json:"name,omitempty"`
+	Description string  `json:"description,omitempty"`
+	Enabled     bool    `json:"enabled,omitempty"`
+	Priority    int     `json:"priority,omitempty"`
+	Options     Options `json:"options"`
+}
+
+// byPriority sorts MetaOptions by priority, highest first.
+type byPriority []*MetaOption
+
+func (f byPriority) Len() int           { return len(f) }
+func (f byPriority) Swap(i, j int)      { f[i], f[j] = f[j], f[i] }
+func (f byPriority) Less(i, j int) bool { return f[i].Priority > f[j].Priority }
+
+// LoadMetaOptions scans the directory tree for meta-option metadata
+// files and loads them. Any regular file named "option-XX.json" is
+// strictly assumed to be a meta-option file. This strictness allows
+// us to fail hard if such a file cannot be parsed.
+//
+// Loading meta-options from disk allows extra files and their
+// configuration be kept together and defined externally.
+func LoadMetaOptions(dir string) ([]*MetaOption, error) {
+	var meta []*MetaOption
+
+	worker := func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if !info.Mode().IsRegular() {
+			return nil
+		}
+		if !strings.HasPrefix(info.Name(), "option-") {
+			return nil
+		}
+		if !strings.HasSuffix(info.Name(), ".json") {
+			return nil
+		}
+
+		content, err := ioutil.ReadFile(path)
+		if err != nil {
+			return err
+		}
+
+		var option MetaOption
+		if err := json.Unmarshal(content, &option); err != nil {
+			return fmt.Errorf("failed to parse %s: %v", path, err)
+		}
+
+		log.Printf("Loaded meta-option '%s'", option.Name)
+
+		meta = append(meta, &option)
+		return nil
+	}
+
+	if err := filepath.Walk(dir, worker); err != nil {
+		return nil, err
+	}
+	return meta, nil
+}
+
+func BuildOptions(metaOptions []*MetaOption) *Options {

Review comment:
       Could you please add tests for this and LoadMetaOptions? Both are pretty well contained/good candidates for units.




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