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

Index creation (GSI-327) #13

Merged
merged 14 commits into from
Aug 22, 2023

Conversation

TheByronHimes
Copy link
Member

@TheByronHimes TheByronHimes commented Aug 16, 2023

MASS performs text searches in its queries, and an index is needed for that to work.
We need a way to make sure that the indexes exist for each collection corresponding to the configured searchable_classes, ideally at deploy time.
This update adds a way to make sure the collections exist (creating them if needed) and creating the indexes if needed.
I'm not sure about the placement of the function, though, which right now is in both run_rest() and consume_events(). I'd appreciate feedback on that in particular.

Update: Per feedback, the new function is only called from the load_resources function in QueryHandler. To avoid having the QueryHandler, which is part of the core, interact with the DB directly, I've moved the index creation function to the DaoCollection class. Its job is to provide the DAOs for a given collection, but it's an outbound adapter that deals with the database (indirectly), and seemed suitable for it.

Copy link
Member

@dontseyit dontseyit left a comment

Choose a reason for hiding this comment

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

We delete and recreate databases in test-bed without restarting the services, this can also happen in the production or staging I believe. This approach here solves only a part of issue. It would be better if we have a mechanism that can be triggered anytime @Cito what do you think?

Once we have the production database I am sure it will be steady and untouched so we won't need indexing again but for staging and development it would be useful.

mass/main.py Outdated Show resolved Hide resolved
@Cito
Copy link
Member

Cito commented Aug 16, 2023

We delete and recreate databases in test-bed without restarting the services, this can also happen in the production or staging

In production or staging databases should only be deleted when first deployed. But in the tests deployments it happens every time.

Suggestion: In load_resource() (and only there) collection_init_and_index_creation() should be called. It's the latest point in time when the database must exist. If you call the API before something has been loaded, you get an error or empty result anyway.

@coveralls
Copy link

coveralls commented Aug 17, 2023

Pull Request Test Coverage Report for Build 5926425191

  • 25 of 26 (96.15%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 91.787%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mass/ports/outbound/dao.py 1 2 50.0%
Totals Coverage Status
Change from base Build 5715713847: 0.2%
Covered Lines: 380
Relevant Lines: 414

💛 - Coveralls

Cito
Cito previously requested changes Aug 18, 2023
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Thanks, that's what I had in mind. Just a few more suggestions.

mass/adapters/outbound/dao.py Show resolved Hide resolved
mass/adapters/outbound/dao.py Outdated Show resolved Hide resolved
mass/adapters/outbound/dao.py Outdated Show resolved Hide resolved
@TheByronHimes TheByronHimes removed the request for review from Cito August 22, 2023 10:32
@TheByronHimes TheByronHimes dismissed Cito’s stale review August 22, 2023 11:31

Changes addressed, Cito out of office, other reviewers

@TheByronHimes TheByronHimes removed the request for review from Cito August 22, 2023 11:31
@TheByronHimes TheByronHimes merged commit 0ad243d into main Aug 22, 2023
6 checks passed
@TheByronHimes TheByronHimes deleted the feature/add_indexes_when_necessary_GSI-327 branch August 22, 2023 11:31
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.

5 participants