Skip to content

Conversation

rizzomichaelg
Copy link
Contributor

Fixes #334

Copy link
Contributor

@natanrolnik natanrolnik left a comment

Choose a reason for hiding this comment

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

@rizzomichaelg this looks good to me, beside the changelog:

1 - Can you please add your name, like in the other features?
2 - Can you please add the number of this PR (652) instead of the issue it fixes (334)?

@facebook-github-bot
Copy link

@rizzomichaelg updated the pull request - view changes

@natanrolnik
Copy link
Contributor

Looks good to me. @steven-supersolid wanna take a look?

@@ -320,9 +321,21 @@ export default class Browser extends DashboardView {
return data;
}

async fetchParseDataCount(source, filters) {
const query = queryFromFilters(source, filters);
const count = await query.count({ useMasterKey: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential performance issue - if a filter uses non-indexed fields then count() will perform a colscan. This could also happen in fetchParseData so may not be a concern, however count has no option to early exit by hitting a limit whereas fetchParseData does.

I'm not sure how to solve this, perhaps have to rely on parse-server having databaseOptions.maxTimeMS specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You raise a great point.

Besides relying on the parse-server timeout, we could also add a restriction on when the count will update and only update if the total number of objects in the table is less than a certain constant. Not super excited about this option, though. Might be best to rely on the server's timeout.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think - let's merge it for now and look at optimizations in a separate PR, unless you want to try anything different?

Copy link
Contributor

Choose a reason for hiding this comment

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

@steven-supersolid I want to release a new version including the Jobs fix, so I'm ok merging this and improving later.

@steven-supersolid steven-supersolid merged commit a716540 into parse-community:master Feb 9, 2017
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.

4 participants