search

icatproject / ids.server

The server component of the ICAT Data Service
Other
3 stars 3 forks source link

Slow SOAP queries with INCLUDEs #117

Open kevinphippsstfc opened 3 years ago

kevinphippsstfc commented 3 years ago

Whilst investigating the long running queries issue (#115) I found that there are two queries in the resolveDatasetIds method of DataSelection using the SOAP API, both of which request a Dataset including the Investigation and Facility, that are very slow. These are on the following lines:

https://github.com/icatproject/ids.server/blob/v1.11.0/src/main/java/org/icatproject/ids/DataSelection.java#L110 https://github.com/icatproject/ids.server/blob/v1.11.0/src/main/java/org/icatproject/ids/DataSelection.java#L130

and when run against the Diamond preprod ICAT they take 2 seconds to complete. This is very repeatable - not randomly slow every so often.

I found that when I request only the required fields using the ICAT REST client using the query:

SELECT ds.id, ds.name, ds.location, inv.id, inv.name, inv.visitId, fac.id, fac.name FROM Dataset ds JOIN ds.investigation inv JOIN inv.facility fac WHERE ds.id=?

the query only takes about 80ms.

Whilst 2 seconds per query might not sound a lot, as with #115 there is a timeout of 30 minutes to generate a preparedID, so without allowing time for anything else, this already limits the number of Datasets that can be requested to 900 (30x30). On the Diamond ICAT we do have users requesting hundreds of Datasets (partly because we don't allow them to select whole Investigations) so this has the potential to be a problem.

At the same time as converting these two queries to use the REST client, that would only leave 2 remaining uses of the SOAP client in this class (on lines 118 and 125) so it would be worth converting these at the same time. That would mean that the horrible double section of exception handling on line 152 could be reduced to just one section for the REST client.

A batch of import statements for the SOAP client classes could also be removed.

Once this has been done, there would be no need to pass the SOAP client into the constructor for DataSelection, thus making that a bit simpler and tidier as well.

RKrahl commented 3 years ago

Many thanks for pointing this out! What you describe here is a small portion of what I called "awkward code" in #109.

RKrahl commented 3 years ago

As discussed with @kevinphippsstfc, we will postpone that one in favour of proceeding with the release of 1.12.