Plugin-498: Added content-type to GCS sink by flakrimjusufi · Pull Request #462 · data-integrations/google-cloud

flakrimjusufi · 2020-12-21T09:52:20Z

Add content-type to GCS sink

JIRA Ticket: https://cdap.atlassian.net/browse/PLUGIN-498

chtyim · 2020-12-21T16:43:45Z

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

      }
-
+      if (contentType == null) {
+        contentType = DEFAULT_CONTENT_TYPE;


Shouldn't modify field in the validate method. Instead, introduce "getter" method do the defaulting logic in there. In the validate method, call the getter method for validation if needed.

Please check the updated PR.

chtyim · 2020-12-21T16:47:00Z

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

  private static final String RECORDS_UPDATED_METRIC = "records.updated";
  public static final String AVRO_NAMED_OUTPUT = "avro.mo.config.namedOutput";
  public static final String COMMON_NAMED_OUTPUT = "mapreduce.output.basename";
+  public static final String CONTENT_TYPE = "Content-Type";


Prefix the FS configuration key. Also, use "." notation. E.g. "io.cdap.gcs.batch.sink.content.type".

This was changed to public static final String CONTENT_TYPE = "io.cdap.gcs.batch.sink.content.type"; in the updated PR.

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

CuriousVini · 2020-12-21T18:28:06Z

@flakrimjusufi If multiple GCS sinks are being used in a single pipeline with different content type. What is the behavior? Any conflicts for content type?

CuriousVini

Please make sure changes have been tested for cases:

Macros
Backwards compatibility
Also please add unit tests to these and modify integration tests to support this fix

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

docs/GCS-batchsink.md

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

docs/GCS-batchsink.md

CuriousVini · 2020-12-21T20:58:26Z

/gcbrun

flakrimjusufi · 2020-12-22T13:17:13Z

Please make sure changes have been tested for cases:

Macros

Backwards compatibility

Also please add unit tests to these and modify integration tests to support this fix

We tested with Macro and everything was okay. The pipeline was able to run successfully.
We tested the pipelines with older versions of plugin and the pipelines were able to run successfully. We also did some further tests in CDAP 6.1.4 and CDAP 6.2.3 and everything worked fine.
Please check the updated PR.

flakrimjusufi · 2020-12-22T13:20:00Z

@flakrimjusufi If multiple GCS sinks are being used in a single pipeline with different content type. What is the behavior? Any conflicts for content type?

We did some tests with multiple GCS sink with different content types and the behavior was as expected. There were no conflicts for content type. The pipelines were able to run successfully.

CuriousVini · 2020-12-28T21:18:17Z

Please make sure changes have been tested for cases:

Macros

Backwards compatibility

Also please add unit tests to these and modify integration tests to support this fix

We tested with Macro and everything was okay. The pipeline was able to run successfully.

We tested the pipelines with older versions of plugin and the pipelines were able to run successfully. We also did some further tests in CDAP 6.1.4 and CDAP 6.2.3 and everything worked fine.

Please check the updated PR.

Could you please add a link to updated integration test PR here?

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

CuriousVini · 2020-12-28T21:31:09Z

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

+    public String getContentType() {
+      if (!Strings.isNullOrEmpty(contentType)) {
+        if (contentType.equals(CONTENT_TYPE_OTHER)) {
+          if (Strings.isNullOrEmpty(customContentType)) {


This means if customContentType is a macro, we use default content type?

Same in here as well, if the value of customContentType is a macro, the value that was specified by the user is being used.

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSOutputFormatProvider.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

bajram-adapt · 2020-12-30T14:44:53Z

Integration tests: cdapio/cdap-integration-tests#1083

flakrimjusufi · 2020-12-30T15:45:33Z

Please make sure changes have been tested for cases:

Macros

Backwards compatibility

Also please add unit tests to these and modify integration tests to support this fix

We tested with Macro and everything was okay. The pipeline was able to run successfully.

We tested the pipelines with older versions of plugin and the pipelines were able to run successfully. We also did some further tests in CDAP 6.1.4 and CDAP 6.2.3 and everything worked fine.

Please check the updated PR.

Could you please add a link to updated integration test PR here?

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/RecordFilterOutputFormat.java

CuriousVini · 2021-01-06T04:34:09Z

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

        }
      }

+      if (containsMacro(NAME_CONTENT_TYPE)) {


no need for this check, by default if its a macro or not provided at configure time, it will be null

PR Updated.

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

CuriousVini · 2021-01-06T04:36:22Z

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

+            failureCollector.addFailure(String.format(
+              "Valid content types for json are %s, %s", CONTENT_TYPE_APPLICATION_JSON,
+              CONTENT_TYPE_TEXT_PLAIN),
+                                        null


nit: move remaining to above line

PR Updated.

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/RecordFilterOutputFormat.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSMultiBatchSink.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/RecordFilterOutputFormat.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSOutputCommitter.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

CuriousVini · 2021-01-08T07:17:15Z

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

+      switch (format) {
+        case FORMAT_AVRO:
+          if (!contentType.equalsIgnoreCase(CONTENT_TYPE_APPLICATION_AVRO)) {
+            failureCollector.addFailure(String.format("Valid content type for avro is %s",


Please add period to the end of all the validation error messages

PR Updated.

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSMultiBatchSink.java

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSOutputCommitter.java

CuriousVini

a few comments. Please squash the commits after fixing.

Also please file a jira for GCS Multi sink plugin

src/test/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSinkTest.java

CuriousVini · 2021-01-12T18:00:21Z

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

    }

+    @Nullable
+    @Description("Gets the value of content type")


this should be a javadoc not annotation. The javadoc should include mapping of format -> content type

PR updated.

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java

flakrimjusufi · 2021-01-13T10:50:20Z

a few comments. Please squash the commits after fixing.

Also please file a jira for GCS Multi sink plugin

The PR is updated according to the latest review. Commits are also squashed.

Here you have the JIRA tikcket for GCS Multi Sink plugin:
https://cdap.atlassian.net/browse/PLUGIN-553

bajram-adapt requested a review from CuriousVini December 21, 2020 14:37

chtyim reviewed Dec 21, 2020

View reviewed changes

CuriousVini requested changes Dec 21, 2020

View reviewed changes

bajram-adapt requested a review from CuriousVini December 22, 2020 14:40

CuriousVini reviewed Dec 28, 2020

View reviewed changes

CuriousVini reviewed Dec 29, 2020

View reviewed changes

CuriousVini added the 6.4 label Dec 29, 2020

flakrimjusufi closed this Dec 30, 2020

flakrimjusufi reopened this Dec 30, 2020

rmstar reviewed Dec 30, 2020

View reviewed changes

src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java Outdated Show resolved Hide resolved

rmstar reviewed Dec 30, 2020

View reviewed changes

src/main/java/io/cdap/plugin/gcp/gcs/sink/RecordFilterOutputFormat.java Outdated Show resolved Hide resolved

bajram-adapt requested a review from CuriousVini January 5, 2021 19:50

CuriousVini reviewed Jan 6, 2021

View reviewed changes

CuriousVini reviewed Jan 8, 2021

View reviewed changes

bajram-adapt requested review from CuriousVini and bajram-adapt January 12, 2021 14:44

CuriousVini reviewed Jan 12, 2021

View reviewed changes

Plugin-498: Added content-type to GCS sink

484b73f

flakrimjusufi force-pushed the feature/PLUGIN-498 branch from 39041a1 to 484b73f Compare January 13, 2021 10:46

bajram-adapt requested a review from CuriousVini January 13, 2021 14:40

CuriousVini approved these changes Jan 15, 2021

View reviewed changes

CuriousVini merged commit f9ec4a1 into data-integrations:develop Jan 15, 2021

This was referenced Jan 18, 2021

Cherrypick for relase/0.15 - PLUGIN-498 #517

Merged

CherryPick for release/0.16 - PLUGIN-498 #518

Merged

Cherrypick for relase/0.14 - PLUGIN-498 #519

Merged

Conversation

flakrimjusufi commented Dec 21, 2020

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!

Uh oh!

CuriousVini commented Dec 21, 2020

Uh oh!

CuriousVini left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CuriousVini commented Dec 21, 2020

Uh oh!

flakrimjusufi commented Dec 22, 2020

Uh oh!

flakrimjusufi commented Dec 22, 2020

Uh oh!

CuriousVini commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajram-adapt commented Dec 30, 2020

Uh oh!

flakrimjusufi commented Dec 30, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CuriousVini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CuriousVini left a comment •

edited

Loading

CuriousVini commented Dec 28, 2020 •

edited

Loading