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

[AMORO-2651] Enable checkstyle when building project #2671

Merged
merged 2 commits into from
Mar 26, 2024
Merged

[AMORO-2651] Enable checkstyle when building project #2671

merged 2 commits into from
Mar 26, 2024

Conversation

klion26
Copy link
Member

@klion26 klion26 commented Mar 25, 2024

Why are the changes needed?

Close #2651.

Brief change log

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added module:mixed-flink Flink moduel for Mixed Format module:mixed-spark Spark module for Mixed Format module:core Core module module:ams-server Ams server module module:mixed-hive Hive moduel for Mixed Format module:mixed-trino trino module for Mixed Format type:build labels Mar 25, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 25, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 12.28070% with 50 lines in your changes are missing coverage. Please review.

Project coverage is 33.98%. Comparing base (5b2143c) to head (2568eda).

Files Patch % Lines
...om/netease/arctic/api/config/OptimizingConfig.java 0.00% 9 Missing ⚠️
.../server/dashboard/model/AmoroSnapshotsOfTable.java 0.00% 5 Missing ⚠️
.../netease/arctic/api/config/TableConfiguration.java 0.00% 4 Missing ⚠️
...om/netease/arctic/api/config/TagConfiguration.java 0.00% 4 Missing ⚠️
...server/dashboard/controller/CatalogController.java 50.00% 4 Missing ⚠️
...tease/arctic/server/dashboard/model/TableMeta.java 0.00% 4 Missing ⚠️
...etease/arctic/server/resource/OptimizerThread.java 0.00% 4 Missing ⚠️
.../main/java/com/netease/arctic/data/ChangedLsn.java 0.00% 2 Missing and 2 partials ⚠️
...va/com/netease/arctic/io/writer/TaskWriterKey.java 25.00% 1 Missing and 2 partials ⚠️
...etease/arctic/server/terminal/ExecutionResult.java 0.00% 2 Missing ⚠️
... and 4 more
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #2671      +/-   ##
============================================
- Coverage     34.04%   33.98%   -0.06%     
+ Complexity     4363     4360       -3     
============================================
  Files           604      604              
  Lines         50727    50748      +21     
  Branches       6671     6671              
============================================
- Hits          17268    17247      -21     
- Misses        32059    32111      +52     
+ Partials       1400     1390      -10
Flag Coverage Δ
core 32.30% <12.28%> (-0.01%) ⬇️
trino 50.42% <ø> (-0.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

klion26
klion26 commented Mar 26, 2024
View reviewed changes
ams/api/src/main/java/com/netease/arctic/api/config/OptimizingConfig.java
@@ -96,15 +96,15 @@ public OptimizingConfig setTargetQuota(double targetQuota) {
return this;
}

public long getMinPlanInterval() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Some of the changes are made by the auto formate by IDEA after configured the checkstyle file.

zhoujinsong
zhoujinsong reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@zhoujinsong zhoujinsong left a comment

Choose a reason for hiding this comment

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

Thanks a lot for pushing forward this improvement!
The new checkstyle improvements have enhanced existing code and reduced the workload for code reviewers.

I left some comments, PTAL.

pom.xml Outdated Show resolved Hide resolved
tools/maven/checkstyle.xml Outdated Show resolved Hide resolved
tools/maven/suppressions.xml Show resolved Hide resolved
zhoujinsong
zhoujinsong approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@zhoujinsong zhoujinsong left a comment

Choose a reason for hiding this comment

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

LGTM.

@zhoujinsong zhoujinsong merged commit c308d13 into apache:master Mar 26, 2024
5 of 7 checks passed
@klion26 klion26 deleted the enable_checkstyle branch March 26, 2024 10:24
@klion26
Copy link
Member Author

klion26 commented Mar 26, 2024

@zhoujinsong thanks for the review and merging!

@klion26 klion26 mentioned this pull request Mar 27, 2024
Merged
3 tasks
@zhoujinsong
Copy link
Contributor

@klion26 I found that the current check style rules only check imports for classes under com.google.common.base.Preconditions in Guava. Should we extend the checking rules to the entire Guava library like all classes under com.google.common?

@klion26
Copy link
Member Author

klion26 commented Apr 3, 2024

@zhoujinsong this has been improved in #2679

@zhoujinsong
Copy link
Contributor

@zhoujinsong this has been improved in #2679

I got it. Thanks a lot for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhoujinsong zhoujinsong zhoujinsong approved these changes

Assignees
No one assigned
Labels
module:ams-server Ams server module module:core Core module module:mixed-flink Flink moduel for Mixed Format module:mixed-hive Hive moduel for Mixed Format module:mixed-spark Spark module for Mixed Format module:mixed-trino trino module for Mixed Format type:build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement]: Add checkstyle check for wildcard import
2 participants
@klion26 @zhoujinsong