Bug #4464

[Workbench] Collections tab loads forever on a specific project

Added by Brett Smith almost 5 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
Workbench
Target version:
Start date:
02/05/2015
Due date:
% Done:

100%

Estimated time:
(Total: 2.00 h)
Story points:
0.5

Description

contents-table.png (67.6 KB) contents-table.png Column borders appear fine in Firefox 35 Brett Smith, 02/09/2015 09:41 PM

Subtasks

Task #5140: Review 4464-api-project-contents-wipResolvedRadhika Chippada


Related issues

Related to Arvados - Bug #4523: [Workbench] Search dialog giving error when searching in "All projects" in qr1hiResolved12/08/2014

Related to Arvados - Story #3021: [Tests] Make jenkins tests run faster.Resolved01/05/2015

Related to Arvados - Bug #5095: [FUSE] arv-mount takes up too much memory and occassionally crashes when listing large 'home' directoryResolved02/16/2015

Has duplicate Arvados - Bug #4522: [Workbench] qr1hi /collections page error "HTTPClient::ReceiveTimeoutError error connecting to API server"Rejected

Has duplicate Arvados - Bug #4942: [Workbench] Data Collections Tab in Projects takes too long to load or failsClosed01/08/2015

Associated revisions

Revision 1d4a39ab
Added by Brett Smith over 4 years ago

Merge branch '4464-api-project-contents-wip'

Refs #4464. Closes #5140.

History

#1 Updated by Tom Clegg almost 5 years ago

Eventually times out and shows Retry button. Browser console log shows "timeout" error message sent by Workbench.

#2 Updated by Ward Vandewege almost 5 years ago

  • Target version changed from Bug Triage to Arvados Future Sprints

#3 Updated by Ward Vandewege almost 5 years ago

  • Story points set to 0.5

#4 Updated by Tom Clegg over 4 years ago

  • Target version changed from Arvados Future Sprints to 2015-02-18 sprint

#5 Updated by Brett Smith over 4 years ago

  • Assigned To set to Brett Smith

#6 Updated by Brett Smith over 4 years ago

  • Status changed from New to In Progress

#7 Updated by Brett Smith over 4 years ago

This project contains several collections with large (50+MiB) manifests. The API request times out because the API server can't serialize the response quickly enough.

The collections list method doesn't include manifest texts by default, for exactly this reason. However, this page uses the project contents method, which has no such limitation. I think the best thing we can do to address this is to have project contents follow the same rules as collections list.

#8 Updated by Brett Smith over 4 years ago

4464-api-project-contents-wip is up for review to implement the change in the last comment. It also does some related cleanup in the group contents method, for example by removing the obsolete include_linked parameter (with Tom's permission).

It's a little difficult to tell whether or not this change will totally resolve this issue. We should leave this ticket open until that can be confirmed.

#9 Updated by Radhika Chippada over 4 years ago

Review feedback:

Documentation updates

  • "Retrieve a list of items which are associated with the given group by ownership (i.e., the group owns the item)". => Can it be simplified as “Retrieve a list of items owned by the group”
  • In the Arguments table, the vertical line separator between type and description columns is disappearing. Seems like due to the parentheses characters in limit default 100
  • “N.B.” => We use “Note:” in most other places and I think is easier to understand.
  • “Because adding access tokens to manifests can be computationally expensive, the manifest_text field is not included” => May be we can simplify it by saying “Because adding manifests can be computationally expensive, the manifest_text field is not included”

Code updates:

  • map { |s| "#{ar_table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }

was there a bug here in earlier version? Just curious.

  • New tests in arvados_model_test.rb

Can you add “job” in the test case names since all these are referring to job data model?

test "selectable_attributes includes database attributes" do => test “job.selectable_attributes includes database attributes" do

  • It would be nice to add a performance test for visiting the Data collections tab for this project (browsing_test.rb). However, since we do not yet have a jenkins job running to measure performance against qr1hi, this will not help compare as of now.

#10 Updated by Brett Smith over 4 years ago

Radhika Chippada wrote:

Documentation updates

  • "Retrieve a list of items which are associated with the given group by ownership (i.e., the group owns the item)". => Can it be simplified as “Retrieve a list of items owned by the group”
  • “N.B.” => We use “Note:” in most other places and I think is easier to understand.

Both done.

  • In the Arguments table, the vertical line separator between type and description columns is disappearing. Seems like due to the parentheses characters in limit default 100

Hmm, I can't reproduce this in Firefox 35. See the attached screenshot. Even with different window sizes, I couldn't get it to disappear. The markup appears correct, both in Textile and generated HTML. What browser are you using? Do you see the same issue for the table under the list method? (I copied most of the text from that, but didn't modify it.)

  • “Because adding access tokens to manifests can be computationally expensive, the manifest_text field is not included” => May be we can simplify it by saying “Because adding manifests can be computationally expensive, the manifest_text field is not included”

This text is copied verbatim from the collections list page. I'm a little wary about changing it because I don't want users to get the impression that it's difficult for Arvados to send blocks of plain text back and forth.

Code updates:

  • map { |s| "#{ar_table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }

was there a bug here in earlier version? Just curious.

Sort of. We need this change for the method to work right when GroupsController is querying Collections with a @select parameter. The old code would always use "groups" as the table name here, and crash looking for columns in the collections table. This change ensures we're referring to the table name for whatever's in @objects.

  • New tests in arvados_model_test.rb

Can you add “job” in the test case names since all these are referring to job data model?

The goal is to test the method in ArvadosModel. I can't call that directly, because ArvadosModel is effectively abstract, so I had to pick some concrete implementation. I picked Job, but there should be nothing Job-specific about the tests—it should be possible to write equivalent tests for any subclass of ArvadosModel, as long as it has attributes that meet the criteria described.

If there's some way to help make clearer that picking Job is effectively an implementation detail, I'm happy to do that. One way I tried to emphasize that is by putting these in arvados_model_test.rb rather than job_test.rb. I'm concerned that putting "job" in the test title will give the opposite impression, that we're testing something specific about the Job class, when that's not the point.

  • It would be nice to add a performance test for visiting the Data collections tab for this project (browsing_test.rb). However, since we do not yet have a jenkins job running to measure performance against qr1hi, this will not help compare as of now.

I had the same thought about a performance test, but yeah, it feels like we don't have good infrastructure for testing performance handling of large collections yet. Besides, performance is only part of the motivation for the branch—the other half is to make this method more consistent with the collections list method, which should happen anyway.

Now at 7c5a88d. Thanks.

#11 Updated by Brett Smith over 4 years ago

  • Status changed from In Progress to Resolved

The project named in this bug now loads very quickly, so I'm marking this as closed. Admittedly there's been some fallout from this API change, but we're tracking those separately.

Also available in: Atom PDF