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.

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

[CLIENT][JAVA] Added a loadOpenLineageJson(InputStream) method, and refactored the existing loadOpenLineageYaml(InputStream) method #2490

Merged
merged 5 commits into from Mar 6, 2024

Conversation

d-m-h
Copy link
Contributor

@d-m-h d-m-h commented Mar 4, 2024

Problem

The loadOpenLineageYaml(InputStream) method asked for YAML (via the method name), but it actually expected JSON via the method implementation.

Closes: #2487

Solution

  1. Introduce a new method, called loadOpenLineageJson(InputStream) that expects and utilises JSON.
  2. Refactor the old one, so it expects and utilises YAML.

Checklist

  • You've signed-off your work
  • Your pull request title follows our guidelines
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • Your comment includes a one-liner for the changelog about the specific purpose of the change (if necessary)
  • You've added a header to source files (if relevant)

SPDX-License-Identifier: Apache-2.0
Copyright 2018-2023 contributors to the OpenLineage project

@boring-cyborg boring-cyborg bot added the area:client/java openlineage-java label Mar 4, 2024
@d-m-h d-m-h force-pushed the client-java/refactor-client-utils-for-clarity branch from 6b63f0c to 66cfa7c Compare March 4, 2024 15:18
@boring-cyborg boring-cyborg bot added the area:documentation Improvements or additions to documentation label Mar 4, 2024
@d-m-h d-m-h force-pushed the client-java/refactor-client-utils-for-clarity branch from d37d429 to 7c7cae4 Compare March 4, 2024 15:27
@d-m-h d-m-h marked this pull request as ready for review March 4, 2024 15:40
@d-m-h d-m-h force-pushed the client-java/refactor-client-utils-for-clarity branch from ba056a5 to 3e3ecfa Compare March 4, 2024 15:47
…oadOpenLineageYaml(InputStream) method

This was done in an effort to be explicit about what the method requires

Also added (auto-generated) JavaDocs

Signed-off-by: Damien Hawes <d-m-h@users.noreply.github.com>
Signed-off-by: Damien Hawes <d-m-h@users.noreply.github.com>
Signed-off-by: Damien Hawes <d-m-h@users.noreply.github.com>
@d-m-h d-m-h force-pushed the client-java/refactor-client-utils-for-clarity branch from 3e3ecfa to bc35bbc Compare March 4, 2024 15:49
@d-m-h d-m-h self-assigned this Mar 4, 2024
@mobuchowski
Copy link
Member

@d-m-h @pawel-big-lebowski do you think we can actually change name of OpenLineageYaml to something like OpenLineageConfig or it is too much of a breaking change?

@pawel-big-lebowski
Copy link
Contributor

@d-m-h @pawel-big-lebowski do you think we can actually change name of OpenLineageYaml to something like OpenLineageConfig or it is too much of a breaking change?

@mobuchowski I think we can go for that. I cannot guarantee this will not cause issue for custom java client users, but this should have a moderate impact on Spark & Flink integration.

@d-m-h I really like the idea of fallback to JSON config if the yaml parsing fails 🥇

Signed-off-by: Damien Hawes <d-m-h@users.noreply.github.com>
Signed-off-by: Damien Hawes <d-m-h@users.noreply.github.com>
@d-m-h
Copy link
Contributor Author

d-m-h commented Mar 5, 2024

@d-m-h @pawel-big-lebowski do you think we can actually change name of OpenLineageYaml to something like OpenLineageConfig or it is too much of a breaking change?

I initially wanted to do that, but I decided on the split method approach and then the subsequent fallback approach, because I could not measure the impact.

@d-m-h d-m-h merged commit 3f52111 into main Mar 6, 2024
38 checks passed
@d-m-h d-m-h deleted the client-java/refactor-client-utils-for-clarity branch March 6, 2024 11:06
Ruihua98 pushed a commit to Ruihua98/OpenLineage that referenced this pull request Mar 15, 2024
…efactored the existing loadOpenLineageYaml(InputStream) method (OpenLineage#2490)

* Added a loadOpenLineageJson(InputStream) method, and refactored the loadOpenLineageYaml(InputStream) method
* Improved the error handling for loadOpenLineageYaml(ConfigPathProvider)
* Explicitly state the exceptions, despite being unchecked exceptions

Signed-off-by: Damien Hawes <d-m-h@users.noreply.github.com>
Signed-off-by: Ruihua Wang <ruihuawang@microsoft.com>
blacklight pushed a commit to blacklight/OpenLineage that referenced this pull request Apr 4, 2024
…efactored the existing loadOpenLineageYaml(InputStream) method (OpenLineage#2490)

* Added a loadOpenLineageJson(InputStream) method, and refactored the loadOpenLineageYaml(InputStream) method
* Improved the error handling for loadOpenLineageYaml(ConfigPathProvider)
* Explicitly state the exceptions, despite being unchecked exceptions

Signed-off-by: Damien Hawes <d-m-h@users.noreply.github.com>
Signed-off-by: Fabio Manganiello <fabio@manganiello.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:client/java openlineage-java area:documentation Improvements or additions to documentation
Projects
None yet
3 participants