-
Notifications
You must be signed in to change notification settings - Fork 9
User Defined Functions tutorial #55
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
User Defined Functions tutorial #55
Conversation
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.
Thanks for the contribution. This is a really comprehensive tutorial and works great for local interactive queries.
However, it would be good to enhance it for more permanent deployments. There is more detail in my comments but in summary:
- Can you add instructions for building a new container image using the Flink SQL runner as a base and layering in the UDF jar. That container can then be used to launch the session cluster without needing to mount the UDF JAR yourself. It also means someone without a copy of your JAR could use the function in the session cluster.
- Can you add a
FlinkDeployment
example of how to deploy a query using your function which is standalone. - I personally think it would be better to handle the Currency symbols/codes in an Enum class which handles all the mapping logic.
...urrency-converter/src/main/java/com/github/streamshub/flink/functions/CurrencyConverter.java
Outdated
Show resolved
Hide resolved
...urrency-converter/src/main/java/com/github/streamshub/flink/functions/CurrencyConverter.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Thomas Cooper <code@tomcooper.dev> Update docs/user-defined-functions/index.md Co-authored-by: Thomas Cooper <code@tomcooper.dev> Update docs/user-defined-functions/index.md Co-authored-by: Thomas Cooper <code@tomcooper.dev> Apply suggestions from code review Co-authored-by: Thomas Cooper <code@tomcooper.dev>
7082403
to
8f72f9b
Compare
4a3e190
to
676072c
Compare
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.
LGTM, a few comments, mostly on aligning the package names and versions between the tutorial and example code. I edited my local copy of you branch and verified that everything works as expected.
We also need to decide if we are going to upload the example udf image to the streamshub quay repo so the standalone example will work as is.
tutorials/user-defined-functions/standalone-etl-udf-deployment.yaml
Outdated
Show resolved
Hide resolved
tutorials/user-defined-functions/standalone-etl-udf-deployment.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Thomas Cooper <code@tomcooper.dev>
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.
Great work @piotrpdev! Just a couple of small nits and then I'll approve.
Co-authored-by: Thomas Cooper <code@tomcooper.dev>
Note
The links to the Interactive ETL example don't work on the website, like the previously mentioned Prometheus links. They do work when reading the Markdown files directly on GitHub.