Add GCS Writer Sink and fix Gcs naming #11328

majetideepak · 2024-10-22T22:33:02Z

Rename GCS to Gcs in code.
Refactor GCS code.
Refactor InsertTests so that both S3 and GCS can reuse the implementation.

netlify · 2024-10-22T22:33:18Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`4cf63d6`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/672dffb942b78a00088acc5e

pedroerp

Thanks @majetideepak. Looks good overall; added a bunch of minor comments.

pedroerp · 2024-10-26T01:54:18Z

velox/connectors/hive/storage_adapters/gcs/GCSUtil.h

@@ -26,6 +26,14 @@ constexpr std::string_view kGCSScheme{"gs://"};

 } // namespace

+static std::string_view kLoremIpsum =


shouldn't this be defined in a test header somewhere?

pedroerp · 2024-10-26T01:57:15Z

velox/connectors/hive/storage_adapters/gcs/tests/GCSFileSystemTest.cpp


+namespace facebook::velox::filesystems {
+namespace {


newline after namespace definition

pedroerp · 2024-10-26T01:58:17Z

velox/connectors/hive/storage_adapters/gcs/tests/GCSFileSystemTest.cpp

-    return preexistingBucketPath() + preexistingObjectName();
+ public:
+  void SetUp() {
+    testbench_ = std::make_shared<GCSTestbench>();


shouldn't this be called GcsTestBench instead (same for the file)? Also, what is up with the "bench" in the name? Is this a benchmark utility?

This uses the storage-testbench library. That is probably why. I renamed it to GcsEmulator
https://github.com/googleapis/storage-testbench

pedroerp · 2024-10-26T02:00:00Z

velox/connectors/hive/storage_adapters/gcs/tests/GCSFileSystemTest.cpp


-  filesystems::GCSFileSystem gcfs(testGcsOptions());
+  filesystems::GCSFileSystem gcfs(testbench_->hiveConfig());


same here. Acronyms are not all upper cases (GcsFileSystem)

fixed everywhere.

pedroerp · 2024-10-26T02:05:04Z

velox/connectors/hive/storage_adapters/gcs/tests/GCSTestbench.h

+class GCSTestbench : public testing::Environment {
+ public:
+  GCSTestbench() {
+    auto port = std::to_string(facebook::velox::exec::test::getFreePorts(1)[0]);


getFreePort() instead

pedroerp · 2024-10-26T02:07:17Z

velox/connectors/hive/storage_adapters/gcs/tests/GCSTestbench.h

+      serverProcess_.terminate();
+      serverProcess_.wait();
+    }
+    if (serverProcess_.valid() && serverProcess_.valid())


is this intended (checking the same condition twice?). Also, curly braces missing

pedroerp · 2024-10-26T02:08:42Z

velox/connectors/hive/storage_adapters/gcs/tests/GCSTestbench.h

+
+namespace facebook::velox::filesystems {
+
+class GCSTestbench : public testing::Environment {


let's rename this to something more descriptive and intuitive. "bench" implies benchmarking which is not the intent.

Named it as GcsEmulator.

pedroerp · 2024-10-26T02:12:23Z

velox/connectors/hive/storage_adapters/gcs/tests/GCSTestbench.h

+
+    objectName_ = "test-object-name";
+    google::cloud::StatusOr<gcs::ObjectMetadata> object =
+        client.InsertObject(bucketName_, objectName_, kLoremIpsum);


should probably move the loremIpsum stuff here.

pedroerp · 2024-10-28T16:58:27Z

velox/connectors/hive/storage_adapters/gcs/tests/GcsFileSystemTest.cpp

+class GcsFileSystemTest : public testing::Test {
+ public:
+  void SetUp() {
+    testbench_ = std::make_shared<GcsEmulator>();


nit: could also call this variable something else (emulator_)?

facebook-github-bot · 2024-10-28T16:59:19Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pedroerp

Thanks @majetideepak

facebook-github-bot · 2024-10-31T23:32:06Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-11T17:56:52Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-13T22:01:14Z

@kgpai merged this pull request in 08dba0f.

conbench-facebook · 2024-11-13T22:30:38Z

Conbench analyzed the 1 benchmark run on commit 08dba0f7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Rename GCS to Gcs in code. Refactor GCS code. Refactor InsertTests so that both S3 and GCS can reuse the implementation. Pull Request resolved: facebookincubator#11328 Reviewed By: DanielHunte Differential Revision: D65069865 Pulled By: kgpai fbshipit-source-id: 4020722f97d9a246d1ed765b697c4f1e8d3a0c2b

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2024

majetideepak force-pushed the gcs-write-sink branch 3 times, most recently from dd238f3 to 7f01fe0 Compare October 24, 2024 12:18

majetideepak marked this pull request as ready for review October 24, 2024 23:29

majetideepak requested a review from assignUser as a code owner October 24, 2024 23:29

majetideepak marked this pull request as draft October 24, 2024 23:29

majetideepak marked this pull request as ready for review October 25, 2024 10:36

majetideepak force-pushed the gcs-write-sink branch from d864ae7 to 3efec9f Compare October 25, 2024 10:41

majetideepak requested review from kgpai and pedroerp October 25, 2024 10:43

majetideepak force-pushed the gcs-write-sink branch 3 times, most recently from 10bbb77 to 9da7598 Compare October 25, 2024 16:52

pedroerp reviewed Oct 26, 2024

View reviewed changes

majetideepak changed the title ~~Add GCS Writer Sink~~ Add GCS Writer Sink and fix naming Oct 26, 2024

majetideepak changed the title ~~Add GCS Writer Sink and fix naming~~ Add GCS Writer Sink and fix Gcs naming Oct 26, 2024

majetideepak force-pushed the gcs-write-sink branch 2 times, most recently from ead10a1 to 4ee20f9 Compare October 27, 2024 11:13

pedroerp reviewed Oct 28, 2024

View reviewed changes

pedroerp approved these changes Oct 28, 2024

View reviewed changes

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Oct 29, 2024

majetideepak mentioned this pull request Oct 30, 2024

[native] Allow new Velox GCS changes prestodb/presto#23917

Merged

majetideepak force-pushed the gcs-write-sink branch from 166dd74 to 2936186 Compare October 31, 2024 23:30

majetideepak added 3 commits November 8, 2024 07:09

Add GCS Writer Sink

c1a3d27

fix dep and ci

ace76fa

fix test

1503412

majetideepak added 2 commits November 8, 2024 07:09

reuse test plan

3f88591

more cleanup

dfd735a

majetideepak force-pushed the gcs-write-sink branch from 2936186 to 4cf63d6 Compare November 8, 2024 12:10

majetideepak added 3 commits November 8, 2024 07:10

address review comments and Gcs naming

ed53c72

Cleanup GcsEmulator

37ccc35

address review comments

4cf63d6

facebook-github-bot closed this in 08dba0f Nov 13, 2024

facebook-github-bot added the Merged label Nov 13, 2024

zuyu mentioned this pull request Nov 13, 2024

[native] Advance Velox and Fix Gcs naming prestodb/presto#24037

Merged

zhouyuan mentioned this pull request Nov 14, 2024

[GLUTEN-6887][VL] Daily Update Velox Version (2024_11_14) apache/incubator-gluten#7942

Merged

majetideepak deleted the gcs-write-sink branch December 17, 2024 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GCS Writer Sink and fix Gcs naming #11328

Add GCS Writer Sink and fix Gcs naming #11328

majetideepak commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading

pedroerp left a comment

pedroerp Oct 26, 2024

pedroerp Oct 26, 2024

pedroerp Oct 26, 2024

majetideepak Oct 26, 2024

pedroerp Oct 26, 2024

majetideepak Oct 26, 2024

pedroerp Oct 26, 2024

pedroerp Oct 26, 2024

pedroerp Oct 26, 2024

majetideepak Oct 26, 2024

pedroerp Oct 26, 2024

pedroerp Oct 28, 2024

facebook-github-bot commented Oct 28, 2024

pedroerp left a comment

facebook-github-bot commented Oct 31, 2024

facebook-github-bot commented Nov 11, 2024

facebook-github-bot commented Nov 13, 2024

conbench-facebook bot commented Nov 13, 2024

		@@ -26,6 +26,14 @@ constexpr std::string_view kGCSScheme{"gs://"};

		} // namespace

		static std::string_view kLoremIpsum =


		filesystems::GCSFileSystem gcfs(testGcsOptions());
		filesystems::GCSFileSystem gcfs(testbench_->hiveConfig());


		namespace facebook::velox::filesystems {

		class GCSTestbench : public testing::Environment {

Add GCS Writer Sink and fix Gcs naming #11328

Add GCS Writer Sink and fix Gcs naming #11328

Conversation

majetideepak commented Oct 22, 2024 • edited Loading

netlify bot commented Oct 22, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

pedroerp 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

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

Choose a reason for hiding this comment

facebook-github-bot commented Oct 28, 2024

pedroerp left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 31, 2024

facebook-github-bot commented Nov 11, 2024

facebook-github-bot commented Nov 13, 2024

conbench-facebook bot commented Nov 13, 2024

majetideepak commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading