Skip to content

Conversation

@timma-yelp
Copy link
Contributor

Added code required to pass configs required for some of the services

@timma-yelp timma-yelp requested a review from a team as a code owner October 30, 2025 13:00
Comment on lines 1438 to 1457
# Prepare configuration required for some of the services
scs_args = {
"cluster_manager": args.cluster_manager,
"spark_app_base_name": app_base_name,
"docker_image": docker_image_digest,
"user_spark_opts": user_spark_opts,
"paasta_cluster": paasta_cluster,
"paasta_pool": args.pool,
"paasta_service": args.service,
"paasta_instance": paasta_instance,
"extra_volumes": cast(List[Mapping[str, str]], volumes),
"force_spark_resource_configs": args.force_spark_resource_configs,
"k8s_server_address": k8s_server_address,
"jira_ticket": args.jira_ticket,
"service_account_name": service_account_name,
"ui_port": int(spark_conf["spark.ui.port"]),
"user": os.environ.get("USER"),
"aws_account_id": get_aws_account_id(aws_creds),
}
args.scs = json.dumps(scs_args, indent=4)
Copy link
Member

Choose a reason for hiding this comment

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

since we stuff this into spark_conf later - should we try to do this in spark_conf_builder.get_spark_conf() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, will take a look and see which is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this code to spark_conf_builder.get_spark_conf()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timma-yelp timma-yelp requested a review from nemacysts November 11, 2025 10:55
@nemacysts nemacysts changed the title Added code to pass configs required for some of the services Set SPARK_DRIVER_TYPE for spark-run Spark drivers Nov 11, 2025
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

looks like a test is failing since we need to update it to account for this new env var - but lgtm otherwise

@timma-yelp
Copy link
Contributor Author

looks like a test is failing since we need to update it to account for this new env var - but lgtm otherwise

Thanks, i am fixing the tests

@timma-yelp timma-yelp requested a review from nemacysts November 11, 2025 18:00
@nemacysts nemacysts merged commit 1eae346 into master Nov 11, 2025
8 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