-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
[FLINK-24718][avro] update AVRO dependency to latest version #18133
[FLINK-24718][avro] update AVRO dependency to latest version #18133
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit aa10e52 (Thu Dec 16 12:46:26 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
aa10e52
to
ac7b434
Compare
635596e
to
df4f1ac
Compare
@flinkbot run azure |
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.
It looks good to me and I would like to get it merged, but I'm not sure if there's a preference to wait for the fix for https://issues.apache.org/jira/browse/AVRO-3273
@fapaul What do you think?
I'm not really fond of the workaround. Does the avro-maven-plugin actually have to be in sync with the avro version? |
@RyanSkraba Can you elaborate on that? |
Hello: it's definitely best practice that the avro-maven-plugin is the same as the avro version. I would recommend waiting for 1.11.1, rather than having On the positive side, the workaround is pretty inoffensive -- explicitly specifying a transitive dependency that is only used during the build. Avro releases about twice a year, but we can ask for a release sooner rather than later as well. I haven't seen other reports from projects that use older versions of Maven. |
@RyanSkraba Since AVRO 1.11.1 is now released, could you update this PR? |
I'd love to see this in 1.16! I'm currently with limited connection, but I'll get this updated to 1.11.1 within the week. |
47d686b
to
379b0d5
Compare
@flinkbot run azure |
Hello Team, Can you please confirm, when we are planning to release Flink 1.16 with Avro 1.11.1? |
379b0d5
to
4abd923
Compare
Hello! I've rebased, removed the maven workaround that we needed for 1.11.0 and squashed everything together. There is a CI failure with TPC-DS e2e tests, but I can't seem to determine if it's related! |
See FLINK-28789 for the cause of the e2e failure. |
+ "\"type_date\": 2014-03-01, \"type_time_millis\": 12:12:12, \"type_time_micros\": 00:00:00.123456, " | ||
+ "\"type_timestamp_millis\": 2014-03-01T12:12:12.321Z, " | ||
+ "\"type_timestamp_micros\": 1970-01-01T00:00:00.123456Z, \"type_decimal_bytes\": \"\\u0007Ð\", " | ||
+ "\"type_date\": \"2014-03-01\", \"type_time_millis\": \"12:12:12\", \"type_time_micros\": \"00:00:00.123456\", " |
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.
Can you elaborate on why these quotes were added? Were they rejected by Avro etc.
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.
Hello! This was a fix introduced by AVRO-3124, cleaning up the toString()
representation to look more like valid JSON.
My personal opinion is that nobody should parse a toString()
return value... but it has been useful in testing.
Hello! For info, did this make it to the list for 1.16? |
@flinkbot run azure |
Not yet, but perhaps we can still squeeze it in :) |
What is the purpose of the change
(Continuing on the work of #17623)
Brief change log
Verifying this change
Please make sure both new and modified tests in this PR follows the conventions defined in our code quality guide: https://flink.apache.org/contributing/code-style-and-quality-common.html#testing
(Please pick either of the following options)
This change is already covered by existing tests, such as flink-avro/.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation