algolia / gatsby-plugin-algolia

A plugin to push to Algolia based on graphQl queries
https://yarn.pm/gatsby-plugin-algolia
Apache License 2.0
177 stars 45 forks source link

feat(concurrentQueries): add option to disable queries happening at the same time #96

Closed prichey closed 3 years ago

prichey commented 3 years ago

Like others, I've been experiencing hanging builds on Netlify. My hunch is that this is due to concurrent index access. For the sake of code cleanliness, I have my queries split up into several even though they operate on the same index.

This PR adds a new option to the plugin configuration, disableConcurrentAccess (which defaults to false, so this is not a breaking change), as an attempt to resolve the hanging builds. This solution has been working for me locally so I'm curious to see if it works for others as well.

The bulk of this PR is simply moving doQuery to a standalone function and then optionally awaiting its resolution. I also made it such that doQuery simply returns its index and toRemove object rather than mutating global state, which I think was also creating issues when index names collided.

Let me know what you think. Once I get this one merged it will be a fairly small change to address https://github.com/algolia/gatsby-plugin-algolia/issues/93#issuecomment-706409564. Thanks for the consideration!

Haroenv commented 3 years ago

this looks great! I'll need to do a detailed review later since the diff is so hard to read, I need to make sure no other changes accidentally happen here :)

Haroenv commented 3 years ago

could you call the feature concurrentQueries which defaults to true?

prichey commented 3 years ago

I also made a few changes to the logging. With this update, logs look like this:

[Algolia] 8 queries to index
[Algolia] Query #0 (TEST_INDEX_FOO): Executing query
[Algolia] Query #0 (TEST_INDEX_FOO): graphql resulted in 79 records
[Algolia] Query #0 (TEST_INDEX_FOO): Starting Partial updates
[Algolia] Query #0 (TEST_INDEX_FOO): Found 1 existing records
[Algolia] Query #0 (TEST_INDEX_FOO): Partial updates – [insert/update: 79, total: 79]
[Algolia] Query #0 (TEST_INDEX_FOO): Splitting in 1 jobs
[Algolia] Query #0 (TEST_INDEX_FOO): Done!
[Algolia] Query #1 (TEST_INDEX_BAR): Executing query
[Algolia] Query #1 (TEST_INDEX_BAR): graphql resulted in 259 records
[Algolia] Query #1 (TEST_INDEX_BAR): Starting Partial updates
[Algolia] Query #1 (TEST_INDEX_BAR): Found 79 existing records
[Algolia] Query #1 (TEST_INDEX_BAR): Partial updates – [insert/update: 259, total: 259]
[Algolia] Query #1 (TEST_INDEX_BAR): Splitting in 1 jobs
[Algolia] Query #1 (TEST_INDEX_BAR): Done!
[Algolia] Query #2 (TEST_INDEX_FOO): Executing query
[Algolia] Query #2 (TEST_INDEX_FOO): graphql resulted in 1638 records
[Algolia] Query #2 (TEST_INDEX_FOO): Starting Partial updates
[Algolia] Query #2 (TEST_INDEX_FOO): Found 338 existing records
[Algolia] Query #2 (TEST_INDEX_FOO): Partial updates – [insert/update: 1638, total: 1638]
[Algolia] Query #2 (TEST_INDEX_FOO): Splitting in 2 jobs
[Algolia] Query #2 (TEST_INDEX_FOO): Done!
...

Let me know if we'd rather not include index name in the logs as a security concern, I just figure they're useful in trying to figure out why indexing might have failed.

Another question is do you prefer offsetting the query number in the logs (i.e. Query #0 is the first vs Query #1?)

prichey commented 3 years ago

@Haroenv I'm happy to update the name! Also totally understand taking your time in reviewing, I understand the diff is a bit hard to read. Thanks for the quick responses!

prichey commented 3 years ago

@Haroenv taking a look now, thanks for patching!

prichey commented 3 years ago

Diff looks great and tested everything, works like a charm! Feel free to merge whenever, thanks! @Haroenv

Haroenv commented 3 years ago

I'm going to release this right away!

EDIT: done as 0.13.0