Skip to content
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

Migrate java-base to use AirbyteProtocol #609

Merged
merged 2 commits into from Oct 19, 2020
Merged

Migrate java-base to use AirbyteProtocol #609

merged 2 commits into from Oct 19, 2020

Conversation

cgardens
Copy link
Contributor

What

  • migrate java base to use AirbyteMessage as an envelope.
  • fix bug where AirbyteProtocolConverter could not handle when json schema sent multiple types (and add test)

With this PR we can run integrations fully using the Airbyte Protocol!!!!!

If you want to try this out, 1. run the app using the dev configs; 2. build the dev images locally for exchangratesapi source and the csv-destination destination and then 3. update these config files as follows. you should then be able to run exchangerates api to csv destination fully on airbyte protocol. once we have all the other existing sources migrated over, we can officially switch.

airbyte-config/init/src/main/resources/config/STANDARD_SOURCE/9fed261d-d107-47fd-8c8b-323023db6e20.json

{
  "sourceId": "9fed261d-d107-47fd-8c8b-323023db6e20",
  "name": "exchangeratesapi.io",
  "dockerRepository": "airbyte/source-exchangeratesapi-singer-abprotocol",
  "dockerImageTag": "dev",
  "documentationUrl": "https://hub.docker.com/r/airbyte/integration-singer-exchangeratesapi_io-source"
}

airbyte-config/init/src/main/resources/config/STANDARD_DESTINATION/8be1cf83-fde1-477f-a4ad-318d23c9f3c6.json

{
  "destinationId": "8be1cf83-fde1-477f-a4ad-318d23c9f3c6",
  "name": "Local CSV",
  "dockerRepository": "airbyte/airbyte-csv-destination-abprotocol",
  "dockerImageTag": "dev",
  "documentationUrl": "https://hub.docker.com/r/airbyte/integration-singer-csv-destination"
}

michel-tricot
michel-tricot approved these changes Oct 19, 2020
View reviewed changes
airbyte-config/models/src/main/java/io/airbyte/config/AirbyteProtocolConverters.java Outdated
@@ -77,9 +81,31 @@ public static Schema toSchema(AirbyteCatalog catalog) {
.withName(airbyteStream.getName())
.withFields(list.stream().map(item -> new Field()
.withName(item.getKey())
.withDataType(DataType.valueOf(item.getValue().get("type").asText().toUpperCase()))
.withDataType(singerTypesToDataType(item.getValue().get("type")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have the word singer in a class that is AB specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment. purely a name issue. not actually singer specific. will fix tha name. https://github.com/airbytehq/airbyte/pull/609/files#r507447435

airbyte-config/models/src/main/java/io/airbyte/config/AirbyteProtocolConverters.java Outdated
* @param singerTypes - list of types discovered by singer.
* @return reduce down to one type which best matches the field's data type
*/
private static DataType singerTypesToDataType(JsonNode node) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not singer right this is jsonschema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. copy paste error. i stole this function from our other converter, but yes you are right. this is jsonschema type to data type.

sherifnada
sherifnada suggested changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a hack around that we don't output the correct types from the integration images. Anything ABprotocol-compliant shouldn't output types that aren't specified in ABP imo.

FWIW I happened to add a fix for this in #610 that makes the integration images output this so this behavior isn't needed in AB core.

@cgardens
Copy link
Contributor Author

This seems like a hack around that we don't output the correct types from the integration images. Anything ABprotocol-compliant shouldn't output types that aren't specified in ABP imo.

@sherifnada - can you be specific about what you feel is hack? this PR makes sure that the java integrations use the ab protocol and that the worker is able to handle valid json schema. I am not understanding what your concern is.

@sherifnada
Copy link
Contributor

@cgardens Maybe the confusion here is around what ABP's type system is. Is it supposed to be all of JsonSchema's types, or whatever is present in DataType? I reviewed with the understanding that it's the latter.

sherifnada
sherifnada approved these changes Oct 19, 2020
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm - brainfart. Got confused about types.

@cgardens cgardens mentioned this pull request Oct 19, 2020
Merged
4 tasks
Base automatically changed from cgardens/ab_protocol to master October 19, 2020 17:23
@cgardens cgardens merged commit 3e8a054 into master Oct 19, 2020
@cgardens cgardens mentioned this pull request Oct 19, 2020
Closed
sherifnada pushed a commit that referenced this pull request Oct 23, 2020
@cgardens @sherifnada
Migrate java-base to use AirbyteProtocol (#609)
2ffbe7c
@swyxio swyxio deleted the cgardens/ab_protocol2 branch October 11, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhizor jrhizor jrhizor approved these changes

@michel-tricot michel-tricot michel-tricot approved these changes

@sherifnada sherifnada sherifnada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cgardens @sherifnada @jrhizor @michel-tricot