hudi-agent commented on code in PR #19970:
URL: https://github.com/apache/hudi/pull/19970#discussion_r4140103681
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/SparkMain.java:
##########
@@ -139,8 +173,128 @@ public static void addAppArgs(SparkLauncher
sparkLauncher, SparkMain.SparkComman
sparkLauncher.addAppArgs(args);
}
- public static void main(String[] args) {
- ValidationUtils.checkArgument(args.length >= 4);
+ /**
+ * Adds the command invocation as fully named options: {@code --command X
--master Y --memory Z}
+ * plus {@code -Dkey=value} pairs for every command argument. This makes the
launcher command
+ * self-describing and removes the risk of mis-aligned positional arguments
at call sites.
+ *
+ * @param namedArgs flat name/value pairs for the command's declared
arguments. Every name
+ * except propsFilePath must be supplied; explicit null
values become empty
+ * strings, preserving the positional slots for optional
values.
+ */
+ public static void addNamedAppArgs(SparkLauncher sparkLauncher,
SparkMain.SparkCommand cmd, String master, String memory, String... namedArgs) {
+ ValidationUtils.checkArgument(namedArgs.length % 2 == 0, "namedArgs must
be flat name/value pairs");
+ Map<String, String> params = new LinkedHashMap<>();
+ for (int i = 0; i < namedArgs.length; i += 2) {
+ ValidationUtils.checkArgument(!params.containsKey(namedArgs[i]),
"Duplicate command argument: " + namedArgs[i]);
+ params.put(namedArgs[i], namedArgs[i + 1] == null ? "" : namedArgs[i +
1]);
+ }
+ validateNamedParams(cmd, params);
+ sparkLauncher.addAppArgs("--command", cmd.toString());
+ sparkLauncher.addAppArgs("--master", master);
+ sparkLauncher.addAppArgs("--memory", memory);
+ params.forEach((name, value) -> sparkLauncher.addAppArgs("-D" + name + "="
+ value));
+ }
+
+ private static void validateNamedParams(SparkCommand command, Map<String,
String> params) {
+ ValidationUtils.checkArgument(command.paramNames.length > 0, "Named
arguments are not supported for " + command);
+ List<String> paramNames = Arrays.asList(command.paramNames);
+ for (String name : params.keySet()) {
+ ValidationUtils.checkArgument(paramNames.contains(name), "Unknown
argument for " + command + ": " + name
+ + ". Pass Spark/Hoodie configs as trailing key=value arguments
without -D.");
+ }
+ for (String name : paramNames) {
+ ValidationUtils.checkArgument(params.containsKey(name) ||
"propsFilePath".equals(name),
+ "Missing argument for " + command + ": " + name);
+ }
+ }
+
+ private static boolean isNamedInvocation(String[] argv) {
+ return argv.length > 0 && argv[0].startsWith("-");
+ }
+
+ /**
+ * Translates {@code --command X --master Y --memory Z -Dkey=value...} into
the positional
+ * form {@code [X, Y, Z, arg1, arg2, ...]} expected by the legacy invocation
path. The named
+ * -D arguments must match the command's positional argument names.
Spark/Hoodie configs
+ * are supplied as trailing {@code key=value} arguments.
+ */
+ @VisibleForTesting
+ static String[] namedArgsToPositional(String[] argv) {
+ NamedArgs namedArgs = parseNamedArgs(argv);
+ SparkCommand command = SparkCommand.valueOf(namedArgs.command);
+ validateNamedParams(command, namedArgs.params);
+ List<String> positional = new ArrayList<>();
+ positional.add(namedArgs.command);
+ positional.add(namedArgs.master);
+ positional.add(namedArgs.memory);
+ for (String name : command.paramNames) {
+ positional.add(namedArgs.params.getOrDefault(name, ""));
+ }
+ positional.addAll(namedArgs.trailingConfigs);
+ return positional.toArray(new String[0]);
+ }
+
+ private static NamedArgs parseNamedArgs(String[] argv) {
+ NamedArgs namedArgs = new NamedArgs();
+ List<String> options = new ArrayList<>();
+ for (int i = 0; i < argv.length; i++) {
+ String arg = argv[i];
+ if ("--".equals(arg)) {
+ namedArgs.trailingConfigs.addAll(Arrays.asList(argv).subList(i + 1,
argv.length));
+ break;
+ } else if (arg.startsWith("-")) {
+ options.add(arg);
+ if (Arrays.asList("--command", "-command", "--master", "-master",
"--memory", "-memory", "-D").contains(arg)) {
Review Comment:
🤖 nit: this inline list repeats the option names from the `@Parameter`
annotations on `NamedArgs`. Could you move it into a `private static final
Set<String>` constant that sits next to `NamedArgs`, so a new or renamed option
only has to be updated in one place?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/SparkMain.java:
##########
@@ -139,8 +173,128 @@ public static void addAppArgs(SparkLauncher
sparkLauncher, SparkMain.SparkComman
sparkLauncher.addAppArgs(args);
}
- public static void main(String[] args) {
- ValidationUtils.checkArgument(args.length >= 4);
+ /**
+ * Adds the command invocation as fully named options: {@code --command X
--master Y --memory Z}
+ * plus {@code -Dkey=value} pairs for every command argument. This makes the
launcher command
+ * self-describing and removes the risk of mis-aligned positional arguments
at call sites.
+ *
+ * @param namedArgs flat name/value pairs for the command's declared
arguments. Every name
+ * except propsFilePath must be supplied; explicit null
values become empty
+ * strings, preserving the positional slots for optional
values.
+ */
+ public static void addNamedAppArgs(SparkLauncher sparkLauncher,
SparkMain.SparkCommand cmd, String master, String memory, String... namedArgs) {
+ ValidationUtils.checkArgument(namedArgs.length % 2 == 0, "namedArgs must
be flat name/value pairs");
+ Map<String, String> params = new LinkedHashMap<>();
+ for (int i = 0; i < namedArgs.length; i += 2) {
+ ValidationUtils.checkArgument(!params.containsKey(namedArgs[i]),
"Duplicate command argument: " + namedArgs[i]);
+ params.put(namedArgs[i], namedArgs[i + 1] == null ? "" : namedArgs[i +
1]);
+ }
+ validateNamedParams(cmd, params);
+ sparkLauncher.addAppArgs("--command", cmd.toString());
+ sparkLauncher.addAppArgs("--master", master);
+ sparkLauncher.addAppArgs("--memory", memory);
+ params.forEach((name, value) -> sparkLauncher.addAppArgs("-D" + name + "="
+ value));
+ }
+
+ private static void validateNamedParams(SparkCommand command, Map<String,
String> params) {
+ ValidationUtils.checkArgument(command.paramNames.length > 0, "Named
arguments are not supported for " + command);
+ List<String> paramNames = Arrays.asList(command.paramNames);
+ for (String name : params.keySet()) {
+ ValidationUtils.checkArgument(paramNames.contains(name), "Unknown
argument for " + command + ": " + name
+ + ". Pass Spark/Hoodie configs as trailing key=value arguments
without -D.");
+ }
+ for (String name : paramNames) {
+ ValidationUtils.checkArgument(params.containsKey(name) ||
"propsFilePath".equals(name),
Review Comment:
🤖 nit: `"propsFilePath"` is a magic string that quietly makes one argument
optional. Could you pull it into a named constant like
`OPTIONAL_PROPS_FILE_PATH_ARG`, or add a short comment explaining why it's the
only optional argument?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
