Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix admin products publication slowness #4260

Merged
merged 11 commits into from
Jun 4, 2018

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented May 21, 2018

Resolves #4040
Impact: minor
Type: performance

Issue

Logging in as admin and going to grid / loading med/larg devtools dataset would cause browser to hang due to crazy admin "Products" publication

Solution

This solves the issue WITHOUT revision control, i.e., only for 2.0.0 release track. It may be solvable in 1.0 track, but revisions adds a layer of complication. Let's get this merged and then figure out whether it is worth trying to fix in 1.0

I also added two relevant indexes in the Products schema.

This also includes the changes from #4256 and #4259

Breaking changes

None.

Testing

  1. As admin, with 1000+ products.
  2. On product grid home, it loads within a few seconds and does not hang.
  3. On product grid for tag, it loads within a few seconds and does not hang, and is filtered correctly.
  4. Test above with and without edit mode. In edit mode, you should only see products for shops for which you have "createProduct" permission (in marketplace setup).
  5. Test any other grid filtering you can think of

@aldeed aldeed changed the base branch from master to feat-4237-mikemurray-remove-revisions May 21, 2018 22:38
@aldeed aldeed changed the title Fix admin products publication slowness [WIP] Fix admin products publication slowness May 21, 2018
@pmn4
Copy link
Collaborator

pmn4 commented May 22, 2018

@aldeed I have been working on a change to the same code to ensure that Marketplace Shops only see their own products. Do you mind taking a look?

@aldeed
Copy link
Contributor Author

aldeed commented May 22, 2018

@pmn4 I looked at your code while doing this and made the same changes in this PR. This is for 2.0 track. Your PR code looked good to me, but I see that you've made some more changes today. I can take another look.

@pmn4
Copy link
Collaborator

pmn4 commented May 22, 2018

thanks @aldeed.
there are two PRs, the one you referenced (which ensures that admins see only the shops for which they have edit rights) and the second (which limits products to either a) all active shops if the current shop is the Primary Shop, or b) the current shop's products if the current Shop is a Merchant Shop)

@aldeed aldeed changed the base branch from feat-4237-mikemurray-remove-revisions to release-2.0.0 May 30, 2018 19:55
@aldeed aldeed changed the title [WIP] Fix admin products publication slowness Fix admin products publication slowness Jun 1, 2018
@aldeed aldeed requested a review from brent-hoover June 1, 2018 19:37
@aldeed aldeed self-assigned this Jun 1, 2018
@aldeed aldeed requested a review from mikemurray June 1, 2018 20:01
@aldeed aldeed changed the base branch from release-2.0.0 to release-1.13.0 June 1, 2018 20:36
@aldeed aldeed removed the request for review from brent-hoover June 1, 2018 21:57
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still pretty slow when loading Tag pages but 1000X better than the current version which just crashes my browser and the server.

@@ -339,6 +352,7 @@ describe("JobCollection", function () {
});

it("should have dependent job saved after cancelled antecedent is also cancelled", function (done) {
this.timeout(15000);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems an odd thing to add to this PR. And just a little odd in general. Is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes I made seem to have changed the order in which Meteor runs the tests, and a bunch of them started consistently timing out on my machine and PR. I think the timeouts happen for whatever tests run first, because often the startup code (mostly migrations) is still running and ends up slowing down how fast the db ops in tests happen.

It seems like Meteor should have some better solution, like allowing us to indicate when startup code is done, and then starting the test run. Maybe if we moved the migrations and other things into Meteor.startup blocks? But anyway, bumping up the timeouts on every test that failed seemed the quickest way to unblock this.

@aldeed aldeed removed the request for review from mikemurray June 4, 2018 20:15
@aldeed aldeed merged commit d1f5a08 into release-1.13.0 Jun 4, 2018
@aldeed aldeed deleted the fix-4040-aldeed-fast-pub-admin-products branch June 4, 2018 20:37
@spencern spencern mentioned this pull request Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants