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.

Sign up for GitHub

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

Jump to bottom

7000 mpconfig rserve #8830

Merged
merged 12 commits into from
Dec 21, 2022
Merged

7000 mpconfig rserve #8830

merged 12 commits into from
Dec 21, 2022

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Jul 4, 2022 •
edited
Loading

What this PR does / why we need it:
Now also changing the Rserve configuration to be MPCONFIG enabled. Also making sure updated settings would be used for every new file read during ingest.

Which issue(s) this PR closes:

None.

Special notes for your reviewer:
None?

Suggestions on how to test this:
There are no tests. How have test for Rserve integration been done so far?

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?:
IMHO no.

Additional documentation:
Doc updates included.

@poikilotherm poikilotherm added Type: Feature a feature request Component: Code Infrastructure formerly "Feature: Code Infrastructure" Feature: Installation Guide User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh labels Jul 4, 2022
@poikilotherm poikilotherm added the Component: Containers Anything related to cloudy Dataverse, shipped in containers. label Jul 4, 2022
@poikilotherm
feat(settings): add rserve properties via MPCONFIG IQSS#7000
cbc7f8a
@poikilotherm
refactor(rserve): introduce MPCONFIG settings retrieval IQSS#7000
6732b4b 
1. Instead of reading the configuration from system properties only,
   switch to using MPCONFIG and JvmSettings fluent API.
2. Instead of saving the configuration in a static variable, retrieve
   the config from the constructor.
   This has 2 advantages: 1) no worries about execution order and
   MPCONFIG not yet ready, 2) update the readers with new config
   settings when changed (no need to restart).
@poikilotherm
fix(settings): align Rserve tempdir default to docs IQSS#7000
d951f99 
The docs said the default is "/tmp/Rserve", while the code had
"/tmp". Changing the code default to the documented one.
@poikilotherm
docs(settings): add Rserve MPCONFIG to guide IQSS#7000
507ae82
@coveralls
Copy link

coveralls commented Sep 19, 2022 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 20.011% when pulling 6a1984d on poikilotherm:7000-mpconfig-rserve into e6c033b on IQSS:develop.

@mreekie mreekie added the bk2211 label Nov 1, 2022
@pdurbin pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Dec 2, 2022
@mreekie
Copy link

mreekie commented Dec 14, 2022

added to sprint Dec 15, 2022

pdurbin
pdurbin approved these changes Dec 19, 2022
View reviewed changes
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.

I didn't test this but the code changes make sense, all the API tests are passing, and the docs look good: https://dataverse-guide--8830.org.readthedocs.build/en/8830/installation/config.html#dataverse-rserve-host

Moving to QA.

@kcondon kcondon self-assigned this Dec 19, 2022
@kcondon
Copy link
Contributor

kcondon commented Dec 19, 2022 •
edited
Loading

@poikilotherm @pdurbin Not sure how this should fail but previously, it uploaded the original file. Now it throws a server log error but leaves the user on the grayed out "action" screen and if you click on it, shows upload page with no files, so a silent error.

It might be due to the type of failure -a type validation issue:
[2022-12-19T15:46:52.833+0000] [Payara 5.2022.3] [WARNING] [AS-EJB-00056] [javax.enterprise.ejb.container] [tid: _ThreadID=96 _ThreadName=http-thread-pool::jk-connector(5)] [timeMillis: 1671464812833] [levelValue: 900] [[
A system exception occurred during an invocation on EJB IngestServiceBean, method: public void edu.harvard.iq.dataverse.ingest.IngestServiceBean.startIngestJobsForDataset(edu.harvard.iq.dataverse.Dataset,edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser)]]

[2022-12-19T15:46:52.833+0000] [Payara 5.2022.3] [WARNING] [] [javax.enterprise.ejb.container] [tid: _ThreadID=96 _ThreadName=http-thread-pool::jk-connector(5)] [timeMillis: 1671464812833] [levelValue: 900] [[

javax.ejb.EJBException: java.lang.NumberFormatException: For input string: "x6311"

@poikilotherm
Copy link
Contributor Author

poikilotherm commented Dec 19, 2022 •
edited
Loading

Hey @kcondon this is really strange... where is that "x" coming from? It's clearly not added by the code...

Is there some strange setting for the port in your domain configuration?

@kcondon
Copy link
Contributor

kcondon commented Dec 19, 2022 •
edited
Loading

So to keep track:

  1. Fix doc for tempdir per Leonid
    [Kevin] Fixed.
  2. Port failure with non numeric in value (x6311) returns user to Upload url with gray overlay rather than to dataset page.
  3. Deleted user setting does not use default but fails the same as in 2.
    [Kevin] Fixed.

@poikilotherm
Copy link
Contributor Author

poikilotherm commented Dec 19, 2022 •
edited
Loading

  • 1. Fix doc for tempdir per Leonid
  • 2. Port failure with non numeric in value (x6311) returns user to Upload url with gray overlay rather than to dataset page.
  • 3. Deleted user setting does not use default but fails the same as in 2.

Need input for (2) from @landreev @pdurbin et al.

Thanks y'all for your patience and thorough testing!

@pdurbin
Copy link
Member

pdurbin commented Dec 20, 2022

@poikilotherm can you please resolve the merge conflicts? Thanks!

@poikilotherm
Merge branch 'develop' into 7000-mpconfig-rserve
04b6326
@poikilotherm
fix(rserve): avoid disfunctional JSF page when invalid Rserve port IQ…
6a1984d 
…SS#7000

When an invalid port was set, which cannot be transformed to an integer,
the JSF page would kind of freeze: it leaves the user on the grayed out
"action" screen and if you click on it, shows upload page with no files,
so a silent error.

By defaulting to 6311 also for this case and logging an error, this
should improve the UX, but leave a hint for an admin in the logs.
@kcondon kcondon merged commit 6154aac into IQSS:develop Dec 21, 2022
@poikilotherm
Copy link
Contributor Author

Thanks for going through all the trouble with me @kcondon! Glad this is done. Another step! ❤️

@poikilotherm poikilotherm deleted the 7000-mpconfig-rserve branch December 21, 2022 15:38
@pdurbin pdurbin added this to the 5.13 milestone Dec 21, 2022
@mikeaintworkin mikeaintworkin mentioned this pull request Jan 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@landreev landreev landreev left review comments

@pdurbin pdurbin pdurbin approved these changes

Assignees

@kcondon kcondon

Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure" Component: Containers Anything related to cloudy Dataverse, shipped in containers. Feature: Installation Guide Size: 3 A percentage of a sprint. 2.1 hours. Type: Feature a feature request User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh
Projects
IQSS Dataverse Project
Status: No status
Milestone
5.13
Development

Successfully merging this pull request may close these issues.
6 participants
@poikilotherm @coveralls @mreekie @kcondon @pdurbin @landreev