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

[ENG-6708] IndexStrategy revamp (more than one index per strategy, wow) #840

Merged

Conversation

aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Jan 17, 2025

revamp IndexStrategy to allow multiple indexes as part of a single coherent strategy -- this better allows common elasticsearch optimizations, like distinct indexes mapped for different kinds of queries

(recommend hiding whitespace changes from diff, especially for changes to existing strategies)

  • simplify existing classes by making them dataclasses (with additional fields) -- an IndexStrategy instance knows its checksum (which may be non-current) and may have multiple SpecificIndex

    • IndexStrategy -- each instance represents a current or past version of the strategy, identified by checksum
      • strategy_name (rename existing name field for disambig)
      • strategy_check (new field; default hexdigest from CURRENT_STRATEGY_CHECKSUM but may be parsed from or index name or indexStrategy query param)
    • IndexStrategy.SpecificIndex -- each instance represents a specific index that belongs to a strategy instance and may or may not exist
      • index_strategy (existing field)
      • subname (new field; unique within a strategy)
  • remove uniindex methods from IndexStrategy (and friends)

    • each_specific_index
    • for_specific_index
    • for_current_index
    • SpecificIndex.pls_setup
    • SpecificIndex.pls_handle_cardsearch
    • SpecificIndex.pls_handle_valuesearch
    • Elastic8IndexStrategy.index_settings
    • Elastic8IndexStrategy.index_mappings
  • add replacement multiindex methods to IndexStrategy (and friends)

    • (classmethod) each_existing_index (based on index names from elastic; may be any hex)
    • each_subnamed_index (includes non-existent)
    • get_index
    • index_subname_set (each index has a subname that's unique within the strategy)
    • is_current
    • pls_setup
    • pls_check_exists
    • pls_start_keeping_live
    • is_kept_live
    • pls_teardown
    • pls_handle_cardsearch
    • pls_handle_valuesearch
    • pls_refresh
    • pls_get_strategy_status (includes index statuses)
    • with_strategy_check (return copy of the strategy with another check)
    • Elastic8IndexStrategy.define_current_indexes (abstractmethod)
    • Elastic8IndexStrategy.each_subnamed_index (based on current_index_definitions)
  • update existing IndexStrategy base methods

    • add strategy_check to indexname_prefix
    • pls_get_default_for_searching (get strategy_check from es alias (or current))
    • pls_make_default_for_searching (by strategy instance (or strategy_check), not SpecificIndex)
  • update share.search.index_strategy public interface (see all)

  • update the "elasticsearch indexes" admin view to list multiple

  • update trovesearch_denorm strategy, split its one index into two ("cards" and "iri_values")

ENG-6708

@coveralls
Copy link

coveralls commented Jan 17, 2025

Coverage Status

coverage: 91.738% (-0.004%) from 91.742%
when pulling 2d267eb on aaxelb:feat/multiple-current-indexes
into c953024 on CenterForOpenScience:develop.

@aaxelb aaxelb changed the title [wip][ENG-6708] IndexStrategy revamp (more than one index per strategy, wow) [ENG-6708] IndexStrategy revamp (more than one index per strategy, wow) Jan 23, 2025
@aaxelb aaxelb marked this pull request as ready for review January 23, 2025 14:58
Copy link
Contributor Author

@aaxelb aaxelb left a comment

Choose a reason for hiding this comment

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

(comments from group review)

share/search/index_strategy/_base.py Show resolved Hide resolved
share/search/index_strategy/__init__.py Show resolved Hide resolved
share/search/index_strategy/_base.py Show resolved Hide resolved
share/search/index_strategy/_base.py Outdated Show resolved Hide resolved
share/search/index_strategy/elastic8.py Outdated Show resolved Hide resolved
@felliott
Copy link
Member

LGTM! We reviewed over zoom, all requested changes made!

@aaxelb aaxelb force-pushed the feat/multiple-current-indexes branch 2 times, most recently from 9cb91f3 to 5b5fc9f Compare January 27, 2025 17:01
@aaxelb aaxelb force-pushed the feat/multiple-current-indexes branch from 5b5fc9f to 2d267eb Compare January 28, 2025 15:26
@felliott
Copy link
Member

Looks good!

@aaxelb
Copy link
Contributor Author

aaxelb commented Jan 28, 2025

(merging despite the drop in test coverage, intending to follow up with more tests)

@aaxelb aaxelb merged commit f9c499e into CenterForOpenScience:develop Jan 28, 2025
2 of 3 checks passed
@aaxelb aaxelb deleted the feat/multiple-current-indexes branch January 28, 2025 18:52
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.

3 participants