Bug #4464
closed[Workbench] Collections tab loads forever on a specific project
Added by Brett Smith almost 10 years ago. Updated over 9 years ago.
Files
contents-table.png (67.6 KB) contents-table.png | Column borders appear fine in Firefox 35 | Brett Smith, 02/09/2015 09:41 PM |
Related issues
Updated by Tom Clegg almost 10 years ago
Eventually times out and shows Retry button. Browser console log shows "timeout" error message sent by Workbench.
Updated by Ward Vandewege almost 10 years ago
- Target version changed from Bug Triage to Arvados Future Sprints
Updated by Tom Clegg over 9 years ago
- Target version changed from Arvados Future Sprints to 2015-02-18 sprint
Updated by Brett Smith over 9 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.
Updated by Brett Smith over 9 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.
Updated by Radhika Chippada over 9 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, themanifest_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.
Updated by Brett Smith over 9 years ago
- File contents-table.png contents-table.png added
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, themanifest_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.
Updated by Brett Smith over 9 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.