fix: make sync opt to be selectable for auto setting update logic #425

kiyohara · 2022-03-31T12:58:42Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Related Issue	-
Need Doc update	no

Describe your change

In algoliasearch-rails gem 1.x the auto setting update logic run asynchronously.
This behavior has been changed since the version 2.0 release to 'synchronously API call.'
ref. Here is the discussion that considered changing the specification.

It means if you have some settings changes, all API calls hook the setting update API, and are blocked until the settings changing process is finished in Algolia server.

I believe the spec change is reasonable for almost all situations because in many cases API calls require the response based on the new setting immediately.
However, I think, Algolia is a backend for sites that has big records and big client accesses, and is required quick response without '504 Gateway Timeout'.
I guess this is one of the reasons that all methods' default sync option is set as 'asynchronous' in this gem, like the algolia_set_settings method.

I think the best way how to decide the sync behavior, especially focused on "auto setting update logic", is to depend on AlgoliaSearch.configuration's :synchronous option similar to any other method's default option like this.

This PR add sync option select logic in auto setting update func.
It decide a sync option (sync or async) based on the above config.
The default behavior is async, same as 1.x.

What problem is this fixing?

When you deploy some new index settings changes to your site using this gem, your site clients might see '504 Gateway Timeout' when they search anything.

This is a real problem we are experiencing with our site management.
Many 504 responses are occurred when we update index settings.
We believe that a quick response with a little incorrect is better than a slow response that is 100% correct (and sometimes it's 504), for our site customers.
So, we decided to change back gem version to 1.26.0.
Of cause, I understand that it's not all user's requirements.
So, I fix it to depend on AlgoliaSearch.configuration.

…o_setting_update_logic

DevinCodes

Hi @kiyohara !

Thank you again for your contribution. I think it's a very valid one, so I'd love to merge this. Before we merge it, though, could you please write a test to ensure it works properly?

Thank you in advance!

(Don't worry about the failing CI for now, this is due to credentials as mentioned in another of your PRs 😉 )

kiyohara · 2022-04-11T13:26:22Z

Hi @DevinCodes.

Thank you for your review.
I'm glad you accepted the concept of this PR.
I wrote tests. Could you check it, please?

Of course, All tests I added have passed in my local dev env.
Please run it in your CI env's rspec.

I'll also add the tests for another PR #426 soon.

…o_setting_update_logic

DevinCodes

I just have one small suggestion, other than that this looks good! Let me know what you think 🙂

DevinCodes · 2022-04-15T08:46:06Z

spec/integration_spec.rb

+    end
+
+    it 'should not call set_setting!' do
+      expect_any_instance_of(Algolia::Search::Index).not_to receive(:set_settings)


Shouldn't this be as follows?

Suggested change

expect_any_instance_of(Algolia::Search::Index).not_to receive(:set_settings)

expect_any_instance_of(Algolia::Search::Index).not_to receive(:set_settings!)

I think your suggestion is perfectly right.

On the other hand, I was wondering why this test passed.
After additional investigation, it turns out that I have overlooked the cache mechanism.
When algolia_ensure_init is called more than once, neither set_settings nor set_settings! were being called ever at the second time and after, under any conditions.
I should have used the Red-Green-Refactor approach, but I skipped the Red check phase. embarrassing.

I have fixed the test, please check it out.

…ate_logic' of github.com:kiyohara/algoliasearch-rails into fix/make_sync_opt_to_be_selectable_for_auto_setting_update_logic

kiyohara · 2022-05-10T11:59:13Z

@DevinCodes I believe the test is more complete thanks to your suggestions.
Thank you so much. It's ready for your review.
Is there anything I can do to help you get started with this PR review?

DevinCodes

Thank you again @kiyohara ! 🙇‍♂️

The PR looks great, I just have one question. This is probably to some context that I'm missing, but afterwards we can merge the PR I think 😄

DevinCodes · 2022-05-11T08:50:56Z

spec/integration_spec.rb

+    end
+
+    it 'should call set_setting' do # NOTE: set_setting! call set_setting internally.
+      expect_any_instance_of(Algolia::Search::Index).to receive(:set_settings).and_call_original


I'm not sure I understand why we need to add the .and_call_original at the end here. Could you explain why this is needed, please?

I think this is a very good point.

Initially, I was going to write a test here to make sure that set_setting is not executed as opposed to the above test to make sure that set_setting! is executed.
However, I found that set_setting! executes set_setting internally, so I changed it to a test to make sure that set_setting is executed.
set_setting! use the return value of set_setting, so and_call_original was needed.

However, your point has made me realize that.
If set_setting! is using set_setting internally, then there is no point in testing the contrast between the two.
I think the key point is whether it is asynchronous, i.e., whether the wait_task is executed.

I modified the test according to this concept.
May I ask you to check it again, please?

DevinCodes · 2022-05-11T08:52:25Z

spec/integration_spec.rb

+    end
+
+    it 'should call set_setting' do
+      expect_any_instance_of(Algolia::Search::Index).to receive(:set_settings).and_call_original


Same as above

Unlike the previous line, this line is a simple typo.
cand_call_original was not needed. 🙇

DevinCodes

Thank you @kiyohara ! 🚀

kiyohara · 2022-05-18T22:28:00Z

I'm so happy! 😄

kiyohara · 2022-06-02T11:41:09Z

@DevinCodes I'm so happy that this PR was merged. Thank you so much.
BTW, do you have any info on the timeline for when the gem containing this PR would be released?

Half of the several problems with our web service operations have been resolved with the new release of algoliasearch-client-ruby. We are so glad and appreciate you 🙏

We are eagerly waiting for the other half of the issues to be resolved by a new version releasing of algoliasearch-rails 😄

DevinCodes · 2022-06-03T07:47:31Z

Thank you for the follow-up @kiyohara , and sorry for the delay!

We'll release a new version on Monday. Your changes will be included in that release 🙂

kiyohara · 2022-06-06T11:27:56Z

@DevinCodes Sorry for urging you to release a new version. I appreciate your kindness.
Thanks to you I can tell my team members that the Gemfile.lock of our product can be updated. We can continue to use Algolia with more peace of mind.
Thank you again!

DevinCodes · 2022-06-06T14:58:52Z

@kiyohara we've just released v2.2.1 with your changes. Thank you again for your contributions 🙂

kiyohara · 2022-06-07T03:33:51Z

@DevinCodes I'm so glad! 🎉 Thank you for your arrangement of the release 🙏

kiyohara and others added 2 commits March 31, 2022 11:26

fix: make sync opt to be selectable for auto setting update logic

5cb61e0

Merge branch 'master' into fix/make_sync_opt_to_be_selectable_for_aut…

2d92839

…o_setting_update_logic

DevinCodes requested changes Apr 8, 2022

View reviewed changes

add specs

a722647

kiyohara requested a review from DevinCodes April 11, 2022 13:26

Merge branch 'master' into fix/make_sync_opt_to_be_selectable_for_aut…

50e6171

…o_setting_update_logic

DevinCodes requested changes Apr 15, 2022

View reviewed changes

kiyohara added 2 commits April 15, 2022 11:38

fix meaningless tests (add cache avoiding)

79577ca

Merge branch 'fix/make_sync_opt_to_be_selectable_for_auto_setting_upd…

d7d8552

…ate_logic' of github.com:kiyohara/algoliasearch-rails into fix/make_sync_opt_to_be_selectable_for_auto_setting_update_logic

kiyohara requested a review from DevinCodes April 15, 2022 13:18

DevinCodes requested changes May 11, 2022

View reviewed changes

fix test point (change from set_setting! to wait_task)

8ea6d0d

kiyohara requested a review from DevinCodes May 11, 2022 13:05

DevinCodes approved these changes May 17, 2022

View reviewed changes

DevinCodes merged commit 3fd9038 into algolia:master May 17, 2022

kiyohara deleted the fix/make_sync_opt_to_be_selectable_for_auto_setting_update_logic branch May 18, 2022 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make sync opt to be selectable for auto setting update logic #425

fix: make sync opt to be selectable for auto setting update logic #425

kiyohara commented Mar 31, 2022

DevinCodes left a comment •

edited

Loading

kiyohara commented Apr 11, 2022

DevinCodes left a comment

DevinCodes Apr 15, 2022

kiyohara Apr 15, 2022

kiyohara commented May 10, 2022

DevinCodes left a comment

DevinCodes May 11, 2022

kiyohara May 11, 2022

DevinCodes May 11, 2022

kiyohara May 11, 2022

DevinCodes left a comment

kiyohara commented May 18, 2022

kiyohara commented Jun 2, 2022

DevinCodes commented Jun 3, 2022

kiyohara commented Jun 6, 2022

DevinCodes commented Jun 6, 2022

kiyohara commented Jun 7, 2022

	expect_any_instance_of(Algolia::Search::Index).not_to receive(:set_settings)
	expect_any_instance_of(Algolia::Search::Index).not_to receive(:set_settings!)

fix: make sync opt to be selectable for auto setting update logic #425

fix: make sync opt to be selectable for auto setting update logic #425

Conversation

kiyohara commented Mar 31, 2022

Describe your change

What problem is this fixing?

DevinCodes left a comment • edited Loading

Choose a reason for hiding this comment

kiyohara commented Apr 11, 2022

DevinCodes left a comment

Choose a reason for hiding this comment

DevinCodes Apr 15, 2022

Choose a reason for hiding this comment

kiyohara Apr 15, 2022

Choose a reason for hiding this comment

kiyohara commented May 10, 2022

DevinCodes left a comment

Choose a reason for hiding this comment

DevinCodes May 11, 2022

Choose a reason for hiding this comment

kiyohara May 11, 2022

Choose a reason for hiding this comment

DevinCodes May 11, 2022

Choose a reason for hiding this comment

kiyohara May 11, 2022

Choose a reason for hiding this comment

DevinCodes left a comment

Choose a reason for hiding this comment

kiyohara commented May 18, 2022

kiyohara commented Jun 2, 2022

DevinCodes commented Jun 3, 2022

kiyohara commented Jun 6, 2022

DevinCodes commented Jun 6, 2022

kiyohara commented Jun 7, 2022

DevinCodes left a comment •

edited

Loading