Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MLCOMPUTE-1389 | remove spark.app.id and generate static spark.app.name #3923

Merged
merged 6 commits into from
Jul 23, 2024

Conversation

CaptainSame
Copy link
Contributor

@CaptainSame CaptainSame commented Jul 18, 2024

  • spark.app.id is generated fresh and added by service_configuration_lib with a random postfix because of which Tron keeps running update config flow every few minutes. This config should be removed and generated by yelp spark-submit wrapper or by Spark itself.
  • spark.app.name is also generated fresh with a random timestamp and postfix which leads to the same problem. This config should be made static and potentially replaced by yelp spark-submit wrapper later

spark_conf.pop("spark.app.id", None)
# use a static spark.app.name to prevent frequent config updates in Tron.
# md5 and base64 will always generate the same encoding for a string.
# This spark.app.name might be overridden by yelp spark-submit wrapper.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from a sourcegraph search for repo:^sysgit/yelpsoa-configs$ spark.app.name it seems like some folks set short static app names, so we may want to follow this up with a follow-up PR that allows static/user-set app names to continue being set if that's something we want to support

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also auto set by spark-etl jobs - sourcegraph: repo:spark_etl$ spark.app.name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this

@CaptainSame CaptainSame merged commit 5372078 into master Jul 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants