Performace problem in multi-file download API (GuestBookResonse)

[landreev]

We have a dataset with 15 files being replaced over 200 versions. (200 x 15 dvobjects; only 15 files in each version). We've known that datasets with many versions are expensive to operate on. So when a user attempts to do a full download, via "Access Dataset" on the page, there's a 10 or so sec. delay before the redirect to the zipper is issued.
However, if a user attempts the same download via /api/access/dataset/..., the delay before the redirect becomes ~ 15 * 10 sec. On close inspection, it is this call in the multi-file download method, repeated for every file, that is responsible for that increase:

   GuestbookResponse gbr = guestbookResponseService.initAPIGuestbookResponse(file.getOwner(), file, session, apiTokenUser);
   guestbookResponseService.save(gbr);

Since the method in FileDownloadService creates the same guestbook entries without causing this delay, when the download is initiated from the page, there has to be an easy way to avoid it in the API as well.

As described above, the scenario involves the external zipper. Writing these guestbook entries will cost the same amount of extra time without it in place. The difference however is that when the application zips the content itself, the guestbook entry is generated for each file as it's being streamed. Meaning, the performance would still be atrocious. But there would not be a combined, continuous idle wait, long enough to cause a timeout on the load balancer. Such as the case was in our prod. instance.

[landreev]

One thing to keep in mind as we optimize this: we should NOT assume, inside that mutlti-file download method in the API, that all the files are from the same dataset. Nobody stops a user from sending some ids to /api/access/datafiles/… that are from different datasets. But in the context of download-all they sure are.

[landreev]

It looks like it's even crazier - commenting out the GuestbookResponseService.save() line there doesn't speed it up either. So it's just the initAPIGuestbookResponse() where something expensive is happening - ?

   GuestbookResponse gbr = guestbookResponseService.initAPIGuestbookResponse(file.getOwner(), file, session, apiTokenUser);
   /*guestbookResponseService.save(gbr);*/

[landreev]

We want to keep in mind that there is a possibility that this isn't just the sheer number of versions, but something with our prod. database; some kind of an inconsistency that may be causing this. So far, the only way this has been reproduced outside of prod. is on a copy for prod. db.
With that in mind, when tested with a modified 5.4.1 build, on that prod. db copy, the problem can be further isolated to this line:

file.getOwner().getLatestVersion();

which kind of makes sense; so replacing that with a native query; or at least caching the latest version and not looking it up for every file every time would fix this.
However, we do want to figure out what exactly is going on (would this be the case for any dataset with 200 versions? or does it need to have this many dvobjects? and/or is this something unique to our prod. db)?
Having a getLatestVersion() method guaranteed to work fast for any dataset would also be nice.

[landreev]

As I mentioned on Thursday, I had that preliminary proof-of-concept fix mostly ready before I realized what that latest version inside the GuestBookResponse method was actually used for (just to make the decision whether to record the response - since we don't want to count downloads from drafts). So instead of passing that version to the method, as I'm currently doing in the branch, we should just make that decision earlier, before that method is called.
And we don't even need to pass it as a boolean to the method either. Note that this is what happens inside the method:

if(version != null && version.isDraft()){
   guestbookResponse.setWriteResponse(false);
}

i.e., we create a guestbookresponse object... but if the version is draft, we mark it as not worthy of being saved in the database. It will be easier to just not call this method at all, if we don't want to save this guestbook response. (I'm assuming this "setWriteResponse" logic is a leftover from the use case for which the code was originally developed - for saving guestbook responses for downloads made via UI pages; where this response objects may be passed between the page and the popups, so it made more sense there).

[landreev]

performance numbers, testing with a copy of the prod. db:
stock 5.4.1: 105 seconds (best result), 10200+ queries.
patched 5.4.1: 1.5 seconds, 1200 queries

[pdurbin]

@landreev and I have been chatting but I wanted to share some numbers more broadly.

I've been testing "download all" via curl with a dataset with 200 versions and 15 tabular files that are replaced with each version.

The 7812-download-all-api-speedup branch definitely reduces the number of queries, from 10376 to 1152. For the details, please see the following:

• old-c5c210afa.txt
• new-ad341af7c.txt

I thought I'd also attach include the REST Assured code I used to create the dataset above: RestAssured.txt

Finally, here's a compressed dump of my database which might come in handy since it took so long to create this dataset with 200 versions: dvndb.sql.txt

[djbrooke]

Thanks @landreev and @pdurbin! Love to see the numbers drop. :)

[landreev]

On an even closer inspection, it was easy to simplify it even further (than described above).
That check inside the initAPIGuestbookResponse() method was simply redundant, since it was already only called like this throughout Access.java:

if (gbrecs != true && file.isReleased()) {
   GuestbookResponse gbr = guestbookResponseService.initAPIGuestbookResponse(file.getOwner(), file, session, apiTokenUser);
   ...

i.e., we already had that boolean, gbrecs, indicating that the guestbook response should not be written. (I actually renamed it donotwriteGBResponse for clarity). And it's easy to ensure that it is set to true upstream, whenever we know that the api is being called on a draft.