[PLUGIN-1818] ErrorDetailsProvider - BigQuery Source/Sink plugin #1458

psainics · 2024-11-06T10:51:39Z

ErrorDetailsProvider - BigQuery Source/Sink plugin

Description

Adding error details provider on bigquery.

CDAP Error logs

Test Case (Input invalid Json)

2024-11-13 11:19:00,929 - ERROR [Executor task launch worker for task 0.0 in stage 0.0 (TID 0):o.a.s.u.Utils@98] - Aborting task
io.cdap.cdap.api.exception.WrappedStageException: Stage 'BigQuery' encountered : io.cdap.cdap.api.exception.ProgramFailureException: Error occurred in the phase: 'Writing'. Error message: Expected value of Field 'raw' to be a valid JSON object or array.
  at io.cdap.cdap.etl.common.ErrorDetails.handleException(ErrorDetails.java:77)
  at io.cdap.cdap.etl.spark.io.StageTrackingRecordWriter.write(StageTrackingRecordWriter.java:57)
  at org.apache.spark.internal.io.HadoopMapReduceWriteConfigUtil.write(SparkHadoopWriter.scala:368)
  at org.apache.spark.internal.io.SparkHadoopWriter$.$anonfun$executeTask$1(SparkHadoopWriter.scala:138)
  at org.apache.spark.util.Utils$.tryWithSafeFinallyAndFailureCallbacks(Utils.scala:1538)
  at org.apache.spark.internal.io.SparkHadoopWriter$.executeTask(SparkHadoopWriter.scala:135)
  at org.apache.spark.internal.io.SparkHadoopWriter$.$anonfun$write$1(SparkHadoopWriter.scala:88)
  at org.apache.spark.scheduler.ResultTask.runTask(ResultTask.scala:90)
  at org.apache.spark.scheduler.Task.run(Task.scala:136)
  at org.apache.spark.executor.Executor$TaskRunner.$anonfun$run$3(Executor.scala:548)
  at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:1504)
  at org.apache.spark.executor.Executor$TaskRunner.run(Executor.scala:551)
  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
  at java.lang.Thread.run(Thread.java:750)
Caused by: io.cdap.cdap.api.exception.ProgramFailureException: Error occurred in the phase: 'Writing'. Error message: Expected value of Field 'raw' to be a valid JSON object or array.
  at io.cdap.cdap.api.exception.ProgramFailureException$Builder.build(ProgramFailureException.java:186)
  at io.cdap.cdap.api.exception.ErrorUtils.getProgramFailureException(ErrorUtils.java:161)
  at io.cdap.plugin.gcp.common.GCPErrorDetailsProvider.getProgramFailureException(GCPErrorDetailsProvider.java:133)
  at io.cdap.plugin.gcp.common.GCPErrorDetailsProvider.getExceptionDetails(GCPErrorDetailsProvider.java:60)
  at io.cdap.cdap.etl.common.ErrorDetails.handleException(ErrorDetails.java:75)
  ... 14 common frames omitted
Caused by: java.lang.IllegalStateException: Expected value of Field 'raw' to be a valid JSON object or array.
  at io.cdap.plugin.gcp.bigquery.sink.BigQueryRecordToJson.writeSimpleTypes(BigQueryRecordToJson.java:192)
  at io.cdap.plugin.gcp.bigquery.sink.BigQueryRecordToJson.write(BigQueryRecordToJson.java:96)
  at io.cdap.plugin.gcp.bigquery.sink.BigQueryRecordToJson.write(BigQueryRecordToJson.java:71)
  at io.cdap.plugin.gcp.bigquery.sink.BigQueryJsonConverter.transform(BigQueryJsonConverter.java:51)
  at io.cdap.plugin.gcp.bigquery.sink.BigQueryJsonConverter.transform(BigQueryJsonConverter.java:32)
  at io.cdap.plugin.gcp.bigquery.sink.BigQueryRecordWriter.write(BigQueryRecordWriter.java:62)
  at io.cdap.plugin.gcp.bigquery.sink.BigQueryRecordWriter.write(BigQueryRecordWriter.java:33)
  at io.cdap.cdap.etl.spark.io.TrackingRecordWriter.write(TrackingRecordWriter.java:41)
  at io.cdap.cdap.etl.spark.io.StageTrackingRecordWriter.write(StageTrackingRecordWriter.java:55)
  ... 13 common frames omitted
  Suppressed: java.io.IOException: Incomplete document
  	at com.google.gson.internal.bind.JsonTreeWriter.close(JsonTreeWriter.java:196)
  	at io.cdap.plugin.gcp.bigquery.sink.BigQueryJsonConverter.transform(BigQueryJsonConverter.java:56)
  	... 18 common frames omitted

Code change

Added BigQueryErrorDetailsProvider.java
Modified AbstractBigQuerySink.java
Modified BigQueryOutputFormat.java
Modified BigQueryRecordToJson.java
Modified BigQuerySinkUtils.java
Modified BigQueryAvroToStructuredTransformer.java
Modified BigQuerySource.java
Modified BigQuerySourceUtils.java
Modified PartitionedBigQueryInputFormat.java
Modified GCPErrorDetailsProvider.java

Unit Tests

Modified BigQueryRecordToJsonTest.java

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQueryAvroToStructuredTransformer.java

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQuerySource.java

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQuerySourceUtils.java

itsankit-google · 2024-11-13T07:55:30Z

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

+   * @param e The IOException to get the error information from.
+   * @return A ProgramFailureException with the given error information.
+   */
+  private ProgramFailureException getProgramFailureException(IOException e, ErrorContext errorContext) {


How is IOException is a user error? Even the error messages are not at all actionable.

We need to convert only those exceptions to ProgramFailureException where we can get enough information from the exceptions.

In underlying places, if we are throwing IOException we should change it to throw ProgramFailureException rather than assuming IOException will always be user error.

There are IOException being thrown from BigQueryOutputFormat in code eg.

google-cloud/src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQueryOutputFormat.java

Line 168 in 601f62b

throw new IOException("The output path '" + outputPath + "' already exists.");

In underlying places, if we are throwing IOException we should change it to throw ProgramFailureException rather than assuming IOException will always be user error.

Ok got it, wll do these changes.

Changed underlying IOException to ProgramFailureException.

itsankit-google · 2024-11-13T07:55:59Z

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

+   * @param e The IllegalStateException to get the error information from.
+   * @return A ProgramFailureException with the given error information.
+   */
+  private ProgramFailureException getProgramFailureException(IllegalStateException e, ErrorContext errorContext) {


Similarly, how is IllegalStateException a user error?

An IllegalStateException usually represents a programming or configuration error in the code. Error Type System might be better fit as it suggests something went wrong with the program's state or lifecycle that's under the application's control rather than being an external client issue.

In code there are places where we can reach IllegalStateException by incorrect user inputs, either we should change those.

google-cloud/src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQueryRecordToJson.java

Line 192 in 601f62b

throw new IllegalStateException(String.format("Expected value of Field '%s' to be a valid JSON " +

this above should be changed to IllegalArgumentException right ?

Changed IllegalStateException to SYSTEM

this above should be changed to IllegalArgumentException right ?

right.

itsankit-google · 2024-11-13T08:08:30Z

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

@@ -64,7 +74,7 @@ public ProgramFailureException getExceptionDetails(Exception e, ErrorContext err
   * @return A ProgramFailureException with the given error information.
   */
  private ProgramFailureException getProgramFailureException(HttpResponseException e,
-    ErrorContext errorContext) {
+                                                               ErrorContext errorContext) {


nit: indentation.

itsankit-google · 2024-11-13T15:39:59Z

src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQueryOutputFormat.java

+              errorMessage, numOfErrors, jobReference.getJobId());
+            throw ErrorUtils.getProgramFailureException(
+              new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorMessageException,
+              String.format(errorMessageFormat, ErrorPhase.COMMITTING, errorMessageException), ErrorType.SYSTEM, true,


I think error type here should be UNKNOWN, how are we sure it is a system issue?

What metric can be used to better distinguish between system / unknown error type ?

Here the job failed on BigQuery "System" since the logs are pulled in from the BigQuery, so the reason will likely be known.

Job can fail on BigQuery due to many reasons like permission issues as well right? So, we need to decide based on error codes & error reason cannot directly say that it is a system error.

Updated to type UNKNOWN

itsankit-google · 2024-11-13T15:40:14Z

src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQueryOutputFormat.java

+              , elapsedTime);
+            throw ErrorUtils.getProgramFailureException(
+              new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorMessage,
+              String.format(errorMessageFormat, ErrorPhase.COMMITTING, errorMessage), ErrorType.SYSTEM, true,


I think error type here should be UNKNOWN, how are we sure it is a system issue?

Yes, UNKNOW is more appropriate here , updated !

itsankit-google

There are multiple exceptions being thrown BigQuerySinkUtils why is that not covered in this PR?

src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQueryOutputFormat.java

psainics · 2024-11-14T08:16:40Z

There are multiple exceptions being thrown BigQuerySinkUtils why is that not covered in this PR?

Wrapped the Io Exception, IllegalArgumentException will be handled by the error details provider !

itsankit-google

Please fix the e2e test failure might be caused by change in error message:
To verify the pipeline is getting failed from GCS Source to GCS Sink On Multiple File having different schemas without connection

itsankit-google · 2024-11-14T11:21:23Z

src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQuerySinkUtils.java

-        throw new IOException(errorMessage.get(), e);
+        throw ErrorUtils.getProgramFailureException(
+          new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), "Bucket already exists", errorMessage.get(),
+          ErrorType.UNKNOWN, true, e);


we can decide on error type here based on status code: see

google-cloud/src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

Line 69 in 601f62b

ErrorUtils.ActionErrorPair pair = ErrorUtils.getActionErrorByStatusCode(statusCode);

Only 409 means bucket already exists for others it means the request failed to create bucket.

updated !

try { GCPUtils.createBucket(storage, bucket, location, cmekKeyName); } catch (StorageException e) { if (e.getCode() != 409) { // A conflict means the bucket already exists // This most likely means multiple stages in the same pipeline are trying to create the same dataset. // Ignore this and move on, since all that matters is that the dataset exists. ErrorUtils.ActionErrorPair pair = ErrorUtils.getActionErrorByStatusCode(e.getCode()); String errorReason = String.format("%s %s %s", e.getCode(), e.getMessage(), pair.getCorrectiveAction()); throw ErrorUtils.getProgramFailureException( new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason, errorMessage.get(), pair.getErrorType(), true, e); } }

itsankit-google · 2024-11-14T11:22:24Z

src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQuerySinkUtils.java

-        throw new IOException(errorMessage.get(), e);
+        throw ErrorUtils.getProgramFailureException(
+          new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), "Dataset already exists", errorMessage.get(),
+          ErrorType.UNKNOWN, true, e);


similar comment here:

google-cloud/src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

Line 69 in 601f62b

ErrorUtils.ActionErrorPair pair = ErrorUtils.getActionErrorByStatusCode(statusCode);

Only 409 means dataset already exists for others it means the request failed to create dataset.

updated !

try { bigQuery.create(builder.build()); } catch (BigQueryException e) { if (e.getCode() != 409) { // A conflict means the dataset already exists (https://cloud.google.com/bigquery/troubleshooting-errors) // This most likely means multiple stages in the same pipeline are trying to create the same dataset. // Ignore this and move on, since all that matters is that the dataset exists. ErrorUtils.ActionErrorPair pair = ErrorUtils.getActionErrorByStatusCode(e.getCode()); String errorReason = String.format("%s %s %s", e.getCode(), e.getMessage(), pair.getCorrectiveAction()); throw ErrorUtils.getProgramFailureException( new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason, errorMessage.get(), pair.getErrorType(), true, e); } }

itsankit-google · 2024-11-14T11:24:26Z

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

+   */
+  private ProgramFailureException getProgramFailureException(IllegalStateException e, ErrorContext errorContext) {
+    String errorMessage = e.getMessage();
+    String subCategory = "IllegalState";


This doesn't sound like an appropriate category. Categories do not contain lower level exception name rather high level generic exception types like access issues

Unless we can point out the exact subCategory, we can omit adding it rather than adding generic stuff in subcategories.

Fair, removed, also removed the String subCategory = "IllegalArgument"

itsankit-google · 2024-11-14T11:27:21Z

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQuerySource.java

+      collector.getOrThrowException();
+    }
+
+


nit remove extra empty line

itsankit-google · 2024-11-14T11:40:35Z

There are multiple exceptions being thrown BigQuerySinkUtils why is that not covered in this PR?

Wrapped the Io Exception, IllegalArgumentException will be handled by the error details provider !

Are we sure that IllegalArgumentException is thrown from methods called from OutputFormat methods? since ErrorDetailsProvider is currently used there only.

itsankit-google · 2024-11-15T04:34:48Z

src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQueryOutputFormat.java

+        throw ErrorUtils.getProgramFailureException(
+          new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorMessage,
+          String.format(errorMessageFormat, ErrorPhase.COMMITTING, errorMessage), ErrorType.SYSTEM, true,
+          new IOException(errorMessage, e));


There is no need to wrap actual exception in Ioexception here now.

Similar comment in all places.

Removed!
I have made changes in a new commit to help with the review, i will squash those before merging.

psainics · 2024-11-15T08:02:43Z

There are multiple exceptions being thrown BigQuerySinkUtils why is that not covered in this PR?

Wrapped the Io Exception, IllegalArgumentException will be handled by the error details provider !

Are we sure that IllegalArgumentException is thrown from methods called from OutputFormat methods? since ErrorDetailsProvider is currently used there only.

The two places that mention IllegalArgumentException is thrown in description on the condition when out schema has more field than BQ Table, but the case is already handled by adding the error in a collector

    for (String field : remainingBQFields) {
      if (bqFields.get(field).getMode() != Field.Mode.NULLABLE) {
        collector.addFailure(String.format("Required Column '%s' is not present in the schema.", field),
                             String.format("Add '%s' to the schema.", field));
      }

is there somewhere i missed it ?

itsankit-google · 2024-11-15T08:11:17Z

There are multiple exceptions being thrown BigQuerySinkUtils why is that not covered in this PR?

Wrapped the Io Exception, IllegalArgumentException will be handled by the error details provider !

Are we sure that IllegalArgumentException is thrown from methods called from OutputFormat methods? since ErrorDetailsProvider is currently used there only.

The two places that mention IllegalArgumentException is thrown in description on the condition when out schema has more field than BQ Table, but the case is already handled by adding the error in a collector
    for (String field : remainingBQFields) {
      if (bqFields.get(field).getMode() != Field.Mode.NULLABLE) {
        collector.addFailure(String.format("Required Column '%s' is not present in the schema.", field),
                             String.format("Add '%s' to the schema.", field));
      }
is there somewhere i missed it ?

sg.

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

itsankit-google · 2024-11-15T08:43:34Z

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

+    String errorMessage = e.getMessage();
+    String errorMessageFormat = "Error occurred in the phase: '%s'. Error message: %s";
+    return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategoryEnum.PLUGIN), errorMessage,
+      String.format(errorMessageFormat, errorContext.getPhase(), errorMessage), ErrorType.SYSTEM, true, e);


dependency might not be true in this case since the exception is thrown from our code.

Got it, updated !

itsankit-google · 2024-11-15T08:44:08Z

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java

+    String errorMessage = e.getMessage();
+    String errorMessageFormat = "Error occurred in the phase: '%s'. Error message: %s";
+    return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategoryEnum.PLUGIN), errorMessage,
+      String.format(errorMessageFormat, errorContext.getPhase(), errorMessage), ErrorType.USER, true, e);


similarly here dependency should not be true unless error is thrown from dependent services client.

Got it, updated !

itsankit-google

couple of minor comments. Otherwise , PR looks good.

Please check once all the e2e tests pass.

psainics added the build Trigger unit test build label Nov 6, 2024

vikasrathee-cs reviewed Nov 7, 2024

View reviewed changes

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQueryAvroToStructuredTransformer.java Outdated Show resolved Hide resolved

vikasrathee-cs reviewed Nov 7, 2024

View reviewed changes

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQueryAvroToStructuredTransformer.java Outdated Show resolved Hide resolved

vikasrathee-cs reviewed Nov 7, 2024

View reviewed changes

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQuerySource.java Show resolved Hide resolved

vikasrathee-cs reviewed Nov 7, 2024

View reviewed changes

src/main/java/io/cdap/plugin/gcp/bigquery/source/BigQuerySourceUtils.java Outdated Show resolved Hide resolved

psainics force-pushed the fem/big-query branch from 3c06ecb to 85d4858 Compare November 8, 2024 04:24

psainics requested a review from vikasrathee-cs November 8, 2024 04:24

psainics changed the title ~~TEST~~ Error details provider - BigQuerySource Wrap known errors Nov 8, 2024

psainics force-pushed the fem/big-query branch from 85d4858 to abf3b12 Compare November 13, 2024 05:01

psainics changed the title ~~Error details provider - BigQuerySource Wrap known errors~~ [PLUGIN-21080] Error details provider - BigQuerySource Wrap known errors Nov 13, 2024

psainics changed the title ~~[PLUGIN-21080] Error details provider - BigQuerySource Wrap known errors~~ [PLUGIN-21080] ErrorDetailsProvider - BigQuery Source/Sink plugin Nov 13, 2024

psainics changed the title ~~[PLUGIN-21080] ErrorDetailsProvider - BigQuery Source/Sink plugin~~ [PLUGIN-1818] ErrorDetailsProvider - BigQuery Source/Sink plugin Nov 13, 2024

psainics force-pushed the fem/big-query branch from abf3b12 to 78192a6 Compare November 13, 2024 05:56

psainics self-assigned this Nov 13, 2024

psainics marked this pull request as ready for review November 13, 2024 06:57

psainics requested a review from itsankit-google November 13, 2024 07:35

psainics force-pushed the fem/big-query branch from 78192a6 to 66e9507 Compare November 13, 2024 07:37

itsankit-google reviewed Nov 13, 2024

View reviewed changes

psainics force-pushed the fem/big-query branch from 66e9507 to 5c897be Compare November 13, 2024 08:28

psainics requested a review from itsankit-google November 13, 2024 08:31

psainics force-pushed the fem/big-query branch 3 times, most recently from 60c479d to 9d501d3 Compare November 13, 2024 10:33

itsankit-google reviewed Nov 13, 2024

View reviewed changes

src/main/java/io/cdap/plugin/gcp/bigquery/sink/BigQueryOutputFormat.java Show resolved Hide resolved

psainics force-pushed the fem/big-query branch from 9d501d3 to 1e8c486 Compare November 14, 2024 05:00

psainics force-pushed the fem/big-query branch from 1e8c486 to 6d65f15 Compare November 14, 2024 08:16

psainics requested a review from itsankit-google November 14, 2024 08:16

itsankit-google requested changes Nov 14, 2024

View reviewed changes

psainics force-pushed the fem/big-query branch 2 times, most recently from 170fc3b to 9e79da9 Compare November 14, 2024 09:27

itsankit-google reviewed Nov 14, 2024

View reviewed changes

psainics force-pushed the fem/big-query branch from 9e79da9 to efeef28 Compare November 14, 2024 11:44

itsankit-google reviewed Nov 15, 2024

View reviewed changes

src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java Show resolved Hide resolved

itsankit-google reviewed Nov 15, 2024

View reviewed changes

Add BigQuery Error Details Provider

29c21ca

psainics force-pushed the fem/big-query branch from 550f350 to 29c21ca Compare November 15, 2024 08:52

[PLUGIN-1818] ErrorDetailsProvider - BigQuery Source/Sink plugin #1458

Are you sure you want to change the base?

[PLUGIN-1818] ErrorDetailsProvider - BigQuery Source/Sink plugin #1458

Conversation

psainics commented Nov 6, 2024 • edited Loading

ErrorDetailsProvider - BigQuery Source/Sink plugin

Description

Code change

Unit Tests

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

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

itsankit-google left a comment

Choose a reason for hiding this comment

psainics commented Nov 14, 2024

itsankit-google left a comment • edited Loading

Choose a reason for hiding this comment

itsankit-google Nov 14, 2024 • edited Loading

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

itsankit-google commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psainics commented Nov 15, 2024

itsankit-google commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsankit-google Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsankit-google left a comment

Choose a reason for hiding this comment

psainics commented Nov 6, 2024 •

edited

Loading

itsankit-google left a comment •

edited

Loading

itsankit-google Nov 14, 2024 •

edited

Loading

itsankit-google Nov 15, 2024 •

edited

Loading