Skip to content

[SPARK-23015][WINDOWS] Fix bug in Windows where starting multiple Spark instances within the same second causes a failure #43706

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.

Sign up for GitHub

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

Closed
Rafnel wants to merge 5 commits into from
Closed

[SPARK-23015][WINDOWS] Fix bug in Windows where starting multiple Spark instances within the same second causes a failure #43706

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94070ab
Update spark-class2.cmd to mitigate bug in Windows where starting mul…
Rafnel Nov 7, 2023
eed35fe
Remove space between RANDOM and TIME in Spark launch file
Rafnel Nov 8, 2023
316fc48
Remove %RANDOM% from temporary Spark launcher file to fix race condit…
Rafnel Mar 14, 2024
a5b952f
Remove unnecessary goto statement in Spark Windows launcher
Rafnel Mar 21, 2024
ba8b056
Update Windows Spark launcher to use check if Powershell is installed…
Rafnel Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions bin/spark-class2.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,9 @@ if not "x%JAVA_HOME%"=="x" (
rem The launcher library prints the command to be executed in a single line suitable for being
rem executed by the batch interpreter. So read all the output of the launcher into a variable.
:gen
set LAUNCHER_OUTPUT=%temp%\spark-class-launcher-output-%RANDOM%.txt
rem SPARK-28302: %RANDOM% would return the same number if we call it instantly after last call,
rem so we should make it sure to generate unique file to avoid process collision of writing into
rem the same file concurrently.
FOR /F %%a IN ('POWERSHELL -COMMAND "$([guid]::NewGuid().ToString())"') DO (SET NEWGUID=%%a)
Copy link
Contributor

@panbingkun panbingkun Mar 16, 2024

Choose a reason for hiding this comment

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

If we use the GUID mechanism, do we still need this logic?

:gen
...
if exist %LAUNCHER_OUTPUT% goto :gen

Copy link
Contributor

@panbingkun panbingkun Mar 16, 2024

Choose a reason for hiding this comment

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

In addition, the current logic strongly relies on Powershell. I am worried about that the lack of Powershell on older versions of Windows will cause launch Spark failures.
Can we eliminate this strong dependence?

Copy link
Contributor Author

@Rafnel Rafnel Mar 21, 2024

Choose a reason for hiding this comment

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

If we use the GUID mechanism, do we still need this logic?

:gen
...
if exist %LAUNCHER_OUTPUT% goto :gen

You are correct, we shouldn't need this anymore. I can remove it.

Copy link
Contributor Author

@Rafnel Rafnel Mar 21, 2024

Choose a reason for hiding this comment

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

In addition, the current logic strongly relies on Powershell. I am worried about that the lack of Powershell on older versions of Windows will cause launch Spark failures. Can we eliminate this strong dependence?

As for this concern, I think this is the most lightweight way to generate a GUID in a .cmd script (without installing any dependencies). Powershell is available in Windows 7+ and Windows Server 2008R2+. I would be surprised if there were users running Spark on Windows XP/Vista or Server versions older than 2008. In its current state, you can't reliably instantiate multiple Spark instances on Windows, so I think it's a worthwhile tradeoff.

Copy link
Contributor Author

@Rafnel Rafnel Mar 21, 2024

Choose a reason for hiding this comment

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

Updated to remove below goto statement.

:gen
...
if exist %LAUNCHER_OUTPUT% goto :gen

Copy link
Contributor

@panbingkun panbingkun Mar 22, 2024

Choose a reason for hiding this comment

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

In addition, the current logic strongly relies on Powershell. I am worried about that the lack of Powershell on older versions of Windows will cause launch Spark failures. Can we eliminate this strong dependence?

As for this concern, I think this is the most lightweight way to generate a GUID in a .cmd script (without installing any dependencies). Powershell is available in Windows 7+ and Windows Server 2008R2+. I would be surprised if there were users running Spark on Windows XP/Vista or Server versions older than 2008. In its current state, you can't reliably instantiate multiple Spark instances on Windows, so I think it's a worthwhile tradeoff.

Perhaps we can first check whether Powershell has been installed. If not, use the original logic (until we find a simpler way to generate GUID), and if it is installed, use the logic based on Powershell to generate GUID?

Or prompt to install Powershell if it is not installed?

Copy link
Contributor Author

@Rafnel Rafnel Apr 2, 2024

Choose a reason for hiding this comment

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

Updated to add a simple check for powershell.exe and if not exists, fall back to using the Windows %RANDOM% which is the current behavior of this script.

set LAUNCHER_OUTPUT=%temp%\spark-class-launcher-output-%NEWGUID%.txt

if exist %LAUNCHER_OUTPUT% goto :gen
rem unset SHELL to indicate non-bash environment to launcher/Main
set SHELL=
Expand Down