7000 mpconfig for JVM settings #8810
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 simplify SystemConfig.getVersion, the MicroProfile Config property dataverse.version is set to the Maven project version via filtering the variable.
Instead of trying to read a built time file from Maven, use MicroProfile Config to retrieve the version and build number. The version is by default set via microprofile-config.properties (or overridden by an env var in a container). The build number is still read from either BuildNumber.properties or, if not present, from MicroProfile Config, defaulting to empty. This also avoids copying extra files into containers to retrieve the version string.
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.
…#7000 By refactoring SystemConfig.getSolrHostColonPort, the Solr endpoint is not just configurable via a database setting, but also by all mechanisms of MicroProfile Config. - The database setting still has priority over the other mechanisms. - It's completely backward compatible, no config change necessary. - Tests have been added to ensure the behaviour - Default ("localhost:8983") for no setting given is now also done via MPCONFIG - Default for container usage ("solr:8983") possible via MPCONFIG profile "ct"
When using Dataverse with a non-default Solr, HTTPS, custom core name or similar, it's necessary to have a configurable URL for the Solr endpoint. This becomes now possible via MicroProfile Config, defaulting to the old variant.
Describe the new options to set the Solr endpoint, crosslinking the old way and adding hints about MPCONFIG profiles.
|
Note to self: find all |
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.
… MPCONFIG IQSS#7000 - Adding dataverse.files.directory equivalent to JvmSettings - Remove all System.getPropert("dataverse.files.directory") or similar - Add default with /tmp/dataverse via microprofile-config.properties as formerly seen at FileUtil and Dataset only - Refactor SwordConfigurationImpl to reuse the NoSuchElementException thrown by MPCONFIG - Refactor GoogleCloudSubmitToArchiveCommand to use the JvmSettings.lookup and create file stream in try-with-resources
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.
…CONFIG IQSS#7000 - Add both settings to JvmSettings to enable lookup - Refactor SystemConfig.getDataverseSiteUrlStatic to use MPCONFIG, but keep current behaviour of constructing the URL from FQDN or DNS reverse lookup. (Out of scope here, see IQSS#6636) - Replace clones of the method in Xrecord, DdiExportUtil, HandlenetServiceBean with direct usages of the static method to avoid unnecessary duplicated code. - Refactor SchemaDotOrgExporterTest with @JvmSetting for site url. - Remove unused constants from SystemConfig - Added default for container usage within "ct" profile, so we avoid extra lookups/settings for development usage. See also IQSS#6636
- Notes about MPCONFIG usage. - Rewording to make it more clear how this shall be used.
…CONFIG IQSS#7000 - Add scopes and settings to JvmSettings for strictness and fast retrieval with MPCONFIG - Rename settings to live under scope "dataverse.pid.ezid|datacite", so they are more aligned to our other settings. This could change again in the future, depending on what other devs feel where they should be located. - Add aliases to make settings backward compatible - in the current state of Dataverse code, only one PID provider can be used at the same time and the double alias is with the deprecated, non-public EZID provider. - Replace SystemConfig.getDataciteRestApiUrlString() with aliases and a default in microprofile-config.properties - Inline with the current setup and docs, make the DataCite MDS API URl default to the Test Fabrica, mds.test.datacite.org
poikilotherm
force-pushed
the
7000-mpconfig
branch
from
1e30c47
to
fd7a2c7
Compare
- Refactoring the DOI related JVM settings and cross links - Updated some facts and external links in their descriptions
Test execution in PersistentIdentifierServiceBeanTest requires to construct an EZID provider. The providers constructor tries to login with the service, which is not relevant for the test. Providing EZID configuration settings via a src/test/resource/META-INF/microprofile-config.properties file makes the warnings go away about missing config options and also avoids sending requests to the real service for every test execution.
poikilotherm
force-pushed
the
7000-mpconfig
branch
from
0ad3e01
to
4e41d25
Compare
|
Closing in favor of smaller scoped pull requests to avoid smoking reviewer brains. See |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This will introduce MicroProfile Config usage instead of any
System.getProperty()calls. This enables configuring the JVM settings via the different MPCONFIG in a backward compatible manner, but eases container usage a lot.However, it will not interfere with Database Settings in this iteration, to have a more focused, smaller step to take. There is one exception: the Solr host and port will be retrievable from MPCONFIG as well as database. (
SystemConfig.getSolrHostColonPort())Which issue(s) this PR closes:
Closes #7680
Closes #7000
Special notes for your reviewer:
Not ready yet.
Suggestions on how to test this:
Will provide unit tests. For more manual testing, you could use another config source with Payara.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope, this is sysadmin only.
Is there a release notes update needed for this change?:
Yes. Batteries will be included.
Additional documentation:
System.getProperty()by accident