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

[AMORO-1747] Ban guava direct import when building #2679

Merged
merged 5 commits into from
Apr 3, 2024

Conversation

klion26
Copy link
Member

@klion26 klion26 commented Mar 26, 2024

Why are the changes needed?

Close #1747.

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? (no)
  • If yes, how is the feature documented? (not applicable)

@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:ams-optimizer AMS optimizer module module:mixed-trino trino module for Mixed Format labels Mar 26, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

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

Project coverage is 34.02%. Comparing base (c308d13) to head (e6ae0e1).
Report is 4 commits behind head on master.

❗ Current head e6ae0e1 differs from pull request most recent head 501945f. Consider uploading reports for the commit 501945f to get more accurate results

Files Patch % Lines
...che/hadoop/hive/metastore/HiveMetaStoreClient.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2679      +/-   ##
============================================
- Coverage     34.03%   34.02%   -0.01%     
+ Complexity     4363     4362       -1     
============================================
  Files           604      604              
  Lines         50748    50748              
  Branches       6671     6671              
============================================
- Hits          17270    17267       -3     
- Misses        32078    32080       +2     
- Partials       1400     1401       +1     
Flag Coverage Δ
core 32.30% <ø> (ø)
trino 50.87% <0.00%> (-0.07%) ⬇️

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.

Copy link
Contributor

@majin1102 majin1102 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zhongqishang zhongqishang left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.
I found some that didn't use shade class because it was suppressed in suppressions.xml.

e.g.
com.netease.arctic.api.config.OptimizingConfig

&& com.google.common.base.Objects.equal(optimizerGroup, that.optimizerGroup);

com.netease.arctic.api.metrics.MetricKey

import com.google.common.collect.ImmutableMap;
...

com.netease.arctic.server.dashboard.controller.OptimizerController

import com.google.common.base.Preconditions;

...

@klion26
Copy link
Member Author

klion26 commented Mar 27, 2024

@zhongqishang thanks for the review. The suppressions ware introduced by #2671, and intended to be modified in #2675.
The pr here just want to replace all the remaining guava import.

I can also modify these if we need to change all guava import in this PR

@klion26 klion26 force-pushed the ban_guava_import branch 2 times, most recently from e5b6594 to 42a8d48 Compare April 2, 2024 01:27
@klion26
Copy link
Member Author

klion26 commented Apr 2, 2024

kindly ping @zhongqishang @zhoujinsong

Copy link
Contributor

@zhongqishang zhongqishang left a comment

Choose a reason for hiding this comment

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

LGTM.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

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

Project coverage is 34.02%. Comparing base (882b78b) to head (0c13eda).

Files Patch % Lines
...rver/dashboard/MixedAndIcebergTableDescriptor.java 0.00% 2 Missing ⚠️
...che/hadoop/hive/metastore/HiveMetaStoreClient.java 0.00% 2 Missing ⚠️
...om/netease/arctic/api/config/OptimizingConfig.java 0.00% 1 Missing ⚠️
...om/netease/arctic/api/config/TagConfiguration.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2679      +/-   ##
============================================
- Coverage     34.02%   34.02%   -0.01%     
  Complexity     4364     4364              
============================================
  Files           604      604              
  Lines         50754    50752       -2     
  Branches       6673     6673              
============================================
- Hits          17269    17268       -1     
+ Misses        32086    32085       -1     
  Partials       1399     1399              
Flag Coverage Δ
core 32.29% <20.00%> (-0.01%) ⬇️
trino 50.93% <0.00%> (ø)

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.

@zhoujinsong zhoujinsong merged commit 0c5a173 into apache:master Apr 3, 2024
4 checks passed
@zhoujinsong
Copy link
Contributor

@klion26 Thanks a lot for your contribution!

@klion26
Copy link
Member Author

klion26 commented Apr 3, 2024

@zhoujinsong @zhongqishang @majin1102 thanks a lot for the review and merging!

@klion26 klion26 deleted the ban_guava_import branch April 3, 2024 08:41
XBaith pushed a commit that referenced this pull request Apr 26, 2024
* [AMORO-1747] Ban guava direct import when building

* spotless:apply for trino mixed format

* fix can find class

* fix all invalid guava import

---------
resolve conflicts
use iceberg-relocated

Co-authored-by: Qishang Zhong <zhongqishang@gmail.com>

(cherry picked from commit 0c5a173)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:ams-optimizer AMS optimizer module module:ams-server Ams server module module:core Core module module:mixed-flink Flink moduel for Mixed Format module:mixed-spark Spark module for Mixed Format module:mixed-trino trino module for Mixed Format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement]: Add guava check to checkstyle rules.
5 participants