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

[SPARK-37785][SQL][CORE] Add Utils.isInRunningSparkTask #35065

Closed

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Dec 30, 2021

What changes were proposed in this pull request?

This PR proposes to add Utils.isInRunningSparkTask to see if the codes are running on tasks e.g., on executors.

Why are the changes needed?

There is currently no single call to see if we're in a running Spark task (e.g., in executors). TaskContext.get == null is being used for that way. We should better explicitly factor out to Utils.

Does this PR introduce any user-facing change?

No, dev-only.

How was this patch tested?

Existing unittests should cover this case.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

@JoshRosen
Copy link
Contributor

Just brainstorming aloud here:

Is the name isAtExecutorSide potentially confusing in the context of local mode (since in that case we don't have separate executor processes)?

I suppose it makes sense if we think of "at the executor side" as meaning

"in the context of a running Spark task"

instead of meaning

"not in the driver JVM".

If we wanted to be more precise, maybe we could call this Utils.inRunningSparkTask or something like that.

I don't feel super strongly about this. I mostly just want to flag the potential for ambiguity in case a new developer uses this method expecting the "not in the driver JVM" semantic.

@HyukjinKwon
Copy link
Member Author

Yeah, I think it's better to be specific on what it means. Let me change like that.

@HyukjinKwon HyukjinKwon changed the title [SPARK-37785][SQL][CORE] Add Utils.isAtExecutorSide [SPARK-37785][SQL][CORE] Add Utils.isInRunningSparkTask Dec 30, 2021
@mridulm
Copy link
Contributor

mridulm commented Dec 30, 2021

With the rename, it looks good to me.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 30, 2021

Thanks all!
Merged to master!

@HyukjinKwon HyukjinKwon deleted the mindor-util-at-executor branch January 4, 2022 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants