Added HTTP error detail provider and refactored HTTP-sink and source package to handle error provider and fix sonar issues #15
Conversation
…handle error provider and fix sonar issues
Amit-CloudSufi
changed the title
| inputSchema = Schema.parseJson(hConf.get(INPUT_SCHEMA_KEY)); | ||
| return new HTTPRecordWriter(config, inputSchema); | ||
| } catch (IOException e) { | ||
| String errorMessage = "Unable to parse and write the record"; |
There was a problem hiding this comment.
error Message should be unable to parse the input schema.
| } catch (IOException e) { | ||
| String errorMessage = "Unable to parse and write the record"; | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorMessage, e.getMessage(), ErrorType.UNKNOWN, true, new IOException(errorMessage)); |
There was a problem hiding this comment.
We can wrap this IOException in IllegalState as this is because somehow schema becomes corrupt in the conf.
psainics
assigned
Amit-CloudSufi and unassigned
vikasrathee-cs,
psainics and
Amit-CloudSufi
| if (config.getMethod().equals(REQUEST_METHOD_PUT) || config.getMethod().equals(REQUEST_METHOD_PATCH) || | ||
| config.getMethod().equals(REQUEST_METHOD_DELETE) | ||
| && !placeHolderList.isEmpty()) { | ||
| && !placeHolderList.isEmpty()) { |
There was a problem hiding this comment.
revert the changes due to indentation as it makes things confusing for the reviewer. In actions on save, reformat the file only where you are making changes
| } | ||
|
|
||
| // Use try-with-resources to ensure response is closed | ||
| try (CloseableHttpResponse response = executeHttpRequest(httpClient, url)) { |
There was a problem hiding this comment.
keep a single try with resources block for httpClient and respose
| if (inputSchema == null) { | ||
| fields = Collections.emptyList(); | ||
| } else { | ||
| assert inputSchema.getFields() != null; |
There was a problem hiding this comment.
are you catching the assertion error somewhere?
if not, you can go ahead with old format, CDF has some validations for schema which will check it.
There was a problem hiding this comment.
handled using java 8, removed assert
| collector.addFailure(errorMessage, "Please ensure that correct credentials are provided."); | ||
| } | ||
| } | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
do we need catch block for HttpHostConnectException which was there previously
| throw new RuntimeException("Failed to read line from http page buffer", e); | ||
| String errorMessage = "Unable to read line from http page buffer"; | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorMessage, e.getMessage(), ErrorType.UNKNOWN, true, new IOException(errorMessage)); |
There was a problem hiding this comment.
why are we creating a new exception when we have instance of IOException here.
| public static final TestConfiguration CONFIG = | ||
| new TestConfiguration(Constants.Explore.EXPLORE_ENABLED, false, | ||
| Constants.AppFabric.SPARK_COMPAT, Compat.SPARK_COMPAT); | ||
| new TestConfiguration(EXPLORE_ENABLED, false, Constants.AppFabric.SPARK_COMPAT, Compat.SPARK_COMPAT); |
There was a problem hiding this comment.
why this change is required? we dont use these classes for test anymore.
|
The Bump PR is merged, please rebase again with develop, there are conflicts 😿 |
|
Not need anymore, raise a new PR - #16. |
httpErrorManagement