Spark Client StorageID support #8918

itaigilo · 2025-04-01T10:21:09Z

Supporting Spark Client usage for lakeFS servers with multiple blockstores.
Also includes GC support.

Required updating lakeFS to the latest version.

Still working on testing it, but this can be reviewed.

github-actions · 2025-04-01T10:28:03Z

E2E Test Results - Quickstart

github-actions · 2025-04-01T10:29:31Z

E2E Test Results - DynamoDB Local - Local Block Adapter

nopcoder · 2025-04-02T10:56:07Z

clients/spark/src/main/scala/io/treeverse/clients/ApiClient.scala

+        if (storageConfigList.isEmpty || storageConfigList.size() == 1) {
+          cfg.getStorageConfig
+        } else {


In case of a non empty list, we still need to pick the configuration from the list - right?

In such case, it'll be returned in the non-list part of the response.
Same handling as in the other clients.

nopcoder · 2025-04-02T11:15:39Z

clients/spark/src/main/scala/io/treeverse/clients/ApiClient.scala

@@ -168,7 +168,7 @@ class ApiClient private (conf: APIConfigurations) {
    val storageNamespace = key.storageClientType match {
      case StorageClientType.HadoopFS =>
        ApiClient
-          .translateURI(URI.create(repo.getStorageNamespace), getBlockstoreType)
+          .translateURI(URI.create(repo.getStorageNamespace), getBlockstoreType(repo.getStorageId))


The storage namespace information is cached in this client.
The blockstore type is not cached and it means that we call the server multiple times.
Suggest to add this information to the cache or cache the configucation at the same level.

I'm adding a getRepo() call to the server, but it's not here, and only for the GC.

What cache are you suggesting here?

was looking at the namespace cache and missed the key to namespace cache - can ignore my comment.

arielshaqed

Thanks, looks like a worthwhile change! Surprised its scope is so low. Let's add how we tested this.

arielshaqed · 2025-04-02T11:25:31Z

clients/spark/build.sbt

@@ -42,7 +42,7 @@ buildInfoPackage := "io.treeverse.clients"
 enablePlugins(S3Plugin, BuildInfoPlugin)

 libraryDependencies ++= Seq(
-  "io.lakefs" % "sdk" % "1.0.0",
+  "io.lakefs" % "sdk" % "1.53.1",


Let's have this as a separate PR! It's not that I'm afraid, the lakeFS compatibility guarantees means this is a perfectly safe change. But I'd still rather be careful because I'm actually scared by this change.

I'd don't mind separating this, but since the CI tests pass here, I'm curious -
What difference will it make?
If merging the lib bump and right after it the MSB changes -
How will it give more confidence?

Because we squash merges, 2 PRs give a more detailed commit history. That makes it easier to git bisect and/or roll back some changes. But not critical.

arielshaqed · 2025-04-02T11:28:07Z

clients/spark/src/main/scala/io/treeverse/clients/ApiClient.scala

-        cfg.getStorageConfig
+        val storageConfigList = cfg.getStorageConfigList
+        if (storageConfigList.isEmpty || storageConfigList.size() == 1) {
+          cfg.getStorageConfig


We could still check storageID when the list has size 1 and specifies a storage ID, no?

I'm complying with the other clients (WebUI, lakectl, Everest) -
Were doing the same check.
It's about making the distinction, and using the list only for actual MSB cases.

itaigilo

Thanks @arielshaqed and @nopcoder for your review.

As written, still on to testing this 😐
Will update afterwards.

Meanwhile, addressed your comments.

itaigilo · 2025-04-02T13:14:44Z

clients/spark/build.sbt

@@ -42,7 +42,7 @@ buildInfoPackage := "io.treeverse.clients"
 enablePlugins(S3Plugin, BuildInfoPlugin)

 libraryDependencies ++= Seq(
-  "io.lakefs" % "sdk" % "1.0.0",
+  "io.lakefs" % "sdk" % "1.53.1",


I'd don't mind separating this, but since the CI tests pass here, I'm curious -
What difference will it make?
If merging the lib bump and right after it the MSB changes -
How will it give more confidence?

itaigilo · 2025-04-02T13:16:11Z

clients/spark/src/main/scala/io/treeverse/clients/ApiClient.scala

@@ -168,7 +168,7 @@ class ApiClient private (conf: APIConfigurations) {
    val storageNamespace = key.storageClientType match {
      case StorageClientType.HadoopFS =>
        ApiClient
-          .translateURI(URI.create(repo.getStorageNamespace), getBlockstoreType)
+          .translateURI(URI.create(repo.getStorageNamespace), getBlockstoreType(repo.getStorageId))


I'm adding a getRepo() call to the server, but it's not here, and only for the GC.

What cache are you suggesting here?

itaigilo · 2025-04-02T13:18:03Z

clients/spark/src/main/scala/io/treeverse/clients/ApiClient.scala

-        cfg.getStorageConfig
+        val storageConfigList = cfg.getStorageConfigList
+        if (storageConfigList.isEmpty || storageConfigList.size() == 1) {
+          cfg.getStorageConfig


I'm complying with the other clients (WebUI, lakectl, Everest) -
Were doing the same check.
It's about making the distinction, and using the list only for actual MSB cases.

itaigilo · 2025-04-02T13:18:47Z

clients/spark/src/main/scala/io/treeverse/clients/ApiClient.scala

+        if (storageConfigList.isEmpty || storageConfigList.size() == 1) {
+          cfg.getStorageConfig
+        } else {


In such case, it'll be returned in the non-list part of the response.
Same handling as in the other clients.

arielshaqed

Nothing real to add, and only comment is not critical. StillLGTM.

arielshaqed · 2025-04-03T12:10:05Z

clients/spark/build.sbt

@@ -42,7 +42,7 @@ buildInfoPackage := "io.treeverse.clients"
 enablePlugins(S3Plugin, BuildInfoPlugin)

 libraryDependencies ++= Seq(
-  "io.lakefs" % "sdk" % "1.0.0",
+  "io.lakefs" % "sdk" % "1.53.1",


Because we squash merges, 2 PRs give a more detailed commit history. That makes it easier to git bisect and/or roll back some changes. But not critical.

nopcoder

lgtm!

Spark Client StorageID support

ead530a

itaigilo added include-changelog PR description should be included in next release changelog minor-change Used for PRs that don't require issue attached msb labels Apr 1, 2025

Fix lint

6779738

itaigilo requested review from a team, nopcoder and arielshaqed April 2, 2025 09:26

itaigilo marked this pull request as ready for review April 2, 2025 10:43

nopcoder requested changes Apr 2, 2025

View reviewed changes

arielshaqed approved these changes Apr 2, 2025

View reviewed changes

itaigilo commented Apr 2, 2025

View reviewed changes

itaigilo requested review from nopcoder and arielshaqed April 3, 2025 10:26

arielshaqed reviewed Apr 3, 2025

View reviewed changes

nopcoder approved these changes Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spark Client StorageID support #8918

Spark Client StorageID support #8918

itaigilo commented Apr 1, 2025 •

edited

Loading

github-actions bot commented Apr 1, 2025

github-actions bot commented Apr 1, 2025

nopcoder Apr 2, 2025

itaigilo Apr 2, 2025

nopcoder Apr 2, 2025

itaigilo Apr 2, 2025

nopcoder Apr 3, 2025

arielshaqed left a comment

arielshaqed Apr 2, 2025

itaigilo Apr 2, 2025

arielshaqed Apr 3, 2025

arielshaqed Apr 2, 2025

itaigilo Apr 2, 2025

itaigilo left a comment

itaigilo Apr 2, 2025

itaigilo Apr 2, 2025

itaigilo Apr 2, 2025

itaigilo Apr 2, 2025

arielshaqed left a comment

arielshaqed Apr 3, 2025

nopcoder left a comment

Spark Client StorageID support #8918

Are you sure you want to change the base?

Spark Client StorageID support #8918

Conversation

itaigilo commented Apr 1, 2025 • edited Loading

github-actions bot commented Apr 1, 2025

E2E Test Results - Quickstart

github-actions bot commented Apr 1, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

itaigilo commented Apr 1, 2025 •

edited

Loading