New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PLUGIN-1826] Error Management HTTP Source and Sink and fix sonar issues #180

Open

Amit-CloudSufi wants to merge 9 commits into data-integrations:develop from cloudsufi:httpPluginErrorMang

Amit-CloudSufi commented Dec 6, 2024 •

edited

Loading

Jira : https://cdap.atlassian.net/browse/PLUGIN-1826

This comment was marked as resolved.

Sign in to view

Amit-CloudSufi force-pushed the httpPluginErrorMang branch from 4cad117 to fac0c1a Compare

December 9, 2024 07:54

psainics assigned Amit-CloudSufi

psainics added the build label

Amit-CloudSufi changed the title ~~Added HTTP error detail provider and refactored HTTP-sink and source package to handle error provider and fix sonar issues~~ [PLUGIN-1826] Error Management HTTP Source and Sink and fix sonar issues

Amit-CloudSufi force-pushed the httpPluginErrorMang branch 2 times, most recently from e612cd0 to d273336 Compare

December 12, 2024 11:59

Amit-CloudSufi force-pushed the httpPluginErrorMang branch from 0af6ccc to a41b2f6 Compare

December 18, 2024 11:03

psainics added build and removed build labels


          Added HTTP error details provider, refactored HTTP-sink and source pa…

2e321fa

…ckage to handle error provider, fix sonar issues and added Validation error for linear retry duration

Amit-CloudSufi force-pushed the httpPluginErrorMang branch from 4f1d5f5 to 2e321fa Compare

December 18, 2024 11:33

psainics requested a review from itsankit-google

December 18, 2024 11:58

itsankit-google requested changes

View reviewed changes

Member

itsankit-google left a comment

please fix unit tests

Amit-CloudSufi requested a review from itsankit-google

December 20, 2024 09:17

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/common/pagination/BaseHttpPaginationIterator.java Outdated

+                              errorMessage, String.format("Error message: %s", errorMessage),
+                              ErrorUtils.getActionErrorByStatusCode(httpStatusCode).getErrorType(),
+                              true, ErrorCodeType.HTTP, String.valueOf(httpStatusCode),
+                              "https://developer.mozilla.org/en-US/docs/Web/HTTP/Status", new IllegalStateException(errorMessage));

Member

itsankit-google Dec 20, 2024

we should use official documentations link: https://datatracker.ietf.org/doc/html/rfc7231#section-6

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/common/pagination/BaseHttpPaginationIterator.java Outdated

+                      String errorMessage = String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
+                              nextPageUrl, httpStatusCode, response.getBody());
+                      throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                              errorMessage, String.format("Error message: %s", errorMessage),

Member

itsankit-google Dec 20, 2024

don't do redundant Error message: %s

Author

Amit-CloudSufi Dec 23, 2024

removed from all files

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/common/pagination/BaseHttpPaginationIterator.java Outdated

@@ @@ -122,8 +130,13 @@ protected BasePage getNextPage() throws IOException { @@
                     case SUCCESS:
                       break;
                     case STOP:
-                      throw new IllegalStateException(String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
-                                                                    nextPageUrl, httpStatusCode, response.getBody()));
+                      String errorMessage = String.format("Fetching from url '%s' returned status code '%d' and body '%s'",

Member

itsankit-google Dec 20, 2024

errorReason

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

+                    throw new IllegalArgumentException("Invalid URL: " + configURL, e);
+                  } catch (IOException e) {
+                    LOG.warn("Error making {} request to URL {}.", config.getMethod(), config.getUrl());
+                    String errorMessage = "Unable to make request. ";

Member

itsankit-google Dec 20, 2024 •

edited

Loading

this is not an actional error reason

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

+                    LOG.warn("Error making {} request to URL {}.", config.getMethod(), config.getUrl());
+                    String errorMessage = "Unable to make request. ";
+                    throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                            errorMessage, String.format("Error message: %s", errorMessage), ErrorType.UNKNOWN, true, e);

Member

itsankit-google Dec 20, 2024

similar comment here

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

-                  if (!shouldRetry) {
-                    messageBuffer.clear();
-                    retryCount = 0;
+                    String errorMessage = String.format("Unable to execute HTTP request to %s.", url);

Member

itsankit-google Dec 20, 2024

errorReason

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

-                    retryCount = 0;
+                    String errorMessage = String.format("Unable to execute HTTP request to %s.", url);
+                    throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                            errorMessage, String.format("Error message: %s", errorMessage), ErrorType.SYSTEM, true, e);

Member

itsankit-google Dec 20, 2024

similar comment here

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

+                    String errorMessage = String.format("Unable to execute HTTP request to %s.", url);
+                    throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                            errorMessage, String.format("Error message: %s", errorMessage), ErrorType.SYSTEM, true, e);

Member

itsankit-google Dec 20, 2024

nit: remove empty line

Author

Amit-CloudSufi Dec 23, 2024

removed

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

+                  } catch (Exception e) {
+                    String errorMessage = String.format("Unexpected error occurred while executing HTTP request to URL: %s", url);
+                    throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                            errorMessage, String.format("Error message: %s", errorMessage), ErrorType.UNKNOWN, true, e);

Member

itsankit-google Dec 20, 2024

similar comment here

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

                     URI uri = URI.create(pageUriStr);
                     AuthScope authScope = new AuthScope(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()));
                     credentialsProvider.setCredentials(authScope,
-                      new UsernamePasswordCredentials(config.getUsername(), config.getPassword()));
+                            new UsernamePasswordCredentials(config.getUsername(), config.getPassword()));

Member

itsankit-google Dec 20, 2024

revert unintended changes.

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

Comment on lines 274 to 275

		new UsernamePasswordCredentials(
		config.getProxyUsername(), config.getProxyPassword()));

Member

itsankit-google Dec 20, 2024

revert unintended changes

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

		}

Member

itsankit-google Dec 20, 2024

nit: remove empty line

Author

Amit-CloudSufi Dec 23, 2024

removed

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

                 private List<PlaceholderBean> getPlaceholderListFromURL() {
                   List<PlaceholderBean> placeholderList = new ArrayList<>();
                   if (!(config.getMethod().equals(REQUEST_METHOD_PUT) || config.getMethod().equals(REQUEST_METHOD_PATCH) ||
-                    config.getMethod().equals(REQUEST_METHOD_DELETE))) {
+                          config.getMethod().equals(REQUEST_METHOD_DELETE))) {

Member

itsankit-google Dec 20, 2024

revert unintended changes

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

Comment on lines 366 to 370

    
                            .await().with()

                            .pollInterval(pollInterval)

                            .pollDelay(config.getWaitTimeBetweenPages(), TimeUnit.MILLISECONDS)

                            .timeout(config.getMaxRetryDuration(), TimeUnit.SECONDS)

                            .until(this::executeHTTPServiceAndCheckStatusCode);

Member

itsankit-google Dec 20, 2024

revert unintended changes

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

+                    String errorMessage = "Error while executing http request for remaining input messages" +
+                            " after the batch execution.";
+                    throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                            errorMessage, String.format("Error message: %s", errorMessage), ErrorType.UNKNOWN, true,

Member

itsankit-google Dec 20, 2024

similar comment here

Member

itsankit-google Dec 20, 2024

Also don't we have status code information here?

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

                   } catch (Exception e) {
-                    throw new RuntimeException("Error while executing http request for remaining input messages " +
-                                                 "after the batch execution. " + e);
+                    String errorMessage = "Error while executing http request for remaining input messages" +

Member

itsankit-google Dec 20, 2024

can we add http request information here?

Author

Amit-CloudSufi Dec 23, 2024

added

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

+                              errorMessage, String.format("Error message: %s", errorMessage),
+                              ErrorUtils.getActionErrorByStatusCode(httpStatusCode).getErrorType(),
+                              true, ErrorCodeType.HTTP, String.valueOf(httpStatusCode),
+                              "https://developer.mozilla.org/en-US/docs/Web/HTTP/Status", new IllegalStateException(errorMessage));

Member

itsankit-google Dec 20, 2024

similar comment here about documentation

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java Outdated

+                      String errorMessage = String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
+                              config.getUrl(), httpStatusCode, httpResponseBody);
+                      throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                              errorMessage, String.format("Error message: %s", errorMessage),

Member

itsankit-google Dec 20, 2024

similar comment here

Author

Amit-CloudSufi Dec 23, 2024

updated

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPRecordWriter.java

                     case SKIP:
                     case SEND:
                       LOG.warn(String.format("Fetching from url '%s' returned status code '%d' and body '%s'",
-                                             config.getUrl(), httpStatusCode, httpResponseBody));
+                              config.getUrl(), httpStatusCode, httpResponseBody));

Member

itsankit-google Dec 20, 2024

revert unintended changes

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/sink/batch/HTTPSink.java Outdated

		@@ -1,3 +1,4 @@

Member

itsankit-google Dec 20, 2024

nit: remove empty line

Author

Amit-CloudSufi Dec 23, 2024

removed


          pr comments fixes

7adc545

Amit-CloudSufi requested a review from itsankit-google

December 23, 2024 10:53

psainics requested a review from sahusanket

December 24, 2024 05:01

sahusanket reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/common/HttpErrorDetailsProvider.java Outdated

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

sahusanket Dec 27, 2024

I see All the 4 methods being Overloaded , have only 1 param changing i.e. ERROR TYPE.

Only 1 method should suffice :

  private ProgramFailureException getProgramFailureException(Exception e, ErrorContext errorContext, ErrorType errorType) {
    String errorMessage = e.getMessage();
    return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
      errorMessage,
      String.format(ERROR_MESSAGE_FORMAT, errorContext.getPhase(), errorMessage), errorType, false, e);
  }

Member

itsankit-google Dec 30, 2024

+1

Author

Amit-CloudSufi Dec 30, 2024

updated

Amit-CloudSufi and others added 6 commits

December 30, 2024 15:54


          refactor code

f9ac6ed


          Remove ZIP File

3bf8c05


          Revert formatting

2625daa


          Revert formatting 2

c9b70e3


          Fix Unit Tests

4aa8ee7


          Merge branch 'develop' into httpPluginErrorMang

72800cd

itsankit-google reviewed

View reviewed changes

src/main/java/io/cdap/plugin/http/common/BaseHttpConfig.java

@@ @@ -28,6 +28,7 @@ @@
               import io.cdap.plugin.http.common.http.OAuthUtil;
               import java.io.File;
+              import java.util.Objects;

Member

itsankit-google Jan 8, 2025

is this used anywhere?

src/main/java/io/cdap/plugin/http/common/pagination/BaseHttpPaginationIterator.java

@@ @@ -26,6 +30,7 @@ @@
               import io.cdap.plugin.http.common.pagination.state.PaginationIteratorState;
               import io.cdap.plugin.http.common.pagination.state.UrlPaginationIteratorState;
               import io.cdap.plugin.http.source.common.BaseHttpSourceConfig;
+              import okhttp3.internal.http2.ErrorCode;

Member

itsankit-google Jan 8, 2025

where is this used?

src/main/java/io/cdap/plugin/http/common/pagination/BaseHttpPaginationIterator.java

Comment on lines +44 to +49

+              import java.util.Arrays;
               import java.util.Iterator;
+              import java.util.List;
               import java.util.NoSuchElementException;
               import java.util.concurrent.TimeUnit;
+              import java.util.stream.Collectors;

Member

itsankit-google Jan 8, 2025

where are these new imports used?

src/main/java/io/cdap/plugin/http/sink/batch/HTTPOutputFormat.java

+                    inputSchema = Schema.parseJson(hConf.get(INPUT_SCHEMA_KEY));
+                    return new HTTPRecordWriter(config, inputSchema);
+                  } catch (IOException e) {
+                    throw new IllegalStateException("Unable to parse the input schema. Reason: " + e.getMessage(), e);

Member

itsankit-google Jan 8, 2025

String.format("Failed to parse the input schema with message: %s", e.getMessage())

src/main/java/io/cdap/plugin/http/common/pagination/BaseHttpPaginationIterator.java

+                      String errorMessage = String.format("Retry failed! Unable to read new page and execute request. " +
+                                      "Fetching from '%s' returned http error status code '%s'. For more details, see %s",
+                              config.getUrl(), httpStatusCode, supportedDocUrl);
+                      String errorReason = "Retry failed. Unable to read new page.";

Member

itsankit-google Jan 8, 2025

ErrorUtils.ActionErrorPair pair = ErrorUtils.getActionErrorByStatusCode(statusCode);
String errorReason = String.format("Unable to read new page: %s %s. %s", e.getStatusCode(), e.getStatusMessage(), pair.getCorrectiveAction());

This comment applies to whole PR.

src/main/java/io/cdap/plugin/http/sink/batch/MessageBuffer.java

-                      sb.append("&");
+                  StringBuilder sb = new StringBuilder();
+                  if (input != null && input.getSchema() != null) {
+                    for (Schema.Field field : Objects.requireNonNull(input.getSchema().getFields())) {

Member

itsankit-google Jan 8, 2025

same comment here

src/main/java/io/cdap/plugin/http/source/batch/HttpBatchSource.java

-                    Preconditions.checkNotNull(schema.getFields()).stream()
-                      .map(Schema.Field::getName)
-                      .collect(Collectors.toList()));
+                  List<String> getNameList = Objects.nonNull(schema) ? Preconditions.checkNotNull(schema.getFields()).stream()

Member

itsankit-google Jan 8, 2025

please don't do direct Objects.nonNull instead move this code inside try & catch NPE and then throw ProgramFailureException with details.

src/main/java/io/cdap/plugin/http/source/batch/HttpBatchSourceConfig.java

Comment on lines +93 to +98

+                    } catch (IOException e) {
+                      String errorMessage = String.format("Unable to validate OAuth and process the request. " +
+                              "Validation failed with reason %s.", e.getMessage());
+                      String errorReason = "Unable to validate OAuth and process the request.";
+                      throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
+                              errorReason, errorMessage, ErrorType.UNKNOWN, true, e);

Member

itsankit-google Jan 8, 2025

if this is called in configurePipeline please use failureCollector instead.

This comment applies to whole PR.

src/main/java/io/cdap/plugin/http/source/common/BaseHttpSourceConfig.java

@@ @@ -511,7 +512,7 @@ public Schema getSchema() { @@
                     return Strings.isNullOrEmpty(schema) ? null : Schema.parseJson(schema);
                   } catch (IOException e) {
                     throw new InvalidConfigPropertyException("Unable to parse output schema: " +
-                                                               schema, e, PROPERTY_SCHEMA);
+                            schema, e, PROPERTY_SCHEMA);

Member

itsankit-google Jan 8, 2025

remove unintended checkstyle changes.

This comment applies to whole PR.

src/main/java/io/cdap/plugin/http/source/common/BaseHttpSourceConfig.java

    
            @@ -545,7 +546,7 @@ public Map<String, String> getFullFieldsMapping() {
          
                  Map<String, String> result = new HashMap<>();

                  if (!Strings.isNullOrEmpty(schema)) {

                    for (Schema.Field field : getSchema().getFields()) {

                    for (Schema.Field field : Objects.requireNonNull(Objects.requireNonNull(getSchema()).getFields())) {

Member

itsankit-google Jan 8, 2025

same comment regarding null checks.


          Refactor

49ef3a6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels