hudi-agent commented on code in PR #19970:
URL: https://github.com/apache/hudi/pull/19970#discussion_r4043944064
##########
hudi-cli/src/main/java/org/apache/hudi/cli/commands/SparkMain.java:
##########
@@ -97,17 +101,45 @@ public class SparkMain {
* Commands.
*/
enum SparkCommand {
- BOOTSTRAP(21), ROLLBACK(6), DEDUPLICATE(8), ROLLBACK_TO_SAVEPOINT(6),
SAVEPOINT(7),
- IMPORT(13), UPSERT(13), COMPACT_SCHEDULE(6), COMPACT_RUN(10),
COMPACT_SCHEDULE_AND_EXECUTE(9),
- COMPACT_UNSCHEDULE_PLAN(9), COMPACT_UNSCHEDULE_FILE(10),
COMPACT_VALIDATE(7), COMPACT_REPAIR(8),
- CLUSTERING_SCHEDULE(6), CLUSTERING_RUN(9),
CLUSTERING_SCHEDULE_AND_EXECUTE(8), CLEAN(5),
- DELETE_MARKER(5), DELETE_SAVEPOINT(5), UPGRADE(5), DOWNGRADE(5),
- REPAIR_DEPRECATED_PARTITION(4), RENAME_PARTITION(6), ARCHIVE(8);
+ BOOTSTRAP(21, "tableName", "tableType", "targetPath", "srcPath",
"rowKeyField", "partitionPathField",
Review Comment:
🤖 nit: `paramNames` here has to stay in the exact same order as the
positional args each command's `main()` branch expects to unpack. Now that
order is duplicated in two places (this enum and the positional-parsing code
below); might be worth a comment on `paramNames` calling out that ordering is
load-bearing, so a future edit to one side doesn't silently break the other.
<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 +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() : "");
+ }
Review Comment:
🤖 nit: `namedArgsToPositional` is doing quite a bit (JCommander parsing,
positional mapping, extra-config passthrough) in one method — might be worth
splitting the "map named params onto positional slots" logic from the "collect
extra -D configs" logic into separate helper methods for readability.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 Confirmed. The only other jar on the CLI classpath per
`hudi-cli-with-bundle.sh` is `hudi-spark-bundle`, and it relocates jcommander
to `org.apache.hudi.com.beust.jcommander`, so it can't satisfy `SparkMain`'s
unrelocated `com.beust.jcommander` imports either. Since all 12 call sites now
go through `addNamedAppArgs`, this would be a `NoClassDefFoundError` on every
Spark-launching CLI command from the bundle, not just the new named path.
##########
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:
🤖 Confirmed — `validateAndAddProperties` appends bare `key=value` tokens
after the `--command/--master/--memory/-D...` args, and JCommander with no main
parameter rejects them. One alternative to adding a main `@Parameter`: have
those call sites route the configs through `addNamedAppArgs` as extra
name/value pairs, since `namedArgsToPositional` already forwards any `-D` key
not in `paramNames` to the trailing-config tail that `makeConfigs` consumes.
That keeps a single parsing path rather than two ways of expressing trailing
configs.
##########
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:
🤖 Agree, and there's a second failure mode here: because any `-D` key not in
`paramNames` is silently forwarded as a trailing config, a typo like
`-DbasePth=/x` leaves the `basePath` slot empty *and* passes `assertGtEq` for
commands like CLEAN/COMPACT_RUN — it just shows up as a bogus hoodie config.
Validating supplied names in `addNamedAppArgs` would catch this on the CLI side
before the Spark app is even launched.
--
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]
