Write batches of GuestBookResponses for performance

[qqmyers]

What this PR does / why we need it: This PR combines a new method to write batches of guestbook entries, versus saving each one separately with the changes from #12110 and #12319, and in doing that, extends Locally FAIR support to the external zipper code. It also switches to using a custom query to get the file id list for a dataset version rather than iterating through all filemetadatas to get datafile objects to get ids.

Which issue(s) this PR closes:

• Closes UI Downloads do not work in datasets with a locally FAIR enabled Dataverse #12579
• Closes When there is a guestbook, you cannot download files using a Preview URL #12535
• Closes Allow Preview User to download files without requiring a Guestbook Re… #12584

Special notes for your reviewer: I've tried to verify that LocallyFAIR constraints are imposed on all the download get calls.

One oddity I saw is that the handleCustomZipper call appears to have forced orig to true while the code to set up the job (e.g. in fileDownloadService.addFileToCustomZipJob seems to handle both. I'm not sure why that should be - perhaps @landreev can comment - this PR changes it to pass the original flag through but I can revert that if needed.

There is also some cleanup of things in #12110 - in the downloadDatafiles method, the String body sent in was parsed as the list of fileIds and then, later in the same method (old line 1095) as the guestbook response. I think the second use would never have happened. I also stopped making a Map that was only used to get the List of values, avoiding repeat conversions of csv strings to Lists and back again, etc.

Suggestions on how to test this: It should pass all the existing tests. Writing guestbook entries for datasets with many files should be faster - seen at QDR with a 10K file dataset.

[edit:] This pr closes the same 2 issues as, and should supersede #12584. So it needs to be tested against the scenarios described there. Copy-and-pasting the "how to test" line from that pr: Create a dataset with a guestbook. Generate a Preview URL. Using the preview url try to download files and dataset zip file. This should work without requiring the guestbook response. [L.A.]

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:

[landreev]

Let me take a closer look... It is entirely possible that this was just a bug - that it always defaulted to the original, and nobody noticed over the years. Although I do remember users asking questions specifically about the handling of tabular files in the output of the zipper; so it must have been working at least at some point.

[landreev]

Let me take a closer look... It is entirely possible that this was just a bug - that it always defaulted to the original, and nobody noticed over the years. Although I do remember users asking questions specifically about the handling of tabular files in the output of the zipper; so it must have been working at least at some point.

Huh, it really is a bug that does just that - always defaults to "format=original". But ONLY when the zipper is accessed via the API. When bundles are downloaded from the jsp pages, you get either the originals or archival tabs, as requested.
/api/access/datafiles/<list> appears to be a somewhat rarely used api at HDV.

[landreev]

@qqmyers Is the pr still work in progress - or is it "ready for triage"?

[qqmyers]

~yes - it probably needs a release note and I wanted to make sure all the IT tests passed, along with getting your nod on the orig zip part, but yes, it could probably move into review w/o waiting for any of that.

[coveralls]

Coverage is 25.01% — GlobalDataverseCommunityConsortium:WriteMultipleGBsAtOnce into IQSS:develop. No base build found for IQSS:develop.

[qqmyers]

@landreev - back to you. In addition to trying to address the comments/open issues, I merged with #12584 and further updated the logic (allowing acess POST calls, avoiding a permission check), and created a custom query to avoid getting the guestbookresponse and calling gbr.getDataset().getId(); I also fixed a bug w.r.t. the gbr eventType not getting set.

As far as I can tell watching a debugger, calling downloads is working, checkGuestbookRequired follows the expected paths when I'm logged in/logged out, draft, previewUrlUser, etc.

[landreev]

Approving.

[landreev]

Testing on qa.
Just tried downloading the infamous 2979-tiny-files dataset all at once. The 2979 GBRs were written successfully (will need to re-measure how long that took exactly; but it felt acceptable). The timestamp is exactly the same on all 2979. The /api/access/datafiles did throw a 500 in the end however. That does NOT happen on all datasets - I can download ones with fewer files. Need to do more testing, will report.

[landreev]

Scratch the 500 part; it said "Internal Server Error" in the browser window, but it was logged as a 200 in the access log. Will continue testing tomorrow.

Testing on qa. Just tried downloading the infamous 2979-tiny-files dataset all at once. The 2979 GBRs were written successfully (will need to re-measure how long that took exactly; but it felt acceptable). The timestamp is exactly the same on all 2979. The /api/access/datafiles did throw a 500 in the end however. That does NOT happen on all datasets - I can download ones with fewer files. Need to do more testing, will report.

[landreev]

The Internal Server Error from /api/access/datafiles reported last night is some kind of a proxy (Apache) level timeout. Works fine when going to :8080. I am close to signing off on and merging the PR.

(but, as an aside, I am beginning to think that the "download all as zip" option under Access Dataset should be conditional not just on the size, but also the number of files in the dataset).

[landreev]

Under this branch, the GuestbookResponses get written at 0.009 sec./gbr on avg (on perf db). Or 25+ sec. for the 2979 files download-all test case.
Under the develop branch, same instance/same database: unlike in the test above, each gbr has its own timestamp; 0.078 sec./gbr, 233 sec. for 2979 files (that includes the extra 2979 INSERT INTO ACTIONLOGRECORD queries after each individual CreateGuestbookResponseCommand).
Confirming 10X improvement.

Downloads appear to be working. Including a less trivial case of /api/access/datafiles where the file ids are sent in a POST.

Merging.

[landreev]

Merging.

@qqmyers Sorry, could you please resolve the merge conflict above.

[landreev]

Just to be OCD, changed "plus there are ..." to "that includes the extra 2979 ... queries" in the end result paragraph.

[landreev]

And, boom, it did in fact close #12584 automatically.

[landreev]

Having measured how long it used to take to write these GBRs, I am having some bad suspicions about what the millions of recorded downloads really mean for this 2979-tiny-files dataset in prod. :(

[landreev]

@qqmyers (just some extra observations, nothing wrong with the merged pr) There is an extra wrinkle re: the net improvement we will see in our prod. environment here. Since we are using the "zipper", we will still be writing the N insert into customzipservicerequest queries before issuing the redirect. These are less expensive than the guestbookresponse equivalents (the table is close to empty at any given time), but it is on a comparable scale. So I should just make a pr to replicate what you're doing for GBRs for these things as well.

As another aside, the zipper is a hack, etc. etc. But it definitely has another serious advantage (aside from offloading long-running and expensive tasks from the main app): The /api/access/datafiles has an overhead of having to perform N permission checks - which the page already had to do before issuing the redirect - and it looks like it has to do that before it starts producing any output. Which on many prod. systems would result in a proxy timeout for a large multi-file download. (Which may/will likely not be the case with the page "thinking" before issuing the redirect in response to a partial submit). The zipper of course starts spitting out bytes right away.

Under this branch, the GuestbookResponses get written at 0.009 sec./gbr on avg (on perf db). Or 25+ sec. for the 2979 files download-all test case. Under the develop branch, same instance/same database: unlike in the test above, each gbr has its own timestamp; 0.078 sec./gbr, 233 sec. for 2979 files (that includes the extra 2979 INSERT INTO ACTIONLOGRECORD queries after each individual CreateGuestbookResponseCommand). Confirming 10X improvement.

Downloads appear to be working. Including a less trivial case of /api/access/datafiles where the file ids are sent in a POST.

Merging.

[qqmyers]

I think those inserts are ~OK - the addFileToCustomZipJob isn't it's own transaction (unless I missed it).

[landreev]

I haven't even looked at the code yet, I just saw that it took a little over 2 min. on qa, between the first and the last of the ~3K zipper INSERT queries. I took a closer look at the postgres log since then, and there is not much happening between these INSERTs either. ... but I am seeing one thing already that I may be able to speed up a bit.

[landreev]

Each zipper INSERT does honestly cost roughly what the individual GBR used to cost prior to this PR. It looks like the savings came from subcontracting a REQUIRES_NEW EJB method to do the persisting of the GBRs. I really just need to replicate that w/ the zipper entries.
But it also looks like I am doing something that results in 2 extra queries between each 2 INSERTs, at least one of which is also measurably expensive. Something I will have to address with a native query.