-
Notifications
You must be signed in to change notification settings - Fork 11
[PLUGIN-1902] Changes done for schema backward compatibility along with metadata API addition for DTS #110
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.
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
Conversation
b24f5ed to
fa9c399
Compare
4ac51bd to
a69805b
Compare
a69805b to
a3b768a
Compare
a3b768a to
e5f72f3
Compare
src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java
Outdated
Show resolved
Hide resolved
|
Any reason why If no strong reason then please refactor it to |
src/main/java/io/cdap/plugin/servicenow/sink/model/ServiceNowSchemaField.java
Outdated
Show resolved
Hide resolved
|
I see a lot of Old code that was used for SCHEMA API , is not used to MetaData APi instead ? Is that expected? and why ? |
Previously Schema API was removed and same classes were used for Metadata API, later due to backward compatibility we had to add schema api again. Renamed the classes again to provide the clarity on which class getting used by which api. |
|
|
||
| if (schemaResponse.getResult() == null && schemaResponse.getResult().getColumns().isEmpty()) { | ||
| throw new RuntimeException("Error - Schema Response does not contain any result"); | ||
| if (schemaType == SchemaType.METADATA_API_BASED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of DTS it directly calls this method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
| SchemaType schemaType) { | ||
| if (isSchemaRequired) { | ||
| this.setUrl(String.format(METADATA_API_URL_TEMPLATE, instanceBaseUrl, tableName)); | ||
| if (SchemaType.METADATA_API_BASED == schemaType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please simplify this
move this.setUrl(String.format(TABLE_API_URL_TEMPLATE, instanceBaseUrl, tableName)); before the if block as the default
| public ServiceNowTableAPIClientImpl(ServiceNowConnectorConfig conf) { | ||
| public ServiceNowTableAPIClientImpl(ServiceNowConnectorConfig conf, Boolean useConnection) { | ||
| this.conf = conf; | ||
| this.useConnection = useConnection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this field ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, removed.
|
Please squash before merging. |
f7c5bc7 to
4af230c
Compare
[PLUGIN-1902 Changes done for schema backward compatibility along with metadata API addition for DTS](#100)