You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "madisonmlin (via GitHub)" <gi...@apache.org> on 2025/10/21 20:06:55 UTC
[PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
madisonmlin opened a new pull request, #3995:
URL: https://github.com/apache/texera/pull/3995
### Purpose ###
This pull request partially addresses issue #3404 by changing the download method of single-file dataset downloads to use browser native downloads. Using the browser to handle downloads enables the browser's file download UI, which includes a progress bar when downloading larger files.
Previously, we would generate a pre-signed URL for the file via LakeFS API and download it as a Blob. Now, we generate a pre-signed URL for the file via S3 Gateway API, which offers additional flexibility, and click on the URL to trigger browser native download.
### Changes ###
- added method to generate pre-signed URL via S3 Gateway API, with additional flexibility to specify file name and type
- updated frontend and backend to directly click on a pre-signed URL generated via the new method for single file dataset downloads
- removed success and error notifications for single file dataset downloads (the success and error of a file download cannot be obtained when using browser native download)
- updated unit tests to verify new download method
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471169830
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
I don't believe so. If we don't override to specify the endpoint our s3Presigner should go through, it will use the default AWS endpoint. Then, the generated presigned URL will point to AWS instead of LakeFS and be able to access the target file.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471748365
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
Sorry, as an amendment to my previous explanation, the LakeFS presigner actually does have the LakeFS endpoint explicitly specified:
```
private lazy val apiClient: ApiClient = {
val client = new ApiClient()
client.setApiKey(StorageConfig.lakefsPassword)
client.setUsername(StorageConfig.lakefsUsername)
client.setPassword(StorageConfig.lakefsPassword)
client.setServers(
List(
new ServerConfiguration(
StorageConfig.lakefsEndpoint,
"LakeFS API server endpoint",
new java.util.HashMap[String, ServerVariable]()
)
).asJava
)
client
}
```
This `apiClient` is used to generate the pre-signed URL. It uses the same `StorageConfig.lakefsEndpoint` that I use to override the s3Presigner endpoint. The only difference is that I parse to remove "api/v1" from the endpoint for the s3Presigner. Could this be causing the incorrect URL issue you mention?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471169830
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
I don't believe so. If we don't override to specify the endpoint our s3Presigner should go through, it will use the default AWS endpoint. Then, the generated presigned URL will point to AWS instead of LakeFS and won't be able to get the file we want.
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +57,31 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val lakefsCredentials =
+ AwsBasicCredentials.create(StorageConfig.lakefsUsername, StorageConfig.lakefsPassword)
Review Comment:
s3Credentials and lakefsCredentials use different username and password, so it's not exactly repeated. Should I unify this into a separate helper?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "aicam (via GitHub)" <gi...@apache.org>.
aicam commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471699756
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
If LakeFS has the URL, why don't you get it from there instead of environment variable? this environment variable is not correct on production environment.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471169830
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
I don't believe so. If we don't override to specify the endpoint our s3Presigner should go through, it will use the default AWS endpoint. Then, the generated presigned URL will point to AWS instead of LakeFS and won't be able to access the target file.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "aicam (via GitHub)" <gi...@apache.org>.
aicam commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471279510
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
I don't understand why AWS is involved here. AWS provides libraries to use MinIO because MinIO is using same protocol, However, it should not cause any issue for us. Currently, the path to MinIO is defined in LakeFS config (called blockstore), and it was using it in previous S3 signed URL generation, why its not using that anymore? maybe a better question is: Why your implementation needs to override baseURL but previous one didn't?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2474500983
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
The LakeFS presigner can be found under `LakeFSStorageClient.scala`:
https://github.com/apache/texera/blob/b7944f3129973fe8a73e92c09e7594b2bff4930b/common/workflow-core/src/main/scala/org/apache/amber/core/storage/util/LakeFSStorageClient.scala
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on PR #3995:
URL: https://github.com/apache/texera/pull/3995#issuecomment-3463015762
The new S3 presigner uses the same environment variable endpoint as the original LakeFS presigner. Here is reference to the apiClient used by LakeFS presigner, which sets the server endpoint (`StorageConfig.lakefsEndpoint`):
```
private lazy val apiClient: ApiClient = {
val client = new ApiClient()
client.setApiKey(StorageConfig.lakefsPassword)
client.setUsername(StorageConfig.lakefsUsername)
client.setPassword(StorageConfig.lakefsPassword)
client.setServers(
List(
new ServerConfiguration(
StorageConfig.lakefsEndpoint,
"LakeFS API server endpoint",
new java.util.HashMap[String, ServerVariable]()
)
).asJava
)
client
}
```
The original LakeFS presigner can be found under `LakeFSStorageClient.scala`: https://github.com/apache/texera/blob/b7944f3129973fe8a73e92c09e7594b2bff4930b/common/workflow-core/src/main/scala/org/apache/amber/core/storage/util/LakeFSStorageClient.scala
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471589945
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
The previous presigner used LakeFS's own API, so it already implicitly knew its own endpoint through the LakeFS config. However, the s3Presigner is a generic AWS SDK tool that MinIO happens to be compatible with. It isn't intended solely for LakeFS. As such, it doesn't know that we want to use it specifically for LakeFS, and instead defaults to the AWS S3 endpoint.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "aicam (via GitHub)" <gi...@apache.org>.
aicam commented on PR #3995:
URL: https://github.com/apache/texera/pull/3995#issuecomment-3534178072
@madisonmlin unfortunately, after iterating over your implementation, I realized that it is wrong since it expose LakeFS to outside and I could not find any implementation in their library, so I contacted the developers of LakeFS in Slack and they asked me to create an issue and request the feature. The [issue](https://github.com/treeverse/lakeFS/issues/9673) is now open and I will get back to this PR after they add this feature.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "chenlica (via GitHub)" <gi...@apache.org>.
chenlica commented on PR #3995:
URL: https://github.com/apache/texera/pull/3995#issuecomment-3430265219
@madisonmlin Please follow our PR template. Check https://github.com/apache/texera/pull/3960 as an example.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "madisonmlin (via GitHub)" <gi...@apache.org>.
madisonmlin commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471589945
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
The previous presigner used LakeFS's own API, so it already implicitly knew its own endpoint through the LakeFS config. However, the s3Presigner is a generic AWS SDK tool that MinIO happens to be compatible with. It isn't intended solely for LakeFS. As such, it defaults to the AWS S3 endpoint unless explicitly overriden.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "chenlica (via GitHub)" <gi...@apache.org>.
chenlica commented on PR #3995:
URL: https://github.com/apache/texera/pull/3995#issuecomment-3536154002
@aicam Thanks. Does the new finding affect any PR?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "aicam (via GitHub)" <gi...@apache.org>.
aicam commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2471131589
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +57,31 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val lakefsCredentials =
+ AwsBasicCredentials.create(StorageConfig.lakefsUsername, StorageConfig.lakefsPassword)
Review Comment:
This line is repeated two times, unify them
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "aicam (via GitHub)" <gi...@apache.org>.
aicam closed pull request #3995: feat: switch single-file dataset downloads to use browser native downloads
URL: https://github.com/apache/texera/pull/3995
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: switch single-file dataset downloads to use browser native downloads [texera]
Posted by "aicam (via GitHub)" <gi...@apache.org>.
aicam commented on code in PR #3995:
URL: https://github.com/apache/texera/pull/3995#discussion_r2449578817
##########
file-service/src/main/scala/org/apache/texera/service/util/S3StorageClient.scala:
##########
@@ -51,6 +56,29 @@ object S3StorageClient {
.build()
}
+ // Initialize S3-compatible presigner for LakeFS S3 Gateway
+ private lazy val s3Presigner: S3Presigner = {
+ val fullUri = new URI(StorageConfig.lakefsEndpoint)
Review Comment:
Can we avoid this override?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org