[DCJ-575] Dataset obtained for ingest flight needn't have relationships or assets #1754
Conversation
These are obtained via costly database calls.
okotsopoulos
changed the title
|
There was a problem hiding this comment.
These changes would be good fits for a @ParameterizedTest but we haven't yet converted our @EmbeddedDatabaseTests to JUnit 5 so I'm designating that improvement OOS.
okotsopoulos
requested review from
rushtong and
fboulnois
and removed request for
a team
| Dataset dataset = null; | ||
| try { | ||
| if (summary != null) { | ||
| summary.storage(storageResourceDao.getStorageResourcesByDatasetId(summary.getId())); | ||
| dataset = new Dataset(summary); | ||
| dataset.tables(tableDao.retrieveTables(dataset.getId())); | ||
| relationshipDao.retrieve(dataset); | ||
| assetDao.retrieve(dataset); | ||
| if (retrieveRelationship) { |
There was a problem hiding this comment.
I'm wondering why these weren't joins in the first place instead of separate db calls. I also wonder how many other times we're making multiple db calls in a case where a single query could be used.
But I agree that this is probably the simplest fix for this problem right now
| * @param id in UUID format | ||
| * @return a Dataset object | ||
| */ | ||
| public Dataset retrieveForIngest(UUID id) { |
There was a problem hiding this comment.
Now that we have a way to get a dataset without relationships, could we put this dataset in the flight map? It looked like the problem with serialization before was due to the relationship structure, due its recursive definition.
There was a problem hiding this comment.
I think we can. There were concerns raised in Datahood Design Discussions that loading the dataset into the flight map could be problematic if the dataset changes between its storage in the map and its usage later on in the flight… but we could guard against that by loading it into the working map after locking the dataset.
I thought this would be worth pursuing if / when we modify the dataset types as discussed below.
| @@ -483,6 +483,11 @@ public boolean delete(UUID id) { | |||
| return rowsAffected > 0; | |||
| } | |||
|
|
|||
| public Dataset retrieve(UUID id, boolean retrieveRelationship, boolean retrieveAsset) { | |||
There was a problem hiding this comment.
I think it's a hazard to have a single "dataset" object whose contents may be initialized differently depending on how it's created, as we don't keep track of which type of initialization was used.
A stricter approach would either store the retrieval flags in the dataset and then throw if an invalid getter was called, or (even better) having different dataset types depending on which fields are present, so invalid access would be known at compile time. Both of these are out of scope for this PR though.
There was a problem hiding this comment.
I agree, I could see this biting us down the line while also seeing the current approach as good for now. I'd like to talk with you more about alternative approaches here -- I had also thought about different dataset types but would appreciate a thought partner to solidify what that would look like.
There was a problem hiding this comment.
I agree with many of @pshapiro4broad 's suggestions but I think they're out of scope for this PR. In terms of a much more narrow scoped PR I think this one is in good shape!
|
Thanks all for the reviews and feedback -- @pshapiro4broad , @fboulnois , @rushtong . I've filed this follow-on ticket for future work and will merge this PR now: https://broadworkbench.atlassian.net/browse/DCJ-600 |
Jira ticket: https://broadworkbench.atlassian.net/browse/DCJ-575
Addresses
By a long shot, the highest drivers of database load during this Prod incident were queries to construct dataset assets and relationships, called as part of constructing full dataset objects from the DB. We deduced that the high number of calls happened while facilitating a high number (300+) of concurrent ingest flights. The size of the ingests had no bearing on this load.
Dataset assets and relationships are not needed to facilitate ingestion, so that load on the database is unnecessary even before you consider whether the queries themselves could be optimized.
As we continue to see DB CPU utilization spikes up to 100% despite doubling our Prod DB's available CPU, we want to pursue an attainable optimization ASAP. Further iteration may be explored later, but my priority with this PR is to get us more breathing room to prioritize such work thoughtfully.
Summary of changes
IngestUtils.getDataset(FlightContext, DatasetService)still constructs the latest version of the Dataset from the DB, but now avoids the costly DB calls to construct relationships and assets.Testing Strategy
Expanded unit tests.
Examined Query Insights for integration test runs (which exercise ingestion, though not at high levels of concurrency) and observed reduced DB CPU utilization attributed to relationship and asset construction.
Integration tests off of this PR:
Integration tests off of
developbranch (control):