Support GCS for delta sharing Server #81

kohei-tosshy · 2021-11-21T12:30:16Z

This is a PR for #20.

I implemented GCS support by using Google Cloud Client Library (https://cloud.google.com/storage/docs/reference/libraries) and Google Cloud Storage connector (https://cloud.google.com/dataproc/docs/concepts/connectors/cloud-storage).
Google Cloud Storage connector (GCS connector) has a dependency problem with Apache Spark releated with Google Guava, so I added this library's shaded jar in server/lib (SBT Unmanaged dependencies, not Managed dependencies in build.sbt).
I implemented this very forcefully, is this no problem?

To use GCP for delta sharing server, you have to do 2 steps.

In your terminal, Do export GOOGLE_APPLICATION_CREDENTIALS=</path/to/your_gcp_service_acount_key.json>
In the server config (e,g, delta-sharing-server.yaml), add a setting for GCP

shares:
- name: "share_gcp"
  schemas:
  - name: "schema_gcp"
    tables:
    - name: "table_gcp"
      location: "gs://delta-start/delta/"

If you want to coneect this server from Apache Spark, execute spark-shell or spark-submit like below.

spark-shell \
--packages io.delta:delta-core_2.12:1.0.0,io.delta:delta-contribs_2.12:1.0.0,io.delta:delta-sharing-spark_2.12:0.2.0 \
--jars </path/to/gcs-connector-latest-hadoop2.jar> \
--conf spark.delta.logStore.oci.impl=io.delta.storage.GCSLogStore \
--conf spark.hadoop.fs.gs.impl=com.google.cloud.hadoop.fs.gcs.GoogleHadoopFS \
--conf spark.hadoop.google.cloud.auth.service.account.enable=true 
--conf spark.hadoop.google.cloud.auth.service.account.json.keyfile=</path/to/your_gcp_service_acount_key.json>

I haven't implement integration test on GCP support yet.
If I understand correctly, what I should do is to add the almost same test with Azure support (https://github.com/delta-io/delta-sharing/blob/main/server/src/test/scala/io/delta/sharing/server/DeltaSharingServiceSuite.scala#L510).
Is this correct?
If my understanding is correct, I'll add this integreation test later.

Signed-off-by: Kohei Toshimitsu <k.tosshy.20@gmail.com>

zsxwing · 2021-11-30T06:57:34Z

Thanks for the contribution. This is awesome!

so I added this library's shaded jar in server/lib (SBT Unmanaged dependencies, not Managed dependencies in build.sbt).

Is Guava the only issue? If so, we can just exclude guava from other dependencies like this a6bd550 This is better than adding a shaded jar to the project.

In your terminal, Do export GOOGLE_APPLICATION_CREDENTIALS=</path/to/your_gcp_service_acount_key.json>

Does the GOOGLE_APPLICATION_CREDENTIALS env work for reading files using GoogleHadoopFileSystem? I saw GCS connector asks users to set a hadoop configuration google.cloud.auth.service.account.json.keyfile: https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/master/gcs/INSTALL.md#configuring-hadoop Since this is the recommended way, can we just read the key file from the hadoop configuration? This will be similar to https://github.com/delta-io/delta-sharing#azure-data-lake-storage-gen2

If I understand correctly, what I should do is to add the almost same test with Azure support

Correct. I will do some manual test with your change for now. We can add the real integration tests later.

zsxwing · 2021-12-07T23:58:03Z

@kohei-tosshy Any thoughts on my above questions?

kohei-tosshy · 2021-12-11T13:20:27Z

@zsxwing

Thank you for your comment.
And sorry for being late for reacting.

Is Guava the only issue? If so, we can just exclude guava from other dependencies like this a6bd550 This is better than adding a shaded jar to the project.

I see. I'll try.

Does the GOOGLE_APPLICATION_CREDENTIALS env work for reading files using GoogleHadoopFileSystem? I saw GCS connector asks users to set a hadoop configuration google.cloud.auth.service.account.json.keyfile: https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/master/gcs/INSTALL.md#configuring-hadoop Since this is the recommended way, can we just read the key file from the hadoop configuration? This will be similar to https://github.com/delta-io/delta-sharing#azure-data-lake-storage-gen2

To get a presigned URL, I used Google Google Cloud Client Library.
I only used GoogleHadoopFileSystem only to changing a path.
GOOGLE_APPLICATION_CREDENTIALS is needed to use Google Google Cloud Client Library.

Correct. I will do some manual test with your change for now. We can add the real integration tests later.

I see. I'll try.

Signed-off-by: Kohei Toshimitsu <k.tosshy.20@gmail.com>

kohei-tosshy · 2021-12-19T13:47:07Z

@zsxwing

Sorry for being late for revising.
I revised Google Guava's dependency problem and added an integration test for GCS support.

zhuansunxt

The code change LGTM.

@zsxwing Do you want to have another look? It'll be nice to include this in the upcoming release.

zhuansunxt · 2022-01-04T07:30:41Z

.gitignore

@@ -21,7 +21,6 @@
 .pydevproject
 .scala_dependencies
 .settings
-/lib/


Revert this change?

I reverted this.

zhuansunxt · 2022-01-04T07:33:46Z

server/src/main/scala/io/delta/sharing/server/CloudFileSigner.scala

+  override def sign(path: Path): String = {
+    val absPath = path.toUri
+    val bucketName = absPath.getHost
+    val objectName = absPath.getPath.stripPrefix("/")


Would be great to have a check assert(objectName.nonEmpty, s"cannot get object key from $path") similar to AWS and Azure signer

I added this.

zsxwing · 2022-01-04T08:09:01Z

To get a presigned URL, I used Google Google Cloud Client Library.
I only used GoogleHadoopFileSystem only to changing a path.
GOOGLE_APPLICATION_CREDENTIALS is needed to use Google Google Cloud Client Library.

Google Google Cloud Client Library also supports other ways to config credentials. But it seems hard to mimic what GoogleHadoopFileSystem does to support various of ways to config credentials. GOOGLE_APPLICATION_CREDENTIALS env is a good start right now. We can support other ways in future if people are asking.

Left some minor comments. Otherwise LGTM.

zsxwing · 2022-01-04T08:03:01Z

server/src/main/scala/io/delta/sharing/server/CloudFileSigner.scala

+class GCSFileSigner(
+    name: URI,
+    conf: Configuration,
+    preSignedUrlTimeoutSeconds: Long) extends CloudFileSigner{


nit

Suggested change

preSignedUrlTimeoutSeconds: Long) extends CloudFileSigner{

preSignedUrlTimeoutSeconds: Long) extends CloudFileSigner {

I revised this.

zsxwing · 2022-01-04T08:03:51Z

server/src/main/scala/io/delta/sharing/server/CloudFileSigner.scala

+    val absPath = path.toUri
+    val bucketName = absPath.getHost
+    val objectName = absPath.getPath.stripPrefix("/")
+    val storage = StorageOptions.newBuilder.build.getService


Is storage thread-safe? If so, we can save it as a val in GCSFileSigner to avoid loading the credentials for each url.

I think storage is thread-safe.
One instance of GCSFileSigner is created in each request for client, so each thread for a request has a dedicated instance of GCSFileSigner and won't touch other GCSFileSigner instances.
And, storage is used only for reading, so it will be OK if some threads for creating a presignied-URL are processed in paralle.

zsxwing · 2022-01-04T08:08:36Z

server/src/test/scala/io/delta/sharing/server/DeltaSharingServiceSuite.scala

@@ -551,4 +551,34 @@ class DeltaSharingServiceSuite extends FunSuite with BeforeAndAfterAll {
    }
    assert(e.getMessage.contains("Server returned HTTP response code: 403")) // 403 Forbidden
  }
+
+  integrationTest("gcs support") {


nit: Could you use ignore for this test for now? We will set up the credentials and enable it later.

I used ignore instead of integrationTest.

Signed-off-by: Kohei Toshimitsu <k.tosshy.20@gmail.com>

zsxwing

LGTM! Thanks!

kohei-tosshy and others added 3 commits November 21, 2021 21:01

Support Google Cloud Storage in Delta Sharing Server

5377da4

Merge branch 'delta-io:main' into main

332638c

Revise a bit on bulid.sbt

e3de422

Signed-off-by: Kohei Toshimitsu <k.tosshy.20@gmail.com>

Revise guava dependency & add intagration test for GCS support

d9be568

Signed-off-by: Kohei Toshimitsu <k.tosshy.20@gmail.com>

zhuansunxt approved these changes Jan 4, 2022

View reviewed changes

zsxwing reviewed Jan 4, 2022

View reviewed changes

Some revises for the code reviews

2e5bd53

Signed-off-by: Kohei Toshimitsu <k.tosshy.20@gmail.com>

zsxwing approved these changes Jan 7, 2022

View reviewed changes

zsxwing merged commit 1d27064 into delta-io:main Jan 7, 2022

zsxwing mentioned this pull request Jan 7, 2022

Support Google Cloud Storage in Delta Sharing Server #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support GCS for delta sharing Server #81

Support GCS for delta sharing Server #81

kohei-tosshy commented Nov 21, 2021

zsxwing commented Nov 30, 2021

zsxwing commented Dec 7, 2021

kohei-tosshy commented Dec 11, 2021 •

edited

Loading

kohei-tosshy commented Dec 19, 2021

zhuansunxt left a comment

zhuansunxt Jan 4, 2022

kohei-tosshy Jan 6, 2022 •

edited

Loading

zhuansunxt Jan 4, 2022

kohei-tosshy Jan 6, 2022 •

edited

Loading

zsxwing commented Jan 4, 2022

zsxwing Jan 4, 2022

kohei-tosshy Jan 6, 2022

zsxwing Jan 4, 2022

kohei-tosshy Jan 6, 2022

zsxwing Jan 4, 2022

kohei-tosshy Jan 6, 2022

zsxwing left a comment

	preSignedUrlTimeoutSeconds: Long) extends CloudFileSigner{
	preSignedUrlTimeoutSeconds: Long) extends CloudFileSigner {

Support GCS for delta sharing Server #81

Support GCS for delta sharing Server #81

Conversation

kohei-tosshy commented Nov 21, 2021

zsxwing commented Nov 30, 2021

zsxwing commented Dec 7, 2021

kohei-tosshy commented Dec 11, 2021 • edited Loading

kohei-tosshy commented Dec 19, 2021

zhuansunxt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohei-tosshy Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohei-tosshy Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

zsxwing commented Jan 4, 2022

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

zsxwing left a comment

Choose a reason for hiding this comment

kohei-tosshy commented Dec 11, 2021 •

edited

Loading

kohei-tosshy Jan 6, 2022 •

edited

Loading

kohei-tosshy Jan 6, 2022 •

edited

Loading