-
Notifications
You must be signed in to change notification settings - Fork 13k
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-26357][format] add FLINK API annotations #18911
[FLINK-26357][format] add FLINK API annotations #18911
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 3f271f3 (Thu Feb 24 16:17:58 UTC 2022) 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:
|
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 PR, @JingGe, I added some comments. In general, it looks fine to me, with the exception of two points:
- What is the rationale behind marking the classes as Experimental? The API seems to be pretty tight in terms of input/output parameters, or do you expect potential changes?
- CI is currently failing, but the failures seem to be unrelated to your changes, so try a rebase.
...ink-parquet/src/main/java/org/apache/flink/formats/parquet/avro/AvroParquetRecordFormat.java
Outdated
Show resolved
Hide resolved
...ts/flink-parquet/src/main/java/org/apache/flink/formats/parquet/avro/AvroParquetReaders.java
Outdated
Show resolved
Hide resolved
...ink-parquet/src/main/java/org/apache/flink/formats/parquet/avro/AvroParquetRecordFormat.java
Outdated
Show resolved
Hide resolved
ab62edb
to
a2c9f43
Compare
Thanks @afedulov for your effort. I have addressed your comments. The reason of using Experimental is that it is the first time we introduce |
@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.
The changes look good but I think it makes sense to change the commit messages slightly to underline which format was changed from [format]
-> [avro-parquet]
a2c9f43
to
fc7fd26
Compare
Thanks @fapaul for the feedback. Commit messages have been updated. Please help merging the PR. Many thanks! |
What is the purpose of the change
Add FLINK API annotations to public classes.
Brief change log
Verifying this change
This change is a trivial code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation