-
Notifications
You must be signed in to change notification settings - Fork 167
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
DTS-3385: upgrade hadoop version #499
Conversation
could you rerun the tests, and run them locally to double check? |
build.sbt
Outdated
ExclusionRule("com.fasterxml.jackson.core"), | ||
ExclusionRule("com.fasterxml.jackson.module"), | ||
ExclusionRule("com.google.guava", "guava") | ||
), | ||
"io.delta" %% "delta-standalone" % "0.5.0" excludeAll( | ||
"io.delta" % "delta-kernel-api" % "3.2.0" excludeAll( |
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.
we don't need to include them twice?
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.
fixed
build.sbt
Outdated
ExclusionRule("com.fasterxml.jackson.module"), | ||
ExclusionRule("com.google.guava", "guava") | ||
), | ||
"io.delta" % "delta-storage" % "3.2.0" excludeAll( |
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.
and exclude this delta-storage library?
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.
fixed
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.
LGTM, let's debug the other test failure separately.
build.sbt
Outdated
ExclusionRule("com.fasterxml.jackson.core"), | ||
ExclusionRule("com.fasterxml.jackson.module"), | ||
ExclusionRule("com.google.guava", "guava") | ||
), | ||
"io.delta" %% "delta-standalone" % "0.5.0" excludeAll( | ||
"io.delta" % "delta-kernel-defaults" % "3.2.0" excludeAll( |
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.
This one is also a duplicate of the import below?
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.
fixed
@@ -114,7 +115,7 @@ private[sharing] class RandomAccessHttpInputStream( | |||
} | |||
|
|||
private def createHttpRequest(start: Long): HttpRequestBase = { | |||
val request = new HttpGet(uri) | |||
val request = new HttpGet(URI.create(uri)) |
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.
this is not needed
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.
fixed
@@ -40,6 +40,7 @@ | |||
package io.delta.sharing.client | |||
|
|||
import java.io.{EOFException, InputStream, IOException} | |||
import java.net.URI |
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.
you can revert this.
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.
fixed
@@ -175,7 +176,12 @@ object AbfsFileSigner { | |||
val getRelativePathMethod = classOf[AzureBlobFileSystemStore] | |||
.getDeclaredMethod("getRelativePath", classOf[Path]) | |||
getRelativePathMethod.setAccessible(true) | |||
getRelativePathMethod.invoke(abfsStore, path).asInstanceOf[String] | |||
var relativePath = getRelativePathMethod.invoke(abfsStore, path).asInstanceOf[String] | |||
// remove duplicate separator character for azure relative path |
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.
And could you add this link in the comment as a reference of why we are doing this change: apache/hadoop@af98f32#diff-94925ffd3b21968d7e6b476f7e85f68f5ea326f186262017fad61a5a6a3815cbL1215
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.
done
upgrading hadoop version so it is compatible with delta-kernel