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

7000 mpconfig basic infra #8823

Merged
merged 35 commits into from
Sep 14, 2022
Merged

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Jun 30, 2022

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.

It doesn't seem we are going to use MPCONFIG as the
primary source of settings from the database for now.
This commit removes the DbConfigSource which has had
errors, too.

Fixes IQSS#7680
The microbean implementation of MicroProfile Config
has not been updated to MPCONFIG 2.0. Switching to
the smallrye implementation for testing scope.
To be able to have more control over JVM settings names,
avoid typos, maybe create lists of settings and so on,
this enum will provide the place to add any old and new
settings that are destined to be made at the JVM level.

Further extensions of this class include adding aliases
when renaming settings, adding predicates for validation
and offering injecting parameters into keys (as used with
the files subsystem).
To provide built-time static settings, property files
under src/main/resources are now filtered by maven to
replace any variables ${propertyname} known by Maven
with their values before storing the file under /target.
To add JVM settings (system properties) during a test,
this helper annotation ensures one is not using a wrong
key and the setting is deleted after the test to avoid
interfering with other tests.
Retrieving a setting as a simple String is a very often
use case within the codebase. To make these lookups easier
to write and comprehend, adding convenience methods here.

These are simple wrappers around the MicroProfile Config lookup.
Save some CPU cycles for negligible memory consumption.
- When renaming older JVM settings, these should be still retrievable from an old config
- The AliasConfigSource now takes aliased settings from JvmSettings for this
- This is just for pure renaming but could be further extended with data manipulation.
Instead of only receiving a setting as String via the fluent lookup API,
also allow to pass a target type to make use of Converters.
To simplify SystemConfig.getVersion, the MicroProfile Config
property dataverse.version is set to the Maven project version
via filtering the variable.
@poikilotherm poikilotherm added Type: Feature a feature request Component: Code Infrastructure formerly "Feature: Code Infrastructure" Feature: Developer Guide Feature: Admin Guide User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh labels Jun 30, 2022
@poikilotherm poikilotherm added the Component: Containers Anything related to cloudy Dataverse, shipped in containers. label Jun 30, 2022
@poikilotherm poikilotherm changed the title 7000 mpconfig infra 7000 mpconfig basic infra Jun 30, 2022
When the alias config properties file cannot be read and an IOException is thrown,
the underlying reason will be logged in addition.
…aliases

Before, the aliases from JVM settings enum where only read when the alias
config file was successfully read. This made no sense - if this file is missing,
we can still properly operate on the aliases from JvmSettings.
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Aug 5, 2022

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 🙃

@scolapasta
Copy link
Contributor

@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...)

@pdurbin pdurbin self-assigned this Sep 1, 2022
Emphasize that the change is for devs for now. The idea is to migrate
settings to the new framework over time.
@pdurbin
Copy link
Member

pdurbin commented Sep 6, 2022

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: pdurbin@898dce1

@pdurbin
Copy link
Member

pdurbin commented Sep 7, 2022

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

pdurbin commented Sep 7, 2022

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.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

See comments in the PR. No real change happens and devs should be ready to use "lookup" once they upgrade Payara.

@pdurbin pdurbin removed their assignment Sep 7, 2022
Co-authored-by: Oliver Bertuch <poikilotherm@users.noreply.github.com>
@pdurbin
Copy link
Member

pdurbin commented Sep 12, 2022

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 pdurbin self-assigned this Sep 14, 2022
@pdurbin
Copy link
Member

pdurbin commented Sep 14, 2022

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 pdurbin merged commit 1c72fbe into IQSS:develop Sep 14, 2022
@pdurbin pdurbin removed their assignment Sep 14, 2022
@pdurbin pdurbin added this to the 5.12 milestone Sep 19, 2022
@poikilotherm poikilotherm deleted the 7000-mpconfig-infra branch May 21, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure" Component: Containers Anything related to cloudy Dataverse, shipped in containers. Feature: Admin Guide Feature: Developer Guide Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microprofile recursive update exception when trying to use the Alias/DBSetting sources
4 participants