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

Build on JDK 8, revert private build of protostuff. #418

Merged
merged 6 commits into from
Mar 11, 2022

Conversation

dblock
Copy link
Member

@dblock dblock commented Mar 10, 2022

Signed-off-by: dblock [email protected]

Description

  • Build on JDK8.
  • Revert private build of protostuff.

Issues Resolved

Check List

  • Commits are signed per the DCO using --signoff

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.

@dblock dblock changed the title Build on JDK 8. Build on JDK 8, revert private build of protostuff. Mar 10, 2022
@dblock dblock mentioned this pull request Mar 10, 2022
1 task
/**
* Mockito does not allow mock final methods. Make my own delegates and mock them.
*
*/
Copy link
Member Author

@dblock dblock Mar 10, 2022

Choose a reason for hiding this comment

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

This seems to have been written to avoid precisely the mockito problem:

Suite: Test class org.opensearch.action.admin.indices.mapping.get.ValidateAnomalyDetectorActionHandlerTests
  1> [2022-03-11T06:01:02,966][INFO ][o.o.a.a.i.m.g.ValidateAnomalyDetectorActionHandlerTests] [testValidateMoreThanTenMultiEntityDetectorsLimit] before test
  1> [2022-03-11T06:01:04,994][INFO ][o.o.a.a.i.m.g.ValidateAnomalyDetectorActionHandlerTests] [testValidateMoreThanTenMultiEntityDetectorsLimit] after test
  2> REPRODUCE WITH: ./gradlew ':test' --tests "org.opensearch.action.admin.indices.mapping.get.ValidateAnomalyDetectorActionHandlerTests.testValidateMoreThanTenMultiEntityDetect
orsLimit" -Dtests.seed=985E69252C997DD5 -Dtests.security.manager=false -Dtests.locale=et-EE -Dtests.timezone=Etc/GMT-9 -Druntime.java=8
  2> org.mockito.exceptions.base.MockitoException:
    Mockito cannot mock this class: class org.opensearch.action.admin.indices.mapping.get.IndexAnomalyDetectorActionHandlerTests$4.

    Mockito can only mock non-private & non-final classes.
    If you're not sure why you're getting this error, please report to the mailing list.


    Java               : 1.8
    JVM vendor name    : Private Build
    JVM vendor version : 25.312-b07
    JVM name           : OpenJDK 64-Bit Server VM
    JVM version        : 1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
    JVM info           : mixed mode
    OS name            : Linux
    OS version         : 5.13.0-1014-aws


    Underlying exception : java.lang.IllegalStateException: Error invoking java.lang.ClassLoader#defineClass
        at __randomizedtesting.SeedInfo.seed([985E69252C997DD5:A070E1A9435257DF]:0) 
        at org.opensearch.mockito.plugin.PriviledgedMockMaker.lambda$createMock$4(PriviledgedMockMaker.java:77)
        at java.security.AccessController.doPrivileged(Native Method)
        at org.opensearch.mockito.plugin.PriviledgedMockMaker.createMock(PriviledgedMockMaker.java:77)
        at org.opensearch.action.admin.indices.mapping.get.ValidateAnomalyDetectorActionHandlerTests.testValidateMoreThanTenMultiEntityDetectorsLimit(ValidateAnomalyDetectorActio
nHandlerTests.java:222)

        Caused by:
        java.lang.IllegalStateException: Error invoking java.lang.ClassLoader#defineClass
            at net.bytebuddy.dynamic.loading.ClassInjector$UsingReflection$Dispatcher$Direct.defineClass(ClassInjector.java:609)

I couldn't make it work, and since this affects tests only and we're dropping support for JDK8 in 2.0, ignoring the handful of tests seems ok.

The code is unused, deleting.

@dblock dblock mentioned this pull request Mar 10, 2022
4 tasks
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #418 (79c9840) into main (31a0dbe) will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #418      +/-   ##
============================================
+ Coverage     77.56%   77.72%   +0.16%     
- Complexity     4107     4111       +4     
============================================
  Files           296      296              
  Lines         17655    17669      +14     
  Branches       1876     1878       +2     
============================================
+ Hits          13694    13734      +40     
+ Misses         3057     3034      -23     
+ Partials        904      901       -3     
Flag Coverage Δ
plugin 77.72% <ø> (+0.16%) ⬆️

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

Impacted Files Coverage Δ
...ransport/DeleteAnomalyDetectorTransportAction.java 51.00% <0.00%> (-0.52%) ⬇️
.../ad/rest/handler/AnomalyDetectorActionHandler.java 6.45% <0.00%> (-0.22%) ⬇️
...opensearch/ad/indices/AnomalyDetectionIndices.java 72.09% <0.00%> (-0.19%) ⬇️
...d/transport/GetAnomalyDetectorTransportAction.java 60.94% <0.00%> (-0.14%) ⬇️
...c/main/java/org/opensearch/ad/util/ParseUtils.java 70.00% <0.00%> (+0.10%) ⬆️
src/main/java/org/opensearch/ad/model/Entity.java 85.18% <0.00%> (+0.13%) ⬆️
...rg/opensearch/ad/AnomalyDetectorProfileRunner.java 68.98% <0.00%> (+0.21%) ⬆️
...in/java/org/opensearch/ad/EntityProfileRunner.java 75.00% <0.00%> (+0.28%) ⬆️
...java/org/opensearch/ad/task/ADBatchTaskRunner.java 82.06% <0.00%> (+0.30%) ⬆️
.../main/java/org/opensearch/ad/NodeStateManager.java 71.61% <0.00%> (+0.37%) ⬆️
... and 8 more

@dblock dblock marked this pull request as ready for review March 10, 2022 23:45
@dblock dblock requested review from a team, amitgalitz, ohltyler and kaituo March 10, 2022 23:45
@ohltyler ohltyler merged commit c0218e5 into opensearch-project:main Mar 11, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 11, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 11, 2022
ohltyler pushed a commit that referenced this pull request Mar 11, 2022
ohltyler pushed a commit that referenced this pull request Mar 11, 2022
@ohltyler ohltyler mentioned this pull request Mar 11, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Remove hand-built protostuff Default CI Java Version to Java 11, run tests on 8, 14 and 17
4 participants