wombatu-kun commented on code in PR #19970:
URL: https://github.com/apache/hudi/pull/19970#discussion_r4023844461
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/SparkMain.java:
##########
@@ -139,8 +171,96 @@ 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; {@code null} values are skipped,
preserving the
+ * legacy semantics of absent trailing arguments (e.g.
propsFilePath)
+ */
+ 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");
+ sparkLauncher.addAppArgs("--command", cmd.toString());
+ sparkLauncher.addAppArgs("--master", master);
+ sparkLauncher.addAppArgs("--memory", memory);
+ for (int i = 0; i < namedArgs.length; i += 2) {
+ if (namedArgs[i + 1] != null) {
+ sparkLauncher.addAppArgs("-D" + namedArgs[i] + "=" + namedArgs[i + 1]);
+ }
+ }
+ }
+
+ 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 are mapped onto the command's positional argument names;
unknown -D keys are
+ * passed through as trailing configs, matching {@link
SparkCommand#makeConfigs(String[])}.
+ */
+ private static String[] namedArgsToPositional(String[] argv) {
+ NamedArgs namedArgs = new NamedArgs();
+ JCommander commander = JCommander.newBuilder()
+ .addObject(namedArgs)
+ .build();
+ commander.parse(argv);
+
+ SparkCommand command = SparkCommand.valueOf(namedArgs.command);
+ Map<String, String> params = namedArgs.params;
+ List<String> positionalArgs = new ArrayList<>(command.paramNames.length);
+ for (String paramName : command.paramNames) {
+ String value = params.get(paramName);
+ // absent named args map to empty strings to keep the positional slots
aligned; trailing
+ // empty slots are handled by getPropsFilePath/makeConfigs exactly like
the legacy form
+ positionalArgs.add(value != null ? value.trim() : "");
+ }
+ // extra -D keys are forwarded as trailing configs, mirroring makeConfigs
behavior
+ List<String> extraConfigs = new ArrayList<>();
+ for (Map.Entry<String, String> entry : params.entrySet()) {
+ if (!Arrays.asList(command.paramNames).contains(entry.getKey())) {
+ extraConfigs.add(entry.getKey() + "=" + entry.getValue());
+ }
+ }
+
+ List<String> positional = new ArrayList<>();
+ positional.add(namedArgs.command);
+ positional.add(namedArgs.master);
+ positional.add(namedArgs.memory);
+ positional.addAll(positionalArgs);
+ positional.addAll(extraConfigs);
+ return positional.toArray(new String[0]);
+ }
+
+ /**
+ * Named-argument form of a SparkMain invocation. Kept separate from the
legacy
+ * positional protocol so that existing call sites and scripts keep working
unchanged:
+ * if the first argument does not look like a named option the legacy path
is used.
+ */
+ private static class NamedArgs {
+ @Parameter(names = {"--command", "-command"}, description = "Name of the
Spark command to run (e.g. ROLLBACK)", required = true)
+ private String command;
+
+ @Parameter(names = {"--master", "-master"}, description = "Spark master
URL (was positional arg 2)", required = true)
+ private String master;
+
+ @Parameter(names = {"--memory", "-memory"}, description = "Spark driver
memory (was positional arg 3)", required = true)
+ private String memory;
+
+ @DynamicParameter(names = "-D", description = "Command arguments and spark
configs as -Dkey=value pairs")
+ private Map<String, String> params = new HashMap<>();
Review Comment:
NamedArgs declares no main parameter, so the bare `key=value` app args that
UtilHelpers.validateAndAddProperties appends right after addNamedAppArgs make
JCommander throw "no main parameter was defined in your arg class", breaking
`--hoodieConfigs` for bootstrap, cleans run, and every compaction and
clustering command. Add a main `@Parameter List<String>` to NamedArgs and
append it to the positional tail where makeConfigs already expects trailing
configs.
##########
hudi-cli/pom.xml:
##########
@@ -236,6 +236,12 @@
<version>${project.version}</version>
</dependency>
+ <!-- JCommander for SparkMain named-argument CLI (HUDI-96) -->
+ <dependency>
+ <groupId>com.beust</groupId>
+ <artifactId>jcommander</artifactId>
Review Comment:
The new dependency is not carried into packaging/hudi-cli-bundle/pom.xml,
whose shade includes omit com.beust:jcommander while every other bundle that
uses it lists and relocates it - and SparkUtil.initLauncher launches SparkMain
out of that bundle with no lib directory beside it. Add the include and the
matching com.beust.jcommander relocation so the named path also works outside
the dev layout.
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/SparkMain.java:
##########
@@ -139,8 +171,96 @@ 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; {@code null} values are skipped,
preserving the
+ * legacy semantics of absent trailing arguments (e.g.
propsFilePath)
+ */
+ 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");
Review Comment:
This only checks that the array has even length, and namedArgsToPositional
sizes the positional array from cmd.paramNames rather than from what was
passed, so dropping a whole name/value pair or misspelling a name leaves an
empty slot no assertEq can see - dropping "basePath" from the CLEAN call site
would run HoodieCleaner against an empty base path. Check each supplied name
against cmd.paramNames here, the way addAppArgs checks arity.
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/SparkMain.java:
##########
@@ -139,8 +171,96 @@ 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; {@code null} values are skipped,
preserving the
+ * legacy semantics of absent trailing arguments (e.g.
propsFilePath)
+ */
+ 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");
+ sparkLauncher.addAppArgs("--command", cmd.toString());
+ sparkLauncher.addAppArgs("--master", master);
+ sparkLauncher.addAppArgs("--memory", memory);
+ for (int i = 0; i < namedArgs.length; i += 2) {
+ if (namedArgs[i + 1] != null) {
+ sparkLauncher.addAppArgs("-D" + namedArgs[i] + "=" + namedArgs[i + 1]);
+ }
+ }
+ }
+
+ 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 are mapped onto the command's positional argument names;
unknown -D keys are
+ * passed through as trailing configs, matching {@link
SparkCommand#makeConfigs(String[])}.
+ */
+ private static String[] namedArgsToPositional(String[] argv) {
Review Comment:
namedArgsToPositional is private and nothing exercises main's argv handling,
so the only new logic here ships untested and the 3 + paramNames.length ==
minArgsCount relation holds for 23 enum constants purely by hand-counting. Make
it @VisibleForTesting static and add a test that walks SparkCommand and asserts
the translated length against minArgsCount.
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/SparkMain.java:
##########
@@ -139,8 +171,96 @@ 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; {@code null} values are skipped,
preserving the
+ * legacy semantics of absent trailing arguments (e.g.
propsFilePath)
+ */
+ 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");
+ sparkLauncher.addAppArgs("--command", cmd.toString());
+ sparkLauncher.addAppArgs("--master", master);
+ sparkLauncher.addAppArgs("--memory", memory);
+ for (int i = 0; i < namedArgs.length; i += 2) {
+ if (namedArgs[i + 1] != null) {
+ sparkLauncher.addAppArgs("-D" + namedArgs[i] + "=" + namedArgs[i + 1]);
+ }
+ }
+ }
+
+ 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 are mapped onto the command's positional argument names;
unknown -D keys are
+ * passed through as trailing configs, matching {@link
SparkCommand#makeConfigs(String[])}.
+ */
+ private static String[] namedArgsToPositional(String[] argv) {
+ NamedArgs namedArgs = new NamedArgs();
+ JCommander commander = JCommander.newBuilder()
+ .addObject(namedArgs)
+ .build();
+ commander.parse(argv);
+
+ SparkCommand command = SparkCommand.valueOf(namedArgs.command);
+ Map<String, String> params = namedArgs.params;
+ List<String> positionalArgs = new ArrayList<>(command.paramNames.length);
+ for (String paramName : command.paramNames) {
+ String value = params.get(paramName);
+ // absent named args map to empty strings to keep the positional slots
aligned; trailing
+ // empty slots are handled by getPropsFilePath/makeConfigs exactly like
the legacy form
+ positionalArgs.add(value != null ? value.trim() : "");
+ }
+ // extra -D keys are forwarded as trailing configs, mirroring makeConfigs
behavior
+ List<String> extraConfigs = new ArrayList<>();
+ for (Map.Entry<String, String> entry : params.entrySet()) {
+ if (!Arrays.asList(command.paramNames).contains(entry.getKey())) {
+ extraConfigs.add(entry.getKey() + "=" + entry.getValue());
+ }
+ }
+
+ List<String> positional = new ArrayList<>();
+ positional.add(namedArgs.command);
+ positional.add(namedArgs.master);
+ positional.add(namedArgs.memory);
+ positional.addAll(positionalArgs);
+ positional.addAll(extraConfigs);
+ return positional.toArray(new String[0]);
+ }
+
+ /**
+ * Named-argument form of a SparkMain invocation. Kept separate from the
legacy
+ * positional protocol so that existing call sites and scripts keep working
unchanged:
+ * if the first argument does not look like a named option the legacy path
is used.
+ */
+ private static class NamedArgs {
+ @Parameter(names = {"--command", "-command"}, description = "Name of the
Spark command to run (e.g. ROLLBACK)", required = true)
+ private String command;
+
+ @Parameter(names = {"--master", "-master"}, description = "Spark master
URL (was positional arg 2)", required = true)
+ private String master;
+
+ @Parameter(names = {"--memory", "-memory"}, description = "Spark driver
memory (was positional arg 3)", required = true)
Review Comment:
This says driver memory, but SparkUtil.initJavaSparkContext puts the value
into HoodieCliSparkConfig.CLI_EXECUTOR_MEMORY and every call site feeds it the
--sparkMemory shell option. Reword to executor memory.
--
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]
