NCEAS / metacatui

MetacatUI: A client-side web interface for DataONE data repositories
https://nceas.github.io/metacatui
Apache License 2.0
42 stars 28 forks source link

Refactor package loading pattern to avoid calls to /meta and /object #580

Closed amoeba closed 5 years ago

amoeba commented 6 years ago

I have a deeply nested package on test.arcticdata.io at https://test.arcticdata.io/#view/urn:uuid:dd7d8b46-04ce-48bf-b335-005fe7a83063. I just visited that page and noticed the package table took a really long time to show up. I then popped open Chrome devtools and saw an endless stream of XHR requests being sent out. test.arctic appears to be running MetacatUI 2.0.0 RC5.

kapture 2018-04-19 at 10 47 10

A lot of the calls early on are to solr but then the requests eventually become calls to /meta and /object for a ton of different PIDs. I suspect that these /meta and /object calls are for the Objects contained in this package's hierarchy (of which there are many). As of 3 minutes in, I'm at 1462 requests and they're still going strong. If I look for the source of the XHR calls, I see this:

https://github.com/NCEAS/metacatui/blob/838c35716a4667007990e9da8368fb29aeafd42f/src/js/collections/DataPackage.js#L454-L472

Correct me if I'm wrong, but I think this code calls /meta and /object.

I'm going to keep looking at this a bit but any and all help would be appreciated.

csjx commented 6 years ago

Sure - this is a scaling issue, and the intention in the future is to lazy load sub packages (sub folders in the table) when someone clicks to open the subfolder. At this time, we populate the DataPackage fully, which as you point out isn't so performant :) I'd say we can turn this into a bug for refactoring the package loading pattern.

amoeba commented 6 years ago

Makes sense to me. This package is one we need to migrate to arcticdata.io and I'm worried about doing that being effectively a DoS. What do you think I should do? Refactor this before migrating this package intro production or just migrate and fix later?

amoeba commented 6 years ago

@gothub made a great point that this could be caused by an error in my packaging code. Like a loop in the documents chain or something. Investigate this.

csjx commented 6 years ago

So after discussing this during the dev call, we realized this is from PackageModel, not DataPackage, so my comment above doesn't really apply. @laurenwalker, we probably need you to weigh in on this.

amoeba commented 6 years ago

I'm not totally sure that's true @csjx: If I grab a stack trace of some of the XHR, they are from PackageModel but others are from DataPackage

laurenwalker commented 6 years ago

There were two fixes to this:

  1. All the /meta and /object calls were being made because in the DataPackage.parse(), when a model is promoted from a DataONEObject to a DataPackage collection, we fetch it again. So this was causing all the nested DataPackages to be fetched.

  2. All the nested packages were grabbed via Solr queries and their download stats were grabbed from the logsolr Solr index. I added some additional logic to PackageModel.createNestedPackages() so that we exit after the first level of nesting.

amoeba commented 6 years ago

Thanks for the quick fix @laurenwalker. Can this patch get deployed to test.arcticdata.io sometime this week so I can test it on my package? If you think I could just apply a patch for just this commit in-place I'll do that on my own.

laurenwalker commented 6 years ago

I updated test.arcticdata.io so it now includes this fix. Let me know if you see any issues there.

Lauren

On Mon, Apr 23, 2018 at 3:00 PM, Bryce Mecum notifications@github.com wrote:

Thanks for the quick fix @laurenwalker https://github.com/laurenwalker. Can this patch get deployed to test.arcticdata.io sometime this week so I can test it on my package? If you think I could just apply a patch for just this commit in-place I'll do that on my own.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NCEAS/metacatui/issues/580#issuecomment-383685814, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVeFkd01SA1YeNFhKrjvU9QITi7nYSvks5triTFgaJpZM4TcUSc .

-- National Center for Ecological Analysis and Synthesis (NCEAS) University of California Santa Barbara (UCSB)

amoeba commented 6 years ago

Great, thanks for doing that. I just loaded https://test.arcticdata.io/#view/urn:uuid:dd7d8b46-04ce-48bf-b335-005fe7a83063 up and the infinite requests problem is fixed. Thanks! Though I still see a lot of logsolr calls as before.

Stepping back a bit, I'm curious if we need any calls to /meta or /object in order to render this view. What do you think?

laurenwalker commented 6 years ago

All those calls are a result of the DataPackage collection being fetched in order to draw the provenance charts. It's a little overkill since we don't need the system metadata for each data object in order to draw those charts. I'll create a new issue for that.

amoeba commented 6 years ago

Thanks @laurenwalker that sounds great.

https://github.com/NCEAS/metacatui/issues/581

amoeba commented 6 years ago

Hey @laurenwalker I guess we're still seeing this issue, https://arcticdata.io/catalog/#view/urn:uuid:c4ff81bd-4f7a-4ce3-b0d3-f7b594693dcd (Watch your Network panel). I didn't do any debugging yet.

Is this related to package loading or something else?

csjx commented 6 years ago

I'm also seeing delays because of many many (100s? 1000s?) of requests going to the CN Log Aggregation Solr endpoint to get usage statistics on every package member. This, too, should be lazy-loaded XHR updates to the table so the render() method is de-coupled and not blocked.

laurenwalker commented 6 years ago

Is this related to package loading or something else?

All those calls are a result of the DataPackage collection being fetched in order to draw the provenance charts. It's a little overkill since we don't need the system metadata for each data object in order to draw those charts.

I agree that the table should be lazy-loaded. We already do that for the table in the Editor. I've been thinking about moving that table view to the MetadataView.

amoeba commented 6 years ago

Thanks @laurenwalker !

laurenwalker commented 5 years ago

This is a duplicate of #581