feat: add GCS sink by Chief-Rishab · Pull Request #469 · raystack/meteor

Chief-Rishab · 2023-01-31T07:50:25Z

No description provided.

kushsharma · 2023-01-31T08:02:47Z

plugins/sinks/gcs/client.go

+	"google.golang.org/api/option"
+)
+
+type GCSClient interface {


We can call this Writer?

kushsharma · 2023-01-31T08:04:40Z

plugins/sinks/gcs/README.md

@@ -0,0 +1,41 @@
+# GCS
+
+Sinks json data to a file in `ndjson` format in a Google Cloud Storage bucket


Maybe we can rephrase this as:

Sinks json data to a file as ndjson format in Google Cloud Storage bucket

kushsharma · 2023-01-31T08:06:32Z

plugins/sinks/gcs/client.go

+	writer := client.Bucket(bucketname).Object(filepath).NewWriter(ctx)
+
+	return &gcsClient{
+		client: client,


I don't see client getting used anywhere, should we get rid of it?

yes, its's only used to create the writer, will update it

kushsharma · 2023-01-31T08:08:04Z

plugins/sinks/gcs/client.go

+	writer *storage.Writer
+}
+
+func newGCSClient(ctx context.Context, serviceAccountJSON []byte, bucketname string, filepath string) (GCSClient, error) {


You should return *gcsClient instead of interface.

Remember always return struct and accept interfaces in args

kushsharma · 2023-01-31T08:09:02Z

plugins/sinks/gcs/client.go

+
+func (c *gcsClient) WriteData(jsonBytes []byte) error {
+	if _, err := c.writer.Write(jsonBytes); err != nil {
+		return errors.Wrap(err, "error in writing json data to an object")


Why client should know if the data is json or something else? It's just raw bytes for it. So this error won't be applicable if you decide to write simple text.

kushsharma · 2023-01-31T08:10:48Z

plugins/sinks/gcs/client.go

+func (c *gcsClient) Close() error {
+	if err := c.writer.Close(); err != nil {
+		return errors.Wrap(err, "error closing the writer")
+	}
+
+	return nil
+}


Maybe simplify this as

func (c *gcsClient) Close() error { return c.writer.Close() }

kushsharma · 2023-01-31T08:16:46Z

plugins/sinks/gcs/gcs.go

+func (s *Sink) resolveBucketandObjectNames() (string, string) {
+	dirs := strings.Split(s.config.Path, "/")
+	bucketname := dirs[0]
+	timestamp := time.Now().Format("2006.01.02 15:04:05")


I would suggest using a pre-defined time RFC format, specially the one without space.

will RFC3339 do ? Output format is "2006-01-02T15:04:05Z07:00"

kushsharma · 2023-01-31T08:17:35Z

plugins/sinks/gcs/gcs.go

+	if s.config.ObjectPrefix != "" {
+		s.config.ObjectPrefix = s.config.ObjectPrefix + "-"
+	}


What if user has provided prefix as hello-, you will add double hyphens?

It should be a single hyphen only. I will add a check for that

kushsharma · 2023-01-31T08:18:02Z

plugins/sinks/gcs/gcs.go

+		s.config.ObjectPrefix = s.config.ObjectPrefix + "-"
+	}
+
+	objectname := s.config.ObjectPrefix + timestamp + ".ndjson"


use fmt.Sprintf to build strings.

kushsharma · 2023-01-31T08:19:26Z

plugins/sinks/gcs/gcs.go

+
+	objectname := s.config.ObjectPrefix + timestamp + ".ndjson"
+	if len(dirs) > 1 {
+		objectname = dirs[len(dirs)-1] + "/" + s.config.ObjectPrefix + timestamp + ".ndjson"


use fmt.Sprintf to build strings.

kushsharma · 2023-01-31T08:19:48Z

plugins/sinks/gcs/gcs.go

+}
+
+func (s *Sink) validateServiceAccountKey() error {
+


unnecessary space?

kushsharma · 2023-01-31T08:21:42Z

plugins/sinks/gcs/gcs.go

+	if err := s.client.Close(); err != nil {
+		return err
+	}
+	return nil
+}


return s.client.Close()

kushsharma · 2023-02-06T09:01:42Z

plugins/sinks/gcs/gcs.go

+	return nil
+}
+
+func (s *Sink) resolveBucketandObjectNames() (string, string) {


A better name could be resolveBucketPath

kushsharma · 2023-02-06T09:04:46Z

plugins/sinks/gcs/gcs.go

+	dirs := strings.Split(s.config.Path, "/")
+	bucketname := dirs[0]


A notation pretty common in object storage is gcs://bucketname or s3://bucketname I guess this line would fail here. Can we use url.Parse(...)?

kushsharma · 2023-02-06T09:08:02Z

plugins/sinks/gcs/gcs.go

+	objectname := fmt.Sprintf("%s%s.ndjson", objectprefix, timestamp)
+
+	if len(dirs) > 1 {
+		objectname = fmt.Sprintf("%s/%s%s.ndjson", dirs[len(dirs)-1], objectprefix, timestamp)


How will dirs[len(dirs)-1] work for a path like gcs://bucketname/path1/path2/

The default path format I assumed was the one I got from GCS bucket/folder copy path options. If that's the required case, will handle the gcs prefix

kushsharma · 2023-02-06T09:10:06Z

plugins/sinks/gcs/gcs.go

+	return nil
+}
+
+func (s *Sink) Close() (err error) {


(err error) could be just error

Chief-Rishab · 2023-02-06T18:32:58Z

@kushsharma done with the changes, taking the input as a URL in the sink config, now URL format it accepts in config is gcs://bucketname/optional_folder_paths.

kushsharma · 2023-02-16T16:30:45Z

LGTM. Nice work.

feat: add GCS sink (raystack#469)

Chief-Rishab added 3 commits January 31, 2023 12:37

GCS: add sink, client and documentation

77cf2dc

GCS: add tests

0021f3b

fix: spacing

6935288

kushsharma reviewed Jan 31, 2023

View reviewed changes

plugins/sinks/gcs/client.go Outdated

"google.golang.org/api/option"

)

type GCSClient interface {

Copy link

Member

kushsharma Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can call this Writer?

kushsharma reviewed Jan 31, 2023

View reviewed changes

plugins/sinks/gcs/gcs.go Outdated

}

func (s *Sink) validateServiceAccountKey() error {

Copy link

Member

kushsharma Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary space?

kushsharma reviewed Jan 31, 2023

View reviewed changes

Chief-Rishab added 2 commits January 31, 2023 15:48

chore: apply changes from suggestions

be3cac6

docs: add gcs sink

b2fa819

Chief-Rishab marked this pull request as ready for review January 31, 2023 11:25

ravisuhag changed the title ~~Add GCS sink~~ feat: add GCS sink Jan 31, 2023

kushsharma requested changes Feb 6, 2023

View reviewed changes

kushsharma added enhancement New feature or request sink Add new sink labels Feb 6, 2023

chore: apply changes from review

8e764a0

Chief-Rishab requested a review from kushsharma February 6, 2023 18:37

ravisuhag merged commit a8e255c into main Feb 16, 2023

ravisuhag deleted the gcs-sink branch February 16, 2023 16:51

anjali9791 added a commit to anjali9791/meteor that referenced this pull request Feb 20, 2023

Merge pull request #1 from odpf/main

7f28a7a

feat: add GCS sink (raystack#469)

		@@ -0,0 +1,41 @@
		# GCS

		Sinks json data to a file in `ndjson` format in a Google Cloud Storage bucket

		dirs := strings.Split(s.config.Path, "/")
		bucketname := dirs[0]

Conversation

Chief-Rishab commented Jan 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chief-Rishab Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kushsharma Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kushsharma Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chief-Rishab Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chief-Rishab commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kushsharma commented Feb 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Chief-Rishab Jan 31, 2023 •

edited

Loading

kushsharma Jan 31, 2023 •

edited

Loading

kushsharma Jan 31, 2023 •

edited

Loading

Chief-Rishab Jan 31, 2023 •

edited

Loading

Chief-Rishab commented Feb 6, 2023 •

edited

Loading