log file with a different instance root directory

[BenediktMeierUIT]

What this PR does / why we need it:
log file with a different instance root directory
Which issue(s) this PR closes:
Closes #10372

Special notes for your reviewer:

Suggestions on how to test this:

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

[pdurbin]

@BenediktMeierUIT thanks for the pull request. Out of curiosity, what is your instance root?

[Louis-wr]

@BenediktMeierUIT thanks for the pull request. Out of curiosity, what is your instance root?

Benedikt as been working with us on dataverse.no

[pdurbin]

Cool. I'm still wondering what you use as an instance root, but no rush. 😄 Oh! Maybe you're using Docker? 🤔

We recently make a similar change at 8efb964 and as discussed in Slack, we know there are more places to fix, as you've done. Thanks!

[BenediktMeierUIT]

Yes, this is a Docker.
And I solved the errors straight away.
8efb964

I'm not quite sure about another point. Why it was done differently.

dataverse/src/main/java/edu/harvard/iq/dataverse/DatasetVersionServiceBean.java

Line 896 in ec1b174

String logDir = System.getProperty("com.sun.aas.instanceRoot") + SEP + "logs" + SEP + "edit-drafts" + SEP;

dataverse/src/main/java/edu/harvard/iq/dataverse/batch/util/LoggingUtil.java

Line 70 in ec1b174

String logDir = System.getProperty("com.sun.aas.instanceRoot") + SEP + "logs" + SEP + "process-failures" + SEP;

(I don't have access to Slack :-( )

[pdurbin]

Sorry, can you please spell out a little more how it's done differently? From a quick look it seems more or less the same to me.

[BenediktMeierUIT]

Sorry...
It's the same, but it's inconsistent.
My question was/is, what is the guideline and how do you want it?

[BenediktMeierUIT]

At that point, it's done differently again:

dataverse/src/main/java/edu/harvard/iq/dataverse/harvest/client/HarvesterServiceBean.java

Line 91 in 5caf436

public static final String DATAVERSE_HARVEST_STOP_FILE="../logs/stopharvest_";

I was going to change the line like this:
public static final String DATAVERSE_HARVEST_STOP_FILE="stopharvest_";
and in the place:

dataverse/src/main/java/edu/harvard/iq/dataverse/harvest/client/HarvesterServiceBean.java

Line 402 in 5caf436

String stopFileName = DATAVERSE_HARVEST_STOP_FILE + harvestingClient.getName() + "." + pid;

String stopFileName = System.getProperty("com.sun.aas.instanceRoot") + File.separator + "logs" + File.separator + DATAVERSE_HARVEST_STOP_FILE + harvestingClient.getName() + "." + pid;

[poikilotherm]

This should be a JVM option defaulting to the system property as well as made consistent everywhere.

[pdurbin]

It's funny, just today I was in DatasetServiceBean.java debugging something and I copied the fix from this PR because I'm in Docker and was hitting a problem without it.

I'd say we definitely want this fix.

And yes, a JVM option/MicroProfile Config setting eventually. In this PR or later.

[Louis-wr]

Yes, A JVM option/MicroProfile Config would be nice in the future but for now we should avoid overreaching this pull request and keep it confine to the bug fix that it is.
In the near future once this is fixed it will be easy to replace all the System.getProperty("com.sun.aas.instanceRoot") with the appropriate newly created JVM option/MicroProfile Config.

[pdurbin]

Yes, sounds good to defer adding a JVM/MPConfig option until later.

[poikilotherm]

Adding the option is almost a one liner. Do you want me to create a small PR to this PR with it? @pdurbin if you leave a review after, I can do QA and we ship this one fast.

[BenediktMeierUIT]

If you give me the JVM/MPConfig name, I can change the format to what you want.

[pdurbin]

Sure, I'm happy to do a review and let @poikilotherm do the QA.

[pdurbin]

I can vouch for the need for this change. Without it, I couldn't get some code I was playing with to execute. I was exporting some JSON for troubleshooting: pdurbin@8adb28d

[poikilotherm]

Sounds like within a container, the application has a different working directory than in a classic deployment. Which means ../logs may point to some place that does not exist / the app user has no permissions to write to.

[poikilotherm]

With the explanation via DM and in https://github.com/IQSS/dataverse/pull/10373/files#r1535665937, I understood better why this change is necessary.

As we should be thorough with this change, can you fix the missing occurrences @BenediktMeierUIT ?

Those would be in dataverse.authorization.AuthFilter and dataverse.harvest.client.HarvesterServiceBean.

[coveralls]

coverage: 20.659%. remained the same
when pulling a2c50b6 on DataverseNO:develop
into 30666f9 on IQSS:develop.

[BenediktMeierUIT]

That's right...
I have changed it.

[poikilotherm]

Looks good to me now, thanks for the change! @pdurbin stage is yours 😄

[pdurbin]

@BenediktMeierUIT for next time, please do not use "develop" as the name of your branch. It make it hard to review and test as described in this issue:

• Document how to update a "develop branch to develop branch" pull request  #9508

[pdurbin]

Tests are passing as as I mentioned above these changes are very similar to changes I made in pdurbin@8adb28d

Merging.