dataverse icon indicating copy to clipboard operation
dataverse copied to clipboard

7000 mpconfig basic infra

Open poikilotherm opened this issue 2 years ago • 3 comments

What this PR does / why we need it:

This will introduce the most basic infrastructure necessary for MicroProfile Config usage to replace any System.getProperty() calls - see also #7000. This enables configuring the JVM settings via the different MPCONFIG in a backward compatible manner (existing installations can keep their configuration but will receive infos in the logs about migrating).

This will ease container and (to some extent) development usage a lot. However, it will not interfere with Database Settings for now.

THIS PULL REQUEST IS THE MOST BASIC INFRASTRUCTURE FOR #8823, #8824, #8825, #8826, #8827, #8828

Which issue(s) this PR closes:

Closes #7680

Special notes for your reviewer: This pull request is basically just setting the JVM settings retrieval infrastructure in place. It doesn't touch any existing code yet (this will be done in succeeding small pull requests). It has unit tests added where necessary.

Suggestions on how to test this: As there are no changes to existing functionality and the whole thing is still decoupled from all of our code in this PR, there isn't much to test. You can run the test included and play around with @JvmSetting for JUnit 5 tests.

Does this PR introduce a user interface change? If mockups are available, please link/include them here: Nope, this PR is only dev related.

Is there a release notes update needed for this change?: Not from this PR IMHO. PRs following up that change actual code should contain hints if necessary.

Additional documentation: Included.

poikilotherm avatar Jun 30 '22 17:06 poikilotherm

Coverage Status

Coverage increased (+0.02%) to 20.095% when pulling 17fc0dc952c7373b8f6cc70ebb78b4495e5f2325 on poikilotherm:7000-mpconfig-infra into 3a91b76deb392e571503d5c9b57f84a5bdd1fb5f on IQSS:develop.

coveralls avatar Jul 19 '22 18:07 coveralls

Hey @scolapasta as requested, merged latest develop.

Is it fine to leave the other PRs for actual features untouched for now and rebase once this is merged?

Oh and dear future reviewer: happy to do some demos for this stuff 🙃

poikilotherm avatar Aug 05 '22 18:08 poikilotherm

@poikilotherm sure, since you'll want to rebase then, that makes perfect sense. (especially since there's a decent chance we'll have 5.12 out by then and you'd have to do it an even extra time...)

scolapasta avatar Aug 05 '22 18:08 scolapasta

I'm basically ready to approve this PR but I thought I'd try the new JvmSettings.<SETTING NAME>.lookup(...) syntax it introduces.

I looked at PR #8824 and played around with our getVersion code.

First I tried changing the method to this...

public String getVersion(boolean withBuildNumber) {
    return JvmSettings.VERSION.lookup();
}

... and added the following to domain.xml ...

 <jvm-options>-Ddataverse.version=awesome</jvm-options>

... but at http://localhost:8080/api/info/version I got an empty JSON object ({}) and java.lang.IllegalStateException: Recursive update in server.log.

Hmm. What did I miss? I noticed that PR #8824 also included this line...

private static final Config config = ConfigProvider.getConfig();

... and while the config object is never used, this seemed to fix it. That is, the version from the API was "awesome". But why is that line necessary? Should we document it?


Update: I pushed my scratch work of playing around with "lookup" and "dataverse.version" here: https://github.com/pdurbin/dataverse/commit/898dce1b0e761ba70501904ca7c033dbe53e678d

pdurbin avatar Sep 06 '22 20:09 pdurbin

@poikilotherm and I chatted. The bottom line is that I'm going to update to the version of Payara in PR #8949 (5.2022.3) to see if I can still reproduce the IllegalStateException: Recursive update error.

This time I reproduced it slightly differently.

First, I made the following change to exercise "lookup":

$ git diff
diff --git a/src/main/java/edu/harvard/iq/dataverse/util/SystemConfig.java b/src/main/java/edu/harvard/iq/dataverse/util/SystemConfig.java
index bd27405fae..814b5311e7 100644
--- a/src/main/java/edu/harvard/iq/dataverse/util/SystemConfig.java
+++ b/src/main/java/edu/harvard/iq/dataverse/util/SystemConfig.java
@@ -8,6 +8,7 @@ import edu.harvard.iq.dataverse.DvObjectContainer;
 import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean;
 import edu.harvard.iq.dataverse.authorization.providers.builtin.BuiltinAuthenticationProvider;
 import edu.harvard.iq.dataverse.authorization.providers.oauth2.AbstractOAuth2AuthenticationProvider;
+import edu.harvard.iq.dataverse.settings.JvmSettings;
 import edu.harvard.iq.dataverse.settings.SettingsServiceBean;
 import edu.harvard.iq.dataverse.validation.PasswordValidatorUtil;
 import java.io.FileInputStream;
@@ -132,6 +133,10 @@ public class SystemConfig {
     // candidate for being moved into some kind of an application-scoped caching
     // service... some CachingService @Singleton - ? (L.A. 5.8)
     public String getVersion(boolean withBuildNumber) {
+
+        if (true) {
+            return JvmSettings.VERSION.lookup();
+        }
         
         if (appVersionString == null) {

As before I have "dataverse.version" defined in domain.xml:

    <jvm-options>-Ddataverse.version=awesome</jvm-options>

Then I built and deployed with mvn clean && mvn package && scripts/dev/dev-rebuild.sh.

Somewhat surprisingly, it worked! I saw "awesome" as the version:

$ curl http://localhost:8080/api/info/version {"status":"OK","data":{"version":"awesome","build":null}}

But the I changed the version in domain.xml like this...

    <jvm-options>-Ddataverse.version=spectacular</jvm-options>

... and instead of seeing a new version, I got the same 500 error and {} as output and IllegalStateException: Recursive update. Here's the stacetrace: server.log.mpconfig.txt

Again, I'll try again with a newer Payara.

pdurbin avatar Sep 07 '22 14:09 pdurbin

Upgrading from Payara 5.2021.5 to 5.2022.3 made the IllegalStateException: Recursive update error go away. I noted the need to upgrade in the release notes in dd27401.

This is ready for QA.

pdurbin avatar Sep 07 '22 14:09 pdurbin

At standup I said I'm basically ready to merge this (maybe after poking a bit more) but as the reviewer I'm happy to let someone else take another look and merge it. As people free up, someone might grab it.

pdurbin avatar Sep 12 '22 15:09 pdurbin

At standup I said I'm still ready to merge this so I assigned myself. Sounds like we want to merge another PR first.

pdurbin avatar Sep 14 '22 17:09 pdurbin