Skip to content

Conversation

jngz-es
Copy link
Contributor

@jngz-es jngz-es commented May 7, 2025

Description

As a common extensible plugin, remove guava dependency to reduce jar hell or conflicts issues caused by guava in extending plugins like SQL, ML etc.

Related Issues

opensearch-project/OpenSearch#18113

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Jing Zhang <[email protected]>

dependencies {
implementation project(path: ":${rootProject.name}-spi", configuration: 'shadow')
implementation group: 'com.google.guava', name: 'guava', version:'32.1.3-jre'
Copy link
Member

Choose a reason for hiding this comment

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

I think the whole section above this can go too:

configurations {
    all {
        resolutionStrategy {
            force 'com.google.guava:guava:32.1.3-jre'
        }
    }
}

Also what about the section below?

    //spotless
    implementation('com.google.googlejavaformat:google-java-format:1.26.0') {
        exclude group: 'com.google.guava'
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I am removing them as well.

@andrross
Copy link
Member

andrross commented May 7, 2025

This makes sense to me! The JDK now natively supports these immutable collections so no need to bring in guava just to get ImmutableList and ImmutableMap.

Copy link

codecov bot commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.67%. Comparing base (f508936) to head (8de1383).
Report is 1 commits behind head on main.

❌ Your project status has failed because the head coverage (37.67%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #773   +/-   ##
=========================================
  Coverage     37.67%   37.67%           
  Complexity      135      135           
=========================================
  Files            22       22           
  Lines          1189     1189           
  Branches        109      109           
=========================================
  Hits            448      448           
  Misses          704      704           
  Partials         37       37           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@prudhvigodithi
Copy link
Member

Thanks @jngz-es thanks for this change. Same as what Andrew mentioned, I have tested with List.of and behaves the same

List<String> immutable = List.of("a", "b", "c");
immutable.add("d"); // this throws exception.

@kaituo
Copy link
Collaborator

kaituo commented May 8, 2025

CI failed. Can you take a look?

@kaituo
Copy link
Collaborator

kaituo commented May 8, 2025

rerun CI

@cwperks
Copy link
Member

cwperks commented May 8, 2025

I support removing guava from this plugin, but also wanted to leave insight I found out about extensible plugins recently.

When pluginA extends pluginB, all of pluginBs deps are available to pluginA at runtime, however they are not available at compile time

Because of this, plugins like AD are able to specify compileOnly dependencies on guava and know that they are available at runtime because of their extension on JS plugin.

The key is to ensure there is a version catalog to keep these common deps in sync.

@kaituo kaituo merged commit 90b53cc into opensearch-project:main May 8, 2025
13 of 14 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in Engineering Effectiveness Board May 8, 2025
@cwperks
Copy link
Member

cwperks commented May 8, 2025

@kaituo make sure to update AD from compileOnly to implementation for the guava deps now that this change is merged.

@cwperks
Copy link
Member

cwperks commented May 9, 2025

@kaituo make sure to update AD from compileOnly to implementation for the guava deps now that this change is merged.

@kaituo I raised opensearch-project/anomaly-detection#1473 to address this

Jeremydupras pushed a commit to Jeremydupras/job-scheduler that referenced this pull request Jun 24, 2025
* remove guava dependency

Signed-off-by: Jing Zhang <[email protected]>

* remove more unused dependencies

Signed-off-by: Jing Zhang <[email protected]>

---------

Signed-off-by: Jing Zhang <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Jeremydupras pushed a commit to Jeremydupras/job-scheduler that referenced this pull request Jun 30, 2025
* remove guava dependency

Signed-off-by: Jing Zhang <[email protected]>

* remove more unused dependencies

Signed-off-by: Jing Zhang <[email protected]>

---------

Signed-off-by: Jing Zhang <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
cwperks added a commit that referenced this pull request Jul 2, 2025
* remove guava dependency (#773)

* remove guava dependency

Signed-off-by: Jing Zhang <[email protected]>

* remove more unused dependencies

Signed-off-by: Jing Zhang <[email protected]>

---------

Signed-off-by: Jing Zhang <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>

* added Request API, Action API and Test

Signed-off-by: Jeremy Dupras <[email protected]>

* changing file names

Signed-off-by: Jeremy Dupras <[email protected]>

* need to resolve permission issues/ changes to gather all scheduled job information

Signed-off-by: Jeremy Dupras <[email protected]>

* adding to RestGetSchedulingAction

Signed-off-by: Jeremy Dupras <[email protected]>

* adding additionl tests

Signed-off-by: Jeremy Dupras <[email protected]>

* Added IT test for ScheduleInfo

Signed-off-by: Jeremy Dupras <[email protected]>

* updating progres  - verified API calls in terminal

Signed-off-by: Jeremy Dupras <[email protected]>

* adding new values to prepare request

Signed-off-by: Jeremy Dupras <[email protected]>

* removing unneeded code

Signed-off-by: Jeremy Dupras <[email protected]>

* adding transport level API

Signed-off-by: Jeremy Dupras <[email protected]>

* adding job type and refactoring json output

Signed-off-by: Jeremy Dupras <[email protected]>

* adding node selection, duplicate filtering and total job count

Signed-off-by: Jeremy Dupras <[email protected]>

* removing node specific API

Signed-off-by: Jeremy Dupras <[email protected]>

* adding integration tests

Signed-off-by: Jeremy Dupras <[email protected]>

* updating Scheduled JobInfoIT

Signed-off-by: Jeremy Dupras <[email protected]>

* removing comments

Signed-off-by: Jeremy Dupras <[email protected]>

* Adding comments

Signed-off-by: Jeremy Dupras <[email protected]>

* removing unnessessary tests

Signed-off-by: Jeremy Dupras <[email protected]>

* Add a CHANGELOG and changelog_verifier workflow (#778)

* Add a CHANGELOG and changelog_verifier workflow

Signed-off-by: Craig Perkins <[email protected]>

* spotless

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>

* Add 3.1.0 release notes (#779)

Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>

* Update Maven snapshots publishing endpoint and credential retrieval (#780)

Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>

* Increment version to 3.2.0 (#783)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>

* removing .idea files

Signed-off-by: Jeremy Dupras <[email protected]>

* fixing formatting issues

Signed-off-by: Jeremy Dupras <[email protected]>

* adding license header

Signed-off-by: Jeremy Dupras <[email protected]>

* change api call, add testing functionality

Signed-off-by: Jeremy Dupras <[email protected]>

* stop tracking .idea/*

Signed-off-by: Jeremy Dupras <[email protected]>

* changing constant calls

Signed-off-by: Jeremy Dupras <[email protected]>

* adding additional testing fields

Signed-off-by: Jeremy Dupras <[email protected]>

* changing permissions

Signed-off-by: Jeremy Dupras <[email protected]>

* removing file

Signed-off-by: Jeremy Dupras <[email protected]>

* adding files -s

Signed-off-by: Jeremy Dupras <[email protected]>

* reset

Signed-off-by: Jeremy Dupras <[email protected]>

* adding to reset

Signed-off-by: Jeremy Dupras <[email protected]>

* deleting comment

Signed-off-by: Jeremy Dupras <[email protected]>

* adding .idea files

Signed-off-by: Jeremy Dupras <[email protected]>

---------

Signed-off-by: Jing Zhang <[email protected]>
Signed-off-by: Jeremy Dupras <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Jeremydupras <[email protected]>
Co-authored-by: Jing Zhang <[email protected]>
Co-authored-by: Jeremy Dupras <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
Co-authored-by: Prudhvi Godithi <[email protected]>
Co-authored-by: Zelin Hao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

5 participants