Conversation
The JNDI names have been changed to be conform with Java EE 7. See https://github.com/javaee-samples/javaee7-samples/tree/master/jpa/datasourcedefinition-applicationxml-pu and others for working examples. (Staying with the old name was not successfull.)
…nection configuration. IQSS#7418
poikilotherm
added
Type: Feature
|
@poikilotherm code review sounds fine after you merge the latest from develop. Thanks. |
It looked like GitHub says it doesn't conflict. Should I update nonetheless? |
|
@poikilotherm I went ahead and merged the latest from develop into your branch in 813e0fb and put it in code review. |
src/main/resources/META-INF/microprofile-config.properties is important for developers to know about but sysadmins following the installation guide need not concern themselves with it. Other tweaks in this commit are cosmetic. I de-emphasized the environment variables by putting them in the bottom.
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
I tested this pull request on a fresh dev installation but I have not tested the upgrade scenarios presented in the release note so they should be tested. I also didn't test setting the new environment variables.
@donsizemore and I are aware that Jenkins is failing on this branch but we have a lead on how to fix it and it shouldn't require any changes to this pull request. In short we need to make sure that the installation scripts in this branch are used on the instance spun up by Jenkins.
| public class StartupFlywayMigrator { | ||
|
|
||
| @Resource(lookup = "jdbc/VDCNetDS") | ||
| @Resource(lookup = "java:app/jdbc/dataverse") |
There was a problem hiding this comment.
In 9dfe7b8 it is explained why this changed:
The JNDI names have been changed to be conform with Java EE 7.
See https://github.com/javaee-samples/javaee7-samples/tree/master/jpa/datasourcedefinition-applicationxml-pu
and others for working examples. (Staying with the old name was not successfull.)
| <include>**/*.xml</include> | ||
| <include>**/firstNames/*.*</include> | ||
| <include>**/*.xsl</include> | ||
| <include>**/*.properties</include> |
There was a problem hiding this comment.
This was added so the following file would be included:
src/main/resources/META-INF/microprofile-config.properties
Co-authored-by: Philip Durbin <philip_durbin@harvard.edu>
|
@poikilotherm we're seeing a number of failures on this branch (screenshot below) but not on develop. Any idea what's going on? We're happy to re-run the tests on your branch, of course, and help troubleshoot.
|
|
@poikilotherm I was able to deploy the app, create user and dataverse but ran into an exception when creating a dataset, perhaps due to ingesting a file. I'll try to refine it some more. Yes, was able to individually create a dataset, then upload text and image files. When I tried saving a tabular Stata file, I saw the error. [2020-12-01T16:49:53.994+0000] [Payara 5.2020.6] [INFO] [] [edu.harvard.iq.dataverse.ingest.IngestServiceBean] [tid: _ThreadID=89 _ThreadName=http-thread-pool::jk-connector(2)] [timeMillis: 1606841393994] [levelValue: 800] [[ [2020-12-01T16:49:54.082+0000] [Payara 5.2020.6] [WARNING] [] [javax.enterprise.system.core.transaction.com.sun.jts.CosTransactions] [tid: _ThreadID=89 _ThreadName=http-thread-pool::jk-connector(2)] [timeMillis: 1606841394082] [levelValue: 900] [[ [2020-12-01T16:49:54.085+0000] [Payara 5.2020.6] [WARNING] [] [javax.enterprise.system.core.transaction.com.sun.jts.jtsxa] [tid: _ThreadID=89 _ThreadName=http-thread-pool::jk-connector(2)] [timeMillis: 1606841394085] [levelValue: 900] [[ |
…finition plus add some more inline docs. IQSS#7418
…onitoring via MPCONFIG. IQSS#7418
…ONFIG. IQSS#7418 Due to payara/Payara#5024 this is currently not possible. Needs to be addressed in a later enhancement when Payara provides a solution.
Turns out I was a loudmouth. Payara does not support variables substitution in all property values, so this would need to be hardcoded as a workaround if absolutely necessary. I opened payara/Payara#5024 to enhance Payara and come back to this in a later enhancement to our beloved codebase. @kcondon @landreev @pdurbin could you folks please decide if a workaround is necessary? A quick patch is also possible, example in the code 😉 |
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
I'm suggesting that we remove MPCONFIG stuff that doesn't work.
src/main/java/edu/harvard/iq/dataverse/util/DataSourceProducer.java
Outdated
Show resolved
Hide resolved
|
@poikilotherm yes, I think it would be nice to understand the workaround for configuring things that we used to. I just left a review suggesting a concrete example: Locally I could try changing these settings, recompiling, and deploying. But how do I know they took effect? Is there a list-jvm-options or something? If it's easier to detect changes to other settings I'd be happy to use those instead. Again, see also the review I just left. The heart of it is that I think it's confusing to see MPCONFIG stuff in places where it doesn't work yet. Thanks. Update: I just played around with adding the properties above. In practice, it looks like this: Then I compiled and deployed the app but I don't know how to tell if these settings above are active. I searched a bit for an asadmin command and finally poked around in the Payara admin GUI (screenshot below) especially under the application (dataverse-5.2) and the JDBC stuff but I couldn't find anything. My understanding is that the properties above should show up somewhere under the application itself rather than at the app server level.
|
|
I had a nice chat with @poikilotherm this morning and I'm going to try testing |
- Show a simple properties example that's easily verifiable by inspecting server.log (JDBC call debugging). - Remove MPCONFIG examples that don't work yet (depends on work in Payara).
|
To test It would better to avoid having to compile, which is what payara/Payara#5024 is about. @kcondon and I had a nice chat and talked about how if things really go south, we can always put out a point release that more or less reverts this pull request, putting us back on the older configuration we've used for years. Also, an individual installation can recompile if need be, as mentioned above. Hopefully we simply won't see any problems. 😄 In addition to this little bit of testing and a lot of talking, I made a few tweaks to the javadoc and release notes. I'm sending this to QA. |
This rename happened in pull request #7422
5 participants
What this PR does / why we need it:
This will move the database connection configuration from the application server down to the application.
This greatly simplifies how to configure the connection details by using MicroProfile Config API.
Which issue(s) this PR closes:
Closes #7418
Requires #7416
Requires #7419
Requires #7420
Special notes for your reviewer:
None.
Suggestions on how to test this:
Be sure to apply the PRs mentioned above first. Follow release notes instructions. Do usual testing.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
🔋 included
Additional documentation:
None.