amogh-jahagirdar commented on code in PR #8423:
URL: https://github.com/apache/iceberg/pull/8423#discussion_r1321737286
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/JobGroupUtils.java:
##########
@@ -39,11 +40,16 @@ public static JobGroupInfo getJobGroupInfo(SparkContext
sparkContext) {
return new JobGroupInfo(groupId, description,
Boolean.parseBoolean(interruptOnCancel));
}
- public static void setJobGroupInfo(SparkContext sparkContext, JobGroupInfo
info) {
- sparkContext.setLocalProperty(JOB_GROUP_ID, info.groupId());
+ private static void setJobGroupInfo(
Review Comment:
Looks like we're directly removing a public method, which we should avoid. I
think adding a new method accepting `overwrite` instead and marking the old
method as deprecated may be a better option here. also cc: @aokolnychyi
@RussellSpitzer @szehon-ho for their inputs
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/JobGroupUtils.java:
##########
@@ -55,10 +61,10 @@ public static <T> T withJobGroupInfo(
SparkContext sparkContext, JobGroupInfo info, Supplier<T> supplier) {
JobGroupInfo previousInfo = getJobGroupInfo(sparkContext);
try {
- setJobGroupInfo(sparkContext, info);
+ setJobGroupInfo(sparkContext, info, false);
Review Comment:
Technically this is a behavior change (which I get, is the goal of the PR)
the issue is not sure how many people actually desire the current behavior. If
it's not a common case to want to override the JobGroupInfo via this method,
then perhaps this behavior can simply change. Otherwise, we may need to support
preventing overriding JobGroupInfo through a different mechanism (some kind of
run time property which defaults to the original override mechanism).
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/JobGroupUtils.java:
##########
@@ -39,11 +40,16 @@ public static JobGroupInfo getJobGroupInfo(SparkContext
sparkContext) {
return new JobGroupInfo(groupId, description,
Boolean.parseBoolean(interruptOnCancel));
}
- public static void setJobGroupInfo(SparkContext sparkContext, JobGroupInfo
info) {
- sparkContext.setLocalProperty(JOB_GROUP_ID, info.groupId());
+ private static void setJobGroupInfo(
+ SparkContext sparkContext, JobGroupInfo info, boolean overwrite) {
+ if (overwrite ||
StringUtils.isBlank(sparkContext.getLocalProperty(JOB_GROUP_ID))) {
+ sparkContext.setLocalProperty(JOB_GROUP_ID, info.groupId());
+ }
+ if (overwrite ||
StringUtils.isBlank(sparkContext.getLocalProperty(JOB_INTERRUPT_ON_CANCEL))) {
+ sparkContext.setLocalProperty(
+ JOB_INTERRUPT_ON_CANCEL, String.valueOf(info.interruptOnCancel()));
+ }
Review Comment:
Style nit, new lines after each if block
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
