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

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

Merged
merged 4 commits into from
Apr 12, 2023

Conversation

Will-Lo
Copy link
Contributor

@Will-Lo Will-Lo commented Apr 8, 2023

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

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
Copy link

codecov-commenter commented Apr 8, 2023

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

Copy link
Contributor

@phet phet left a comment

Choose a reason for hiding this comment

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

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

@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Will-Lo Will-Lo Apr 10, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

@phet phet left a comment

Choose a reason for hiding this comment

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

one minor Q, but otherwise all looks good

@@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Will-Lo Will-Lo merged commit 2340671 into apache:master Apr 12, 2023
phet added a commit to phet/gobblin that referenced this pull request Aug 15, 2023
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants