PLUGIN-1936: Read data in a streaming manner #131
Conversation
| } | ||
|
|
||
| /** | ||
| * Check whether the result is empty or not. |
There was a problem hiding this comment.
Also explain how are we checking is result is empty
| return GSON.fromJson(ja, type); | ||
| } | ||
|
|
||
| public List<Map<String, String>> parseResponseToResultListOfMap(InputStream in) throws ServiceNowAPIException { |
There was a problem hiding this comment.
Why are we creating a List of map? Instead we can directly read it as a JsonObject, something like:
gson.fromJson(reader, ServiceNowRecordObject.class);
| String startDate, String endDate, int offset, | ||
| int limit) throws ServiceNowAPIException { | ||
| final List<Map<String, String>> results = new ArrayList<>(); | ||
| //final List<Map<String, String>> results = new ArrayList<>(); |
| final RestAPIResponse[] restAPIResponse = new RestAPIResponse[1]; | ||
| Callable<Boolean> fetchRecords = () -> { | ||
| results.addAll(fetchTableRecords(tableName, valueType, startDate, endDate, offset, limit)); | ||
| // results.addAll(fetchTableRecords(tableName, valueType, startDate, endDate, offset, limit)); |
| Callable<Boolean> fetchRecords = () -> { | ||
| results.addAll(fetchTableRecords(tableName, valueType, startDate, endDate, offset, limit)); | ||
| // results.addAll(fetchTableRecords(tableName, valueType, startDate, endDate, offset, limit)); | ||
| restAPIResponse[0] = fetchTableRecords(tableName, valueType, startDate, endDate, offset, limit); |
There was a problem hiding this comment.
can you please explain what are we trying to do here? why restAPIResponse is an array
| private final ServiceNowMultiSourceConfig multiSourcePluginConf; | ||
| private ServiceNowTableAPIClientImpl restApi; | ||
| private final Gson gson = new Gson(); | ||
| private final Type mapType = new TypeToken<Map<String, String>>() { }.getType(); |
There was a problem hiding this comment.
should use an object instead of map
| return false; | ||
| } | ||
| this.jsonReader = new JsonReader(new InputStreamReader(in, StandardCharsets.UTF_8)); | ||
| this.jsonReader.setLenient(true); |
| JsonToken top; | ||
| try { | ||
| top = jsonReader.peek(); | ||
| LOG.info("Peeking JSON token for table {}: {}", tableName, top); |
There was a problem hiding this comment.
please remove, it should be a debug log
| } | ||
|
|
||
| public void closeCurrentPage() { | ||
| LOG.info("Closing current page for table {}", tableName); |
| return restAPIResponse; | ||
| } | ||
|
|
||
| private boolean openNextPage() throws IOException, ServiceNowAPIException { |
There was a problem hiding this comment.
looks redundant, as same function is present in src/main/java/io/cdap/plugin/servicenow/source/ServiceNowMultiRecordReader.java
can we abstract these out in a common file. Same comment for nextKeyValue()
3 participants
PLUGIN-1936
Issue : OutOfMemory (OOM) Errors seen in the ServiceNow Connector frequently when transfering large number of records
RCA: The entire response is converted into string and the JSON is parsed in memory leading to OOM issues.
Proposed Fix: Read the records incrementally i.e. one by one in a streaming manner.
Changes :
prepareResponseWithBodyAndStream:in theRESTAPIResponseclass to store a reusable InputStream in theRESTAPIResponseobject. Also changed the logic for preparing theresponseBodyin theStringformatopenNextPageto initialise theGson'sJsonReaderand read theresultarray in the response andcloseCurrentPageto close theJsonReadernextKeyValue(): Refactored the existing logic to stream one record at a time.fetchData()fetchTableRecordsRetryableMode()fetchTableRecords()