[PLUGIN-1848] Error Management for File plugin Source/Sink by Amit-CloudSufi · Pull Request #1912 · cdapio/hydrator-plugins

Amit-CloudSufi · 2025-01-17T12:52:55Z

https://cdap.atlassian.net/browse/PLUGIN-1848

[
  {
    "stageName": "File",
    "errorCategory": "Plugin-'File'",
    "errorReason": "Input path /Users/pusaini/wsf/cdap/env/group_by_test_data_1/data.csv does not exist",
    "errorMessage": "Input path /Users/pusaini/wsf/cdap/env/group_by_test_data_1/data.csv does not exist",
    "errorType": "USER",
    "dependency": "false"
  },
  {
    "errorCategory": "Plugin",
    "errorReason": "Input path /Users/pusaini/wsf/cdap/env/group_by_test_data_1/data.csv does not exist",
    "errorMessage": "Input path /Users/pusaini/wsf/cdap/env/group_by_test_data_1/data.csv does not exist",
    "errorType": "USER",
    "dependency": "false"
  }
]

itsankit-google · 2025-01-22T07:05:16Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSource.java

      throw new IllegalArgumentException(errorMessage, e);
+    } catch (InstantiationException e) {
+      context.getFailureCollector().addFailure(
+        String.format("Could not find the '%s' input format.", fileFormat), null)


Could not load the input format %s

itsankit-google · 2025-01-22T07:08:45Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSource.java

+    try {
+      job = JobUtils.createInstance();
+    } catch (IOException e) {
+      collector.addFailure("Failed to create job instance.", e.getMessage())


e.getMessage() is not a corrective action:

collector.addFailure(String.format("Failed to create job instance, %s: %s", e.getClass().getName(), e.getMessage()), null)

This comment applies to whole PR.

itsankit-google · 2025-01-22T07:10:03Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSource.java

+        schema = schemaDetector.detectSchema(config.getPath(context), pattern,
+                                             formatContext, getFileSystemProperties(null));
+      } catch (IOException e) {
+        collector.addFailure("Error when trying to detect schema: " + e.getMessage(), null)


collector.addFailure(String.format("Failed to auto-detect schema, %s: %s", e.getClass().getName(), e.getMessage()), null)

itsankit-google · 2025-01-22T07:12:38Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSource.java

@@ -243,7 +284,14 @@ protected ValidatingInputFormat getInputFormatForRun(BatchSourceContext context)
          + "or re-create the pipeline with all required properties.",
        fileFormat, properties);
      throw new IllegalArgumentException(errorMessage, e);


please convert IllegalArgumentException to ProgramFailureException.

itsankit-google · 2025-01-22T07:21:16Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSink.java

    Map<String, String> outputProperties = new HashMap<>(validatingOutputFormat.getOutputFormatConfiguration());
    outputProperties.putAll(getFileSystemProperties(context));
    outputProperties.put(FileOutputFormat.OUTDIR, getOutputDir(context));
+    context.setErrorDetailsProvider(new ErrorDetailsProviderSpec(getErrorDetailsProviderClassName()));


since GCSBatchSink does not override getErrorDetailsProviderClassName(), this will get set to null, see here: https://github.com/data-integrations/google-cloud/blob/77e0ac8a386d12b7b55f7e9fab2b1adaa88d9a01/src/main/java/io/cdap/plugin/gcp/gcs/sink/GCSBatchSink.java#L168-L172

Please check getErrorDetailsProviderClassName is not null or empty and then only set it.

similar for AbstractFileBatchSource

See data-integrations/database-plugins#540

itsankit-google · 2025-01-22T07:22:31Z

core-plugins/src/main/java/io/cdap/plugin/batch/sink/FileSink.java

        return GSON.fromJson(fileSystemProperties, MAP_TYPE);
      } catch (JsonSyntaxException e) {
-        throw new IllegalArgumentException("Unable to parse filesystem properties: " + e.getMessage(), e);
+        throw new IllegalArgumentException(


throw ProgramFailureException instead

core-plugins/src/main/java/io/cdap/plugin/batch/source/FileErrorDetailsProvider.java

core-plugins/src/main/java/io/cdap/plugin/common/HydratorErrorDetailsProvider.java

itsankit-google · 2025-01-22T07:23:39Z

core-plugins/src/main/java/io/cdap/plugin/common/HydratorErrorDetailsProvider.java

+import javax.annotation.Nullable;
+
+/**
+ * Error details provided for the Snowflake


Snowflake?

itsankit-google · 2025-01-22T07:25:04Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSource.java

+    try {
+      fileStatus = pathFileSystem.globStatus(path);
+    } catch (IOException e) {
+      collector.addFailure("Failed to get file status for path " + path, e.getMessage())


use String.format everywhere.

e.getMessage(), is not a correctiveAction

also preserve error class name,

%s: %s, e.getClass().getName(), e.getMessage()

itsankit-google · 2025-01-22T07:26:31Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSource.java

        fileFormat, properties);
      throw new IllegalArgumentException(errorMessage, e);
+    } catch (InstantiationException e) {
+      context.getFailureCollector().addFailure(


create a local variable collector = context.getFailureCollector() and use at other places in same method.

itsankit-google · 2025-01-22T07:27:32Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSource.java

+        String.format("Could not find the '%s' input format.", fileFormat), null)
+        .withPluginNotFound(fileFormat, fileFormat, ValidatingInputFormat.PLUGIN_TYPE)
+        .withStacktrace(e.getStackTrace());
+      context.getFailureCollector().getOrThrowException();


throw collector.getOrThrowException();

This way you wouldn't need to return null at the end.

itsankit-google · 2025-01-23T08:45:36Z

core-plugins/src/main/java/io/cdap/plugin/common/HydratorErrorDetailsProvider.java

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


private ProgramFailureException getProgramFailureException(IllegalArgumentException exception, ErrorContext errorContext,

itsankit-google · 2025-01-23T08:52:03Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSink.java

+      context.getFailureCollector().getOrThrowException();
    }
+    return null;


same comment here

itsankit-google · 2025-01-23T09:00:09Z

format-common/src/main/java/io/cdap/plugin/format/plugin/AbstractFileSink.java

+              + "were invalid when the pipeline was deployed. Set the format to a "
+              + "different value, or re-create the pipeline with all required properties.",
+          fileFormat, properties);
      throw new IllegalArgumentException(errorMessage, e);


similar comment to throw ProgramFailureException

itsankit-google · 2025-01-23T09:00:52Z

core-plugins/src/main/java/io/cdap/plugin/batch/sink/FileSink.java

+            e.getMessage());
+        throw ErrorUtils.getProgramFailureException(
+            new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorMessage, errorMessage,
+            ErrorType.SYSTEM, false, null);


why SYSTEM?

itsankit-google · 2025-01-23T09:06:37Z

core-plugins/src/main/java/io/cdap/plugin/batch/sink/FileSink.java

      } catch (JsonSyntaxException e) {
-        throw new IllegalArgumentException("Unable to parse filesystem properties: " + e.getMessage(), e);
+        String errorMessage = String.format(
+            "Failed to parse filesystem properties %s with message: %s", fileSystemProperties,


can we please follow the same format everywhere? %s : %s", e.getClass().getName(), e.getMessage()

itsankit-google · 2025-01-23T11:54:47Z

core-plugins/src/main/java/io/cdap/plugin/batch/sink/FileSink.java

      } catch (JsonSyntaxException e) {
-        throw new IllegalArgumentException("Unable to parse filesystem properties: " + e.getMessage(), e);
+        String errorMessage = String.format(
+            "Failed to parse filesystem properties %s with message: %s: %s", fileSystemProperties,


"Failed to parse filesystem properties %s, %s: %s"

itsankit-google · 2025-01-23T11:55:23Z

core-plugins/src/main/java/io/cdap/plugin/batch/source/FileSourceConfig.java

      return GSON.fromJson(fileSystemProperties, MAP_STRING_STRING_TYPE);
    } catch (Exception e) {
-      throw new IllegalArgumentException("Unable to parse filesystem properties: " + e.getMessage(), e);
+      throw new IllegalArgumentException(String.format("Unable to parse filesystem properties: %s", e.getMessage()), e);


"Unable to parse filesystem properties, %s: %s", e.getClass().getName(), e.getMessage()

itsankit-google · 2025-01-23T12:07:46Z

Please add a JIRA in the title

psainics force-pushed the filePLuginErrorMang branch from 1e92b0c to 4bb60cf Compare January 20, 2025 04:12

psainics added the build Trigger unit test build label Jan 20, 2025

Amit-CloudSufi force-pushed the filePLuginErrorMang branch from a2cd069 to d25f116 Compare January 20, 2025 07:52

psainics requested a review from itsankit-google January 21, 2025 08:06

Amit-CloudSufi force-pushed the filePLuginErrorMang branch 3 times, most recently from 3d77a35 to a7b066a Compare January 22, 2025 02:31

psainics marked this pull request as ready for review January 22, 2025 06:20

itsankit-google reviewed Jan 22, 2025

View reviewed changes

core-plugins/src/main/java/io/cdap/plugin/batch/source/FileErrorDetailsProvider.java Show resolved Hide resolved

itsankit-google reviewed Jan 22, 2025

View reviewed changes

core-plugins/src/main/java/io/cdap/plugin/common/HydratorErrorDetailsProvider.java Show resolved Hide resolved

itsankit-google reviewed Jan 22, 2025

View reviewed changes

Amit-CloudSufi requested review from itsankit-google and psainics January 23, 2025 07:48

psainics approved these changes Jan 23, 2025

View reviewed changes

psainics force-pushed the filePLuginErrorMang branch from 875e48f to 8923a7d Compare January 23, 2025 08:13

itsankit-google reviewed Jan 23, 2025

View reviewed changes

Amit-CloudSufi force-pushed the filePLuginErrorMang branch from 8923a7d to bc96350 Compare January 23, 2025 09:02

itsankit-google reviewed Jan 23, 2025

View reviewed changes

Amit-CloudSufi force-pushed the filePLuginErrorMang branch from 331d51b to 6fb4e61 Compare January 23, 2025 09:27

Amit-CloudSufi requested a review from itsankit-google January 23, 2025 09:28

Error Management for File plugin Source/Sink

6f4b5d8

Amit-CloudSufi force-pushed the filePLuginErrorMang branch from 6fb4e61 to 6f4b5d8 Compare January 23, 2025 09:28

itsankit-google reviewed Jan 23, 2025

View reviewed changes

itsankit-google approved these changes Jan 23, 2025

View reviewed changes

itsankit-google changed the title ~~Error Management for File plugin Source/Sink~~ [PLUGIN-1848] Error Management for File plugin Source/Sink Jan 23, 2025

psainics merged commit 7f24625 into cdapio:develop Jan 24, 2025
5 checks passed

psainics deleted the filePLuginErrorMang branch January 24, 2025 06:59

itsankit-google mentioned this pull request Jan 24, 2025

[PLUGIN-1848] File error message update #1915

Merged

psainics mentioned this pull request Jan 24, 2025

[🍒][PLUGIN-1848] Error Management for File plugin Source/Sink #1916

Merged

psainics assigned Amit-CloudSufi Jan 24, 2025

psainics mentioned this pull request Jan 24, 2025

[🍒][PLUGIN-1848] File error message update #1917

Closed

Conversation

Amit-CloudSufi commented Jan 17, 2025 • edited by psainics Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsankit-google Jan 22, 2025 • 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!

Uh oh!

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!

itsankit-google Jan 23, 2025 • 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!

itsankit-google Jan 23, 2025 • 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!

itsankit-google commented Jan 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Amit-CloudSufi commented Jan 17, 2025 •

edited by psainics

Loading

itsankit-google Jan 22, 2025 •

edited

Loading

itsankit-google Jan 23, 2025 •

edited

Loading

itsankit-google Jan 23, 2025 •

edited

Loading