[GOBBLIN-1808] Bump Guava version from 15.0 to 20.0 #3669

Will-Lo · 2023-04-08T01:32:56Z

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1808

Description

Here are some details about my PR, including screenshots (if applicable):
TODO: Update documentation/dist folders to have the right version of Guava

Currently the version of Guava, 15.0, is from 2013, we want to introduce a newer version of Guava that should hopefully not cause as many issues with other dependencies, such as Guava 20.0 (2016).

This is the start of many version upgrades Gobblin will need to modernize and stay compatible with recent dependencies (e.g. GRPC)

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Passes unit tests

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

codecov-commenter · 2023-04-08T01:51:03Z

Codecov Report

Merging #3669 (e16e9ee) into master (0d69363) will decrease coverage by 2.05%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##             master    #3669      +/-   ##
============================================
- Coverage     46.84%   44.79%   -2.05%     
+ Complexity    10751     2089    -8662     
============================================
  Files          2139      411    -1728     
  Lines         84032    17697   -66335     
  Branches       9335     2157    -7178     
============================================
- Hits          39366     7928   -31438     
+ Misses        41082     8911   -32171     
+ Partials       3584      858    -2726

Impacted Files	Coverage Δ
...ocation/PriorityIterableBasedRequestAllocator.java	`68.42% <0.00%> (ø)`
...pache/gobblin/aws/GobblinAWSConfigurationKeys.java	`66.66% <100.00%> (ø)`

... and 1732 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

phet

not too painful at build-time--nice! lets now take it through the paces at run-time

phet · 2023-04-10T16:50:42Z

gradle/scripts/dependencyDefinitions.gradle

@@ -49,8 +49,9 @@ ext.externalDependency = [
    "commonsPool": "org.apache.commons:commons-pool2:2.4.2",
    "datanucleusCore": "org.datanucleus:datanucleus-core:3.2.10",
    "datanucleusRdbms": "org.datanucleus:datanucleus-rdbms:3.2.9",
+    "errorProne": "com.google.errorprone:error_prone_annotations:2.0.15",


did I miss it? I would have expected to see a new/adjusted import in this PR to bring in (e.g. annotations) from this new package.

Only needed it in gobblin-utility/build.gradle, not sure if I should've added it to every build.gradle module?

I saw that, but wouldn't there also be a new import in java code? ...or are classes already imported under a package that stays fixed, but the package now comes from a separate jar than it used to?

overall, I'm still unclear (from reading only the code under PR) why this dep is needed?

Errorprone is needed for some annotations such as @IgnoreReturnValue which is used in Guava transitively, but they removed the direct dependency on it so we have to include it manually.

It's discussed in this Github thread where the fix is to include errorProne as a compile dependency, otherwise we will have to bump to a future version of Guava google/guava#2837

yes, I see... makes sense there would be no imports needed in the case of one of our direct deps having left out their own dependency so it falls instead upon us to specify for them. clearly preferable when transitive dependency resolution automates the solution... but now that we know what's missing, not too hard for us to add manually.

phet

one minor Q, but otherwise all looks good

phet · 2023-04-11T16:23:56Z

gradle/scripts/dependencyDefinitions.gradle

@@ -49,8 +49,9 @@ ext.externalDependency = [
    "commonsPool": "org.apache.commons:commons-pool2:2.4.2",
    "datanucleusCore": "org.datanucleus:datanucleus-core:3.2.10",
    "datanucleusRdbms": "org.datanucleus:datanucleus-rdbms:3.2.9",
+    "errorProne": "com.google.errorprone:error_prone_annotations:2.0.15",


I saw that, but wouldn't there also be a new import in java code? ...or are classes already imported under a package that stays fixed, but the package now comes from a separate jar than it used to?

overall, I'm still unclear (from reading only the code under PR) why this dep is needed?

arjun4084346 · 2023-04-11T23:40:37Z

gobblin-modules/gobblin-elasticsearch-deps/build.gradle

@@ -33,7 +33,7 @@ tasks.remove(tasks.uploadShadow)
 dependencies {
  compile "org.elasticsearch.client:transport:5.6.8"
  compile "org.elasticsearch.client:elasticsearch-rest-high-level-client:5.6.8"
-  compile "com.google.guava:guava:18.0"
+  compile "com.google.guava:guava:20.0"


why does this module define complete coordinate of dependency here, why not compile externalDependency guava

* upstream/master: [GOBBLIN-1832] Emit warning instead of failing job for retention of Hive Table Views (apache#3695) [GOBBLIN-1831] Use flowexecutionid in kafka monitor and jobnames (apache#3694) [GOBBLIN-1824]Improving the Efficiency of Work Planning in Manifest-Based DistCp Jobs (apache#3686) [GOBBLIN-1829] Fixes bug where the wrong workunit event was being tracked for keepin… (apache#3691) [GOBBLIN-1828] Implement Timeout for Creating Writer Functionality (apache#3690) [GOBBLIN-1827] Add check that if nested field is optional and has a non-null default… (apache#3689) [GOBBLIN-1826] Change isAssignableFrom() to isSuperTypeOf() per Guava 20 javadocs to… (apache#3688) [GOBBLIN-1822]Logging Abnormal Helix Task States (apache#3685) [GOBBLIN-1819] Log helix workflow information and timeout information during submission wait / polling (apache#3681) [GOBBLIN-1821] Let flow execution ID propagate to the Job ID if it exists (apache#3684) [GOBBLIN-1810] Support general iceberg catalog (support configurable behavior for metadata retention policy) (apache#3680) Add null default value to observability events that are additionally added (apache#3682) [GOBBLIN-1816] Add job properties and GaaS instance ID to observability event (apache#3676) [GOBBLIN-1785] add MR_JARS_BASE_DIR and logic to delete old mr jar dirs (apache#3642) initiliaze yarn clients in yarn app launcher so that a child class can override the yarn client creation logic (apache#3679) [GOBBLIN-1811]Fix Iceberg Registration Serialization (apache#3673) [GOBBLIN-1817] change some deprecated code and fix minor codestyle (apache#3678) [GOBBLIN-1812] Mockito should only be test compile (apache#3674) [GOBBLIN-1813] Helix workflows submission timeouts are configurable (apache#3677) [GOBBLIN-1810] Support general iceberg catalog in icebergMetadataWriter (apache#3672) Refactor yarn app launchers to support extending these classes (apache#3671) [GOBBLIN-1808] Bump Guava version from 15.0 to 20.0 (apache#3669) [GOBBLIN-1806] Submit dataset summary event post commit and integrate them into GaaSObservabilityEvent (apache#3667) [GOBBLIN-1814] Add `MRJobLauncher` configurability for any failing mapper to be fatal to the MR job (apache#3675) Add new lookback version finder for use with iceberg retention (apache#3670)

Will-Lo added 2 commits April 7, 2023 18:03

Test

89e1c1a

Fix compile time errors with guava 20

004fef9

phet approved these changes Apr 10, 2023

View reviewed changes

Update docs and scripts to reference to guava 20

e16e9ee

Will-Lo marked this pull request as ready for review April 10, 2023 19:52

phet approved these changes Apr 11, 2023

View reviewed changes

arjun4084346 reviewed Apr 11, 2023

View reviewed changes

arjun4084346 approved these changes Apr 11, 2023

View reviewed changes

Address review

7414354

phet approved these changes Apr 12, 2023

View reviewed changes

Will-Lo merged commit 2340671 into apache:master Apr 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-1808] Bump Guava version from 15.0 to 20.0 #3669

[GOBBLIN-1808] Bump Guava version from 15.0 to 20.0 #3669

Will-Lo commented Apr 8, 2023

codecov-commenter commented Apr 8, 2023 •

edited

Loading

phet left a comment

phet Apr 10, 2023

Will-Lo Apr 10, 2023 •

edited

Loading

phet Apr 11, 2023

Will-Lo Apr 12, 2023

phet Apr 12, 2023

phet left a comment

phet Apr 11, 2023

arjun4084346 Apr 11, 2023

[GOBBLIN-1808] Bump Guava version from 15.0 to 20.0 #3669

[GOBBLIN-1808] Bump Guava version from 15.0 to 20.0 #3669

Conversation

Will-Lo commented Apr 8, 2023

JIRA

Description

Tests

Commits

codecov-commenter commented Apr 8, 2023 • edited Loading

Codecov Report

phet left a comment

Choose a reason for hiding this comment

phet Apr 10, 2023

Choose a reason for hiding this comment

Will-Lo Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

phet Apr 11, 2023

Choose a reason for hiding this comment

Will-Lo Apr 12, 2023

Choose a reason for hiding this comment

phet Apr 12, 2023

Choose a reason for hiding this comment

phet left a comment

Choose a reason for hiding this comment

phet Apr 11, 2023

Choose a reason for hiding this comment

arjun4084346 Apr 11, 2023

Choose a reason for hiding this comment

codecov-commenter commented Apr 8, 2023 •

edited

Loading

Will-Lo Apr 10, 2023 •

edited

Loading