fix: update limit for es response #4303

Ali-D-Akbar · 2024-03-26T20:02:09Z

PROD-3986
Adds a setting to update_index. This will increase the max_result_window for all existing indexes and subsequently update any newly created index as well.

~~Also updates the unit test that previously didn't support a custom value of max_result_window in ES related logic.~~
Keeping the same limit count for unit tests as we won't reach this limit while testing.

DawoudSheraz · 2024-03-28T12:10:55Z

course_discovery/apps/edx_elasticsearch_dsl_extensions/management/commands/update_index.py

+        conn.indices.put_settings(body={"index": {"max_result_window": settings.MAX_RESULT_WINDOW}})
+


If this is global setting, it should be set only once. it should not be set in a periodically running management command.

This setting is added to each index separately. Having it in this place specifically works as all the new/updated indexes will have this setting.

Is the operation idempotent?

Yes, it is.

zawan-ila

Why do we need it in 2 management commands? Wouldn't the settings update when only one runs? Would it be nicer to extract this into a separate management command?

Ali-D-Akbar · 2024-03-29T14:08:15Z

Why do we need it in 2 management commands?

To update this setting for both cases where we're either updating (through update_index) or creating new/rebuilding indexes (through search_index).

Wouldn't the settings update when only one runs?

In case when we create new index, it won't have the settings. These settings work for pre-existing indexes.

Would it be nicer to extract this into a separate management command?

That's debatable although the same command will be needed to run in both of the commands above.

DawoudSheraz · 2024-03-29T17:10:01Z

course_discovery/apps/core/tests/mixins.py

+        try:
+            self.es.indices.put_settings(body={"index": {"max_result_window": settings.MAX_RESULT_WINDOW}})
+        except NotFoundError:
+            pass


I personally am not a fan of this, specially in unit tests. Its a recipe for weirdness and unexpected behavior.

Basically, no setting is added when there is no index.

under what Testcases is this valid?

I don't really see any tests for this. Not sure why this should be required in the first place?

For some testing, can you try this in setUpclass? I still dont understand why this is needed for tests as there are not any items in unit tests thaa can cross the threshold.

The settings needs to be updated bcz I changed ELASTICSEARCH_DSL_QUERYSET_PAGINATION, ELASTICSEARCH_DSL_LOAD_PER_QUERY to 15000 in base.py.
Should I set this to 10k for tests only?

Hmmm, yeah I think we can keep the settings as it is for tests and not complicate this unnecessarily.

DawoudSheraz · 2024-03-29T17:10:39Z

course_discovery/apps/api/v1/tests/test_views/test_catalog_queries.py

@@ -8,6 +9,7 @@
 from course_discovery.apps.course_metadata.tests.factories import CourseFactory, CourseRunFactory


+@pytest.mark.django_db


why is this needed? I am assuming APITestCase is child of django or DRF's TestCase which takes care of Django DB ops.

True, it can be removed. 👍

Well, go ahead then.

DawoudSheraz · 2024-03-29T17:11:15Z

course_discovery/apps/edx_elasticsearch_dsl_extensions/management/commands/search_index.py

+        try:
+            es_connection = get_connection()
+            es_connection.indices.put_settings(body={"index": {"max_result_window": settings.MAX_RESULT_WINDOW}})
+        except NotFoundError:
+            pass


In what cases will NotFoundError raise?

Just in case if no index is created, we won't be adding this setting to any index.

How so? This is being done inside _create. Under what circumstances will we not create an index despite calling create?

It'd be nice to put this logic inside the ElasticSearchUtils class. Then you might be able to get by by calling it within ElasticsearchUtils.create_index.

@zawan-ila can you elaborate on where I should put these settings in the utils?

This is what @zawan-ila is referring to https://github.com/openedx/course-discovery/blob/master/course_discovery/apps/core/utils.py#L28. When calling create_index, this result_window update can be done there (possibly without try/catch?).

I tried adding this settings isn't being added for all indexes. Seems like it needs to be added after all the indexes and aliases are defined.

zawan-ila · 2024-04-04T08:26:23Z

course_discovery/apps/core/tests/mixins.py

+        try:
+            self.es.indices.put_settings(body={"index": {"max_result_window": settings.MAX_RESULT_WINDOW}})
+        except NotFoundError:
+            pass


I don't really see any tests for this. Not sure why this should be required in the first place?

zawan-ila · 2024-04-04T08:30:19Z

course_discovery/apps/edx_elasticsearch_dsl_extensions/management/commands/search_index.py

+        try:
+            es_connection = get_connection()
+            es_connection.indices.put_settings(body={"index": {"max_result_window": settings.MAX_RESULT_WINDOW}})
+        except NotFoundError:
+            pass


It'd be nice to put this logic inside the ElasticSearchUtils class. Then you might be able to get by by calling it within ElasticsearchUtils.create_index.

zawan-ila · 2024-04-04T08:41:39Z

course_discovery/apps/edx_elasticsearch_dsl_extensions/management/commands/update_index.py

@@ -127,6 +127,8 @@ def _update(self, models, options):
        for index_alias_mapper in alias_mappings:
            index_alias_mapper.registered_index._name = index_alias_mapper.alias  # pylint: disable=protected-access

+        conn.indices.put_settings(body={"index": {"max_result_window": settings.MAX_RESULT_WINDOW}})


Similar to my comment above, can we place this inside ElasticsearchUtils?

DawoudSheraz · 2024-04-05T09:34:41Z

course_discovery/apps/api/tests/test_serializers.py

@@ -2624,8 +2624,7 @@ def get_expected_data(cls, person, request):


 @pytest.mark.django_db


You might not need django_db mark if you are already using Django testcase.

zawan-ila · 2024-04-09T13:24:36Z

course_discovery/apps/edx_elasticsearch_dsl_extensions/management/commands/update_index.py

@@ -117,11 +117,13 @@ def _update(self, models, options):
                    )
                    if record_count_is_sane:
                        ElasticsearchUtils.set_alias(conn, alias, new_index_name)
+                        ElasticsearchUtils.update_max_result_window(conn, settings.MAX_RESULT_WINDOW, new_index_name)


Would be really nice if we could do this just once at the start of the loop.

Should be done only after set_alias.

Ali-D-Akbar force-pushed the aakbar/PROD-3986 branch 4 times, most recently from 315212d to 8f47923 Compare March 28, 2024 10:49

DawoudSheraz reviewed Mar 28, 2024

View reviewed changes

Ali-D-Akbar force-pushed the aakbar/PROD-3986 branch 4 times, most recently from c58dc9f to b24cdba Compare March 29, 2024 12:09

Ali-D-Akbar marked this pull request as ready for review March 29, 2024 12:28

Ali-D-Akbar requested a review from DawoudSheraz March 29, 2024 12:29

zawan-ila reviewed Mar 29, 2024

View reviewed changes

DawoudSheraz reviewed Mar 29, 2024

View reviewed changes

Ali-D-Akbar requested review from DawoudSheraz and zawan-ila April 3, 2024 19:33

zawan-ila reviewed Apr 4, 2024

View reviewed changes

Ali-D-Akbar force-pushed the aakbar/PROD-3986 branch 2 times, most recently from 3cc82b8 to bee2823 Compare April 4, 2024 09:22

DawoudSheraz reviewed Apr 5, 2024

View reviewed changes

Ali-D-Akbar force-pushed the aakbar/PROD-3986 branch 2 times, most recently from 69709a0 to 141c3c9 Compare April 9, 2024 11:00

fix: update limit for es response

7b2a58e

Ali-D-Akbar force-pushed the aakbar/PROD-3986 branch from 8d5f9cc to 7b2a58e Compare April 9, 2024 11:33

DawoudSheraz approved these changes Apr 9, 2024

View reviewed changes

zawan-ila approved these changes Apr 9, 2024

View reviewed changes

Ali-D-Akbar merged commit a700f20 into master Apr 9, 2024
12 checks passed

Ali-D-Akbar deleted the aakbar/PROD-3986 branch April 9, 2024 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update limit for es response #4303

fix: update limit for es response #4303

Ali-D-Akbar commented Mar 26, 2024 •

edited

Loading

DawoudSheraz Mar 28, 2024

Ali-D-Akbar Mar 29, 2024

DawoudSheraz Apr 4, 2024

Ali-D-Akbar Apr 4, 2024

zawan-ila left a comment

Ali-D-Akbar commented Mar 29, 2024 •

edited

Loading

DawoudSheraz Mar 29, 2024

Ali-D-Akbar Apr 1, 2024

DawoudSheraz Apr 4, 2024

zawan-ila Apr 4, 2024

DawoudSheraz Apr 8, 2024

Ali-D-Akbar Apr 8, 2024

DawoudSheraz Apr 8, 2024

DawoudSheraz Mar 29, 2024

Ali-D-Akbar Apr 1, 2024

DawoudSheraz Apr 4, 2024

DawoudSheraz Mar 29, 2024

Ali-D-Akbar Apr 1, 2024

DawoudSheraz Apr 4, 2024

zawan-ila Apr 4, 2024

Ali-D-Akbar Apr 5, 2024 •

edited

Loading

DawoudSheraz Apr 8, 2024

Ali-D-Akbar Apr 9, 2024

zawan-ila Apr 4, 2024

zawan-ila Apr 4, 2024

zawan-ila Apr 4, 2024

DawoudSheraz Apr 5, 2024

zawan-ila Apr 9, 2024

Ali-D-Akbar Apr 9, 2024

		conn.indices.put_settings(body={"index": {"max_result_window": settings.MAX_RESULT_WINDOW}})

		@@ -8,6 +9,7 @@
		from course_discovery.apps.course_metadata.tests.factories import CourseFactory, CourseRunFactory


		@pytest.mark.django_db

		@@ -2624,8 +2624,7 @@ def get_expected_data(cls, person, request):


		@pytest.mark.django_db

fix: update limit for es response #4303

fix: update limit for es response #4303

Conversation

Ali-D-Akbar commented Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zawan-ila left a comment

Choose a reason for hiding this comment

Ali-D-Akbar commented Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ali-D-Akbar Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ali-D-Akbar commented Mar 26, 2024 •

edited

Loading

Ali-D-Akbar commented Mar 29, 2024 •

edited

Loading

Ali-D-Akbar Apr 5, 2024 •

edited

Loading