[SPARK-3767] [CORE] Support wildcard in Spark properties

[devaraj-kavali]

What changes were proposed in this pull request?

Added provision to specify the 'spark.executor.extraJavaOptions' value in terms of the Executor Id(i.e. @execid@). @execid@ will be replaced with the Executor Id while starting the executor.

How was this patch tested?

I have verified this by checking the executor process command and gc logs. I verified the same in different deployement modes(Standalone, YARN, Mesos).

[AmplabJenkins]

Can one of the admins verify this patch?

[rxin]

I think this is a good idea in general, but I'm less sure about @. Is @ used in windows? Any other places?

[devaraj-kavali]

Thanks @rxin for checking this, I don't think @ is used any where. Here again we are replacing only for 'spark.executor.extraJavaOptions' value when @execid@ occurs, any other @ symbols we leave as it is, so I don't think any problem occurs due to this.

[BryanCutler]

would it be a little cleaner to do the following

val extraJavaOpts = conf.getOption("spark.executor.extraJavaOptions").map {
  Utils.substituteExecIdWildCard(_, taskId)
}.getOrElse("")

[devaraj-kavali]

Thanks @BryanCutler for the suggestion, I have addressed it in the latest.

[devaraj-kavali]

@rxin, Please have a look into this and let me know any thing needs to be done here. About @, M/R also uses @ for the taskid wild card in java opts and there is no problem in windows and as well as in other places with @.

[vanzin]

Sorry this has been forgotten for so long. I think instead of adding a new way of referencing variables, it might be better to use the new code I'm adding in #14468; that way it would be easier to reference executor id (by creating a new ConfigProvider with that info) and even other config information if desired.

At the very least conflicts need to be resolved.

[devaraj-kavali]

@vanzin Thanks for looking into this, I have resolved the conflicts.

[vanzin]

Hmmm... wouldn't this be a one line change (for standalone and mesos) if instead you modified CommandUtils.buildLocalCommand to apply variable substitution to command.javaOpts instead of just to command.arguments?

That would replace {{EXECUTOR_ID}} in any of the arguments, since that variable is already in the list of things to replace (see ExecutorRunner.substituteVariables.)

You'd still need some extra code for YARN since it doesn't use that path. But it would be a smaller change that re-uses existing functionality.

EDIT: mesos seems to use a different path too. I just want to avoid creating yet another syntax for variable substitution. We should either use the standalone-style internal variable substitution ({{VARIABLE}}), or the one supported by SparkConf (${variable}), and re-use the code that processes those instead of adding more code to do the same thing.

[vanzin]

Also the PR title is misleading, since this change is adding a single "wildcard" to a single configuration. The title makes it sound like it's way more generic than it is.

[vanzin]

Hi @devaraj-kavali, are you still interested in updating this PR? I really think it should use the features I added in SPARK-16671 (especially the ConfigReader code), to avoid yet another way of expanding variables.

[devaraj-kavali]

@vanzin, SPARK-3767 was resolved as 'Won't Fix' by @srowen. I was in assumption that SPARK-16671 covers this as well.

[vanzin]

If you're willing to work on it you can re-open the bug. Otherwise you should close this PR.

[devaraj-kavali]

I will update this PR with the ConfigReader and reopen the jira.