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

Upgrade to Java 17 #9764

Merged
merged 23 commits into from
Aug 21, 2023
Merged

Upgrade to Java 17 #9764

merged 23 commits into from
Aug 21, 2023

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Aug 7, 2023

What this PR does / why we need it:

Upgrade from Java 11 to 17

Which issue(s) this PR closes:

Special notes for your reviewer:

The biggest change (in terms of number of files) has to do with REST Assured. I had to upgrade it to get it working on Java 17. The upgrade required a namespace change so I had to fix all the classes that import REST Assured.

The following tests were failing...

Error:  Errors: 
Error:    DataAccessTest.testCreateNewStorageIO_createsFileAccessIObyDefault:59 » NullPointer
Error:    DatasetUtilTest.testGetThumbnailCandidates:33 » NullPointer
Error:    DatasetUtilTest.testGetThumbnailNullDataset:51 » NullPointer

... and I fixed the DatasetUtilTest failures by cherry picking 23cc3b5 and DataAccessTest by my setting the dataset authority in a mock (c1b57a1).

I've set up an API test running on the side at https://github.com/pdurbin/dataverse-api-test-runner/actions/workflows/java17.yml but I'd be happy to put this into the main code base.

Jenkins tests are failing until we switch Jenkins to Payara 6.

Suggestions on how to test this:

Test with Java 17

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No.

Is there a release notes update needed for this change?:

Included.

Additional documentation:

Included.

@pdurbin pdurbin mentioned this pull request Aug 7, 2023
@pdurbin pdurbin self-assigned this Aug 7, 2023
@pdurbin pdurbin added D: Payara 6 Upgrade Issues and PRs about the move to Jakarta EE 10 + Payara 6 Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) labels Aug 7, 2023
@pdurbin pdurbin removed their assignment Aug 7, 2023
@pdurbin pdurbin linked an issue Aug 7, 2023 that may be closed by this pull request
@pdurbin pdurbin added this to the 6.0 milestone Aug 7, 2023
pdurbin and others added 5 commits August 7, 2023 16:18
During unit testing it occured that sometimes parts of paths are null (there was a missing mock for authority).
Changing the code structure here to catch this anytime.

This hard to track cause of trouble was revealed when running tests with JDK 17, because since
JDK 16 all java.nio.file.Path may not contain null elements for the first component.

See also: https://stackoverflow.com/questions/68791709
Get this test to pass:
DataAccessTest#testCreateNewStorageIO_createsFileAccessIObyDefault
@github-actions

This comment has been minimized.

@sekmiller sekmiller self-assigned this Aug 9, 2023
@coveralls
Copy link

coveralls commented Aug 11, 2023

Coverage Status

coverage: 20.368% (+0.002%) from 20.366% when pulling f0c6778 on 8094-java-17 into f82d593 on develop.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@pdurbin pdurbin self-assigned this Aug 16, 2023
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@pdurbin
Copy link
Member Author

pdurbin commented Aug 16, 2023

Whoops! We just talked about this during sprint planning and I gave it a 33 because of 4-5 tests failing but after a single tiny fix at 8de674b having to do with syntax for the upgraded REST Assured library that's part of this PR (by necessity to get it to run on Java 17), the API test suite is passing in multiple places now:

I'm putting this back into ready for QA.

Please note that https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9764/15/display/redirect (the regular PR job) is expected to fail because the job uses Java 11. This is why we (ok, @donsizemore ) set up the dedicated Java 17 Jenkins job above.

@pdurbin pdurbin removed their assignment Aug 16, 2023
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@pdurbin
Copy link
Member Author

pdurbin commented Aug 17, 2023

@kcondon if it's of interest, thanks to @poikilotherm in #9789 (merged into this PR), we are now uploading a war file built from PRs to GitHub Actions.

To find the war file (dataverse-java17.war), under the list of checks, you can click the "Details" link under "Maven Unit Tests / (Stable / JDK 17) Unit Tests" and then "Summary".

I find it easier to go directly to https://github.com/IQSS/dataverse/actions/workflows/maven_unit_test.yml and then pick the right branch for the PR. Either way will get you there.

Note the file will be downloaded as dataverse-java17.war.zip and you'll need to unzip it. (This is a GitHub Actions thing: https://github.com/actions/upload-artifact#zipped-artifact-downloads .)

@kcondon kcondon self-assigned this Aug 18, 2023
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2023

Issues found:

  1. Log entries related to flash and cookies. Not sure yet when they happen, happens sporadically. No apparent user impact: [2023-08-18T14:18:31.624+0000] [Payara 6.2023.8] [SEVERE] [faces.externalcontext.flash.bad.cookie] [jakarta.enterprise.resource.webcontainer.faces.flash] [tid: _ThreadID=99 _ThreadName=http-thread-pool::jk-connector(1)] [timeMillis: 1692368311624] [levelValue: 1000] [[
    JSF1094: Could not decode flash data from incoming cookie value Invalid characters in decrypted value. Processing will continue, but the flash is unavailable for this request.]] [KC] opened as a separate issue: Payara 6: Log errors related to decoding flash data from cookies. #9800

  2. Basic search seems not to differentiate/work, adv, navbar search do. "Check limited perms" returns all. Seems to refresh the page only, since terms are cleared from search box. They work and remain on demo with correct results shown. [KC Opened separate issue: https://github.com/Payara 6: Basic search no longer works due to extra, blank url query param #9803

  3. Download file doesn't work with private url, throws 403. This might be siteURL related due to private going to port 8080. Will double check. Yes, was site url set to 8080. Should recheck installer for what it's telling us but not a j17 issue. [KC] opened a separate issue: Payara6: URL encoding has changed so going through Apache or directly to 8080 can break things; export download, privateURL download. #9797

  4. Downloading export from dataset page works when siteURL uses port 8080 but fails when going through apache: [KC] opened a separate issue: Payara6: URL encoding has changed so going through Apache or directly to 8080 can break things; export download, privateURL download. #9797

{"status":"ERROR","message":"A dataset with the persistentId doi%3A10.70122/FK2/JAKDVW could not be found."}

[2023-08-18T15:47:47.365+0000] [Payara 6.2023.8] [INFO] [] [edu.harvard.iq.dataverse.AbstractGlobalIdServiceBean] [tid: _ThreadID=97 _ThreadName=http-thread-pool::jk-connector(4)] [timeMillis: 1692373667365] [levelValue: 800] [[
Error parsing identifier: doi%3A10.70122/FK2/JAKDVW: ':' not found in string]]

  1. When downloading a file with tou/guestbook enabled, when popup appears, throws server log error but it appears to work:
    gb_tou_popup_log_err.txt [KC] Payara 6: When downloading a file with tou/guestbook enabled, works but throws server log error. #9806

  2. Server log error when running harvest client, seems to work anyway:
    harvest_client_svr_log_err.txt [KC] Payara 6: Server log error when running harvest client, harvest continues to work. #9809

@github-actions
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:8094-java-17
ghcr.io/gdcc/configbaker:8094-java-17

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

1 similar comment
@github-actions
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:8094-java-17
ghcr.io/gdcc/configbaker:8094-java-17

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

@kcondon
Copy link
Contributor

kcondon commented Aug 21, 2023

Given that none of the above issues are showstoppers specifically related to j17, we've decided to merge and look at the issues separately. See above for issue.

@kcondon kcondon merged commit 45a3936 into develop Aug 21, 2023
@kcondon kcondon deleted the 8094-java-17 branch August 21, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D: Payara 6 Upgrade Issues and PRs about the move to Jakarta EE 10 + Payara 6 Size: 30 A percentage of a sprint. 21 hours. (formerly size:33)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate codebase to Java 17 LTS
6 participants