-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-45486][CONNECT] Make add_artifact request idempotent #43314
Conversation
cc @grundprinzip, @HyukjinKwon -- (same change as previously discussed). |
Mind retriggering the failed tests https://github.com/cdkrot/apache_spark/runs/17602108213 ? |
Retriggered, thanks! |
@@ -82,6 +83,40 @@ class SparkConnectArtifactManager(sessionHolder: SessionHolder) extends Logging | |||
*/ | |||
def getSparkConnectPythonIncludes: Seq[String] = pythonIncludeList.asScala.toSeq | |||
|
|||
private def areFilesEqual(path1: Path, path2: Path): Boolean = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you re-use some functions from libs like Apache Common: IOUtils::contentEquals
or FileUtils.contentEquals
. Or there is some reason to don't do that. Please, clarify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good, let me see if we can use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this, thanks for the feedback!
Merged to master. |
The JIRA number was wrong. Let me switch the JIRA with SPARK-45486 |
### What changes were proposed in this pull request? Make addArtifact API retrying on errors. Note this is safe operation since addArtifact is idempotent operation (#43314) ### Why are the changes needed? For the same reasons as we make other API retryable. ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? Added test. Testing by hand against custom spark server. ### Was this patch authored or co-authored using generative AI tooling? No & never Closes #44740 from cdkrot/SPARK-46723-addartifact. Authored-by: Alice Sayutina <alice.sayutina@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
What changes were proposed in this pull request?
Before the update the error looks like that
Now it looks:
Why are the changes needed?
Makes retrying more robust, adds user-friendly error (see above).
Does this PR introduce any user-facing change?
Mostly internal improvements
How was this patch tested?
Unit testing, testing against server
Was this patch authored or co-authored using generative AI tooling?
No