Skip to content

feat: "newest courses first" Algolia sort — config-gated replica registry + fail-safe#118

Open
macdiesel wants to merge 22 commits into
masterfrom
bbeggs/newest-courses-sort-replica
Open

feat: "newest courses first" Algolia sort — config-gated replica registry + fail-safe#118
macdiesel wants to merge 22 commits into
masterfrom
bbeggs/newest-courses-sort-replica

Conversation

@macdiesel

@macdiesel macdiesel commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

Backend support for a "newest courses first" sort on the enterprise Learner Portal search page, via a recency-sorted Algolia virtual replica. This PR builds and configures the replica; the waffle flag (edx-enterprise) and the MFE pointing search at it ship in separate PRs.

It also generalizes replica handling into one settings-driven registry and makes replica configuration fail-safe, so the replica feature and its safe configuration ship together.

What's here

  • Recency replicarecently_released_timestamp (a course's earliest course-run start of any status — the Discovery release date, ENT-11386) added to course records; a replica whose customRanking leads with desc(recently_released_timestamp). Undated courses get 0 so they sort last under a desc ranking (deliberately not the far-future ALGOLIA_DEFAULT_TIMESTAMP, which would float them to the top).
  • Unified, settings-driven replica registryALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS'] (an index_name -> Algolia index settings map in settings/base.py) is the single source of truth for all additional sort replicas, alongside the base REPLICA_INDEX_NAME "by duration" replica. _get_algolia_replica_names, _configured_replicas, configure_algolia_index, and the secured-key restrictIndices (replica_index_names) all read it. A replica is declared/configured/restricted only when its index-name key is set — inert by default (no virtual(None)), so it's safe to deploy ahead of ops enabling it. Adding a future sort is one new map entry plus the field its ranking sorts on. ALGOLIA is in DICT_UPDATE_KEYS, so the deployment YAML is merged (not replaced) — code-defined replica settings are preserved while ops sets per-environment names/credentials.
  • Virtual replicas — replicas are declared virtual(name), so they mirror the primary's records (no added record count or cost). The virtual-vs-standard trade-off (relevance-biased sort under a text query) is documented in ADR 0014.
  • Unified set_index_settings — folded the old primary_index boolean / separate replica method into one name-based set_index_settings(index_settings, index_name=None) routing through _get_index.
  • Fail-safe configuration — each replica's set_index_settings is wrapped so an AlgoliaException is logged and skipped; one replica failing never aborts the reindex (primary + other replicas stay configured), so the degraded state is always the safe base sort. configure_algolia_index fails loudly if the primary index isn't initialized, so a misconfigured reindex can't silently no-op.
  • Docs — ADR 0014 records the design (settings-driven registry, config-vs-flag layering, virtual-vs-standard trade-off, the missing-index open question + operational mitigation); docs/how_to/add_an_algolia_sort_replica.rst walks through adding a future sort end to end.
  • Fixed incremental_reindex_algolia for the unified set_index_settings signature, with a TODO (ENT-11982) to extend it to the full registry and drop the two-client hack at the incremental-reindex cutover.

Test

  • test_algolia_utils.py / test_algolia.py / command tests green (only the 3 pre-existing time-sensitive test_get_course_run* failures remain, which fail on master too). pycodestyle / pylint / isort clean.

Rollout (see ADR 0014)

Deploy + reindex_algolia here before ops sets the MFE env var / enables the enterprise.search_default_sort_newest waffle flag.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.96%. Comparing base (aa42e89) to head (9303a22).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   85.74%   85.96%   +0.22%     
==========================================
  Files         109      109              
  Lines        6735     6771      +36     
  Branches      833      838       +5     
==========================================
+ Hits         5775     5821      +46     
+ Misses        830      821       -9     
+ Partials      130      129       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@macdiesel macdiesel force-pushed the bbeggs/newest-courses-sort-replica branch from ced0c19 to bdb8e35 Compare June 12, 2026 02:04
macdiesel added a commit that referenced this pull request Jun 12, 2026
Configuring the recently-published replica is an optional, additive step. If it
raises AlgoliaException (replica not yet declared, transient error), the failure
would otherwise propagate out of configure_algolia_index and abort the whole
reindex -- taking down configuration of the primary relevance index that every
learner depends on.

Wrap the recency-replica set_index_settings(index_name=...) call so the exception
is logged and skipped, leaving the primary index and its base replica configured.
The degraded state is always the safe base (relevance) sort; the recency replica
catches up on the next successful run. Record this fail-safe in ADR 0014 and cover
it with a test.

Stacked on the new-index PR (#118).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
macdiesel added a commit that referenced this pull request Jun 12, 2026
Configuring the recently-published replica is an optional, additive step. If it
raises AlgoliaException (replica not yet declared, transient error), the failure
would otherwise propagate out of configure_algolia_index and abort the whole
reindex -- taking down configuration of the primary relevance index that every
learner depends on.

Wrap the recency-replica set_index_settings(index_name=...) call so the exception
is logged and skipped, leaving the primary index and its base replica configured.
The degraded state is always the safe base (relevance) sort; the recency replica
catches up on the next successful run. Record this fail-safe in ADR 0014 and cover
it with a test.

Stacked on the new-index PR (#118).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
macdiesel added a commit that referenced this pull request Jun 15, 2026
Configuring an optional sort replica is additive and must never abort the
reindex. Wrap each per-replica set_index_settings(index_name=...) call (now driven
by the registry loop in configure_algolia_index) so an AlgoliaException is logged
and skipped, leaving the primary index and base replica configured. The degraded
state is always the safe base (relevance) sort; the replica catches up on the next
successful run. Record the fail-safe in ADR 0014 and cover it with a test.

Stacked on the new-index PR (#118).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
macdiesel added a commit that referenced this pull request Jun 15, 2026
Configuring a sort replica is additive and must never abort the reindex. Wrap each
per-replica set_index_settings(index_name=...) call in the unified registry loop so
an AlgoliaException is logged and skipped, leaving the primary index and the other
replicas configured. The degraded state is always the safe base (relevance) sort;
the failed replica catches up on the next successful run. Record the fail-safe in
ADR 0014 and cover it with a test (base replica succeeds while recency fails).

Stacked on the new-index PR (#118).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
macdiesel added a commit that referenced this pull request Jun 15, 2026
Configuring a sort replica is additive and must never abort the reindex. Wrap each
per-replica set_index_settings(index_name=...) call in the unified registry loop so
an AlgoliaException is logged and skipped, leaving the primary index and the other
replicas configured. The degraded state is always the safe base (relevance) sort;
the failed replica catches up on the next successful run. Record the fail-safe in
ADR 0014 and cover it with a test (base replica succeeds while recency fails).

Stacked on the new-index PR (#118).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@macdiesel macdiesel changed the title feat: recently-published Algolia replica for "newest courses first" sort feat: "newest courses first" Algolia sort — config-gated replica registry + fail-safe Jun 15, 2026
macdiesel and others added 11 commits June 15, 2026 15:18
…t" sort

Indexes recently_published_timestamp (the course's earliest published run start, sharing the is_course_new_content signal) and configures a desc()-sorted Algolia replica so the Learner Portal search can offer a "newest courses first" sort. The replica is only declared/configured when RECENTLY_PUBLISHED_REPLICA_INDEX_NAME is set (safe no-op until ops provisions it); the name is also added to the secured-API-key restrictIndices so the MFE's scoped key can query it.

ENT-11384

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reframes the docs/references note as docs/decisions/0014, an ADR whose central (proposed) decision is how to handle the recency replica being unavailable: rely on config-gating (covered today) + rollout order + the waffle-flag kill-switch, rather than runtime index-existence detection. Records the runtime-fallback alternative as the escape hatch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ourse-object field

Raises patch coverage on the newest-sort change: refactors the module-level replica-list construction into a testable _build_algolia_replicas() (covers both the configured and unconfigured branches); adds a _algolia_object_from_product course test asserting recently_published_timestamp; and adds generate_secured_api_key tests covering the recency replica being included/omitted from restrictIndices (the property and both branches).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The recency-replica coverage tests call the module-level _build_algolia_replicas
and _algolia_object_from_product helpers directly. Add the method-scoped
# pylint: disable=protected-access used elsewhere in this file so CI lint passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the two-valued primary_index boolean with an optional index_name that
routes through the existing _get_index() helper (which already defaults to the
primary index). This folds the separate set_replica_index_settings method back
into set_index_settings: one method now configures the primary, the base
REPLICA_INDEX_NAME replica, and the recently-published replica uniformly.

Removes the near-duplicate method (same guard + try/except/log) and the docstring
cross-reference that signalled the overlap. All three call sites live in
configure_algolia_index; tests updated to match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The unify refactor removed set_index_settings' primary_index kwarg, breaking this
caller (E1123 unexpected-keyword-arg). Configure the replica by name instead, and
wire the SDK client onto the AlgoliaSearchClient so the index_name lookup
(_get_index) resolves the replica -- the old primary_index=False path used the
cached replica_index handle and didn't need the client set.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Generalize the single recency-replica gate into a registry of optional sort
replicas (OPTIONAL_ALGOLIA_REPLICA_CONFIG_KEYS in api_client/constants, the shared
source of truth for which replicas exist). The indexer (_build_algolia_replicas,
configure_algolia_index) and the secured-API-key restriction now loop over the
registry: each replica is declared, configured, and made queryable ONLY when its
settings.ALGOLIA index-name key is non-empty.

This makes "not in config -> not used" structural and obvious (inert by default,
the safe thing to check in), and reduces a future sort order to a registry entry
plus the field its customRanking sorts on -- no change to the gating logic. The
waffle flag remains the instant, scoped runtime on/off; the config gate only
governs whether the replica infrastructure exists. Updates ADR 0014.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The registry previously covered only the optional sort replicas; the base
duration replica was still special-cased (hardcoded algolia_replica_index,
a dedicated configure call, a separate secured-key append). Generalize so ALL
replicas flow through one registry, base first:

- ALGOLIA_REPLICA_CONFIG_KEYS (api_client/constants) now lists every replica
  (REPLICA_INDEX_NAME, then RECENTLY_PUBLISHED_REPLICA_INDEX_NAME), in
  declaration order, as the single source of truth.
- _build_algolia_replicas / _configured_replicas / configure_algolia_index and
  the secured-key restrictIndices all loop the registry; each replica is
  declared/configured/restricted only when its index name is set. This also
  fixes a latent virtual(None) for the base replica when REPLICA_INDEX_NAME is
  unset.
- The client's init_index/index_exists still eagerly manage the primary +
  base-replica handles -- the required-core-pair lifecycle, intentionally left
  separate from "which replicas get declared/configured."

Updates ADR 0014 and tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
REPLICA_INDEX_NAME is the base replica sorted by desc(duration); the learner-portal
MFE points its video search (SEARCH_INDEX_IDS.VIDEOS) at this index (its
ALGOLIA_REPLICA_INDEX_NAME env var). Document this at the settings default and in
the replica registry, and note that the ALGOLIA dict is replaced (not merged) from
deployment config. No rename -- the key is an established, ops-facing config name.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Configuring a sort replica is additive and must never abort the reindex. Wrap each
per-replica set_index_settings(index_name=...) call in the unified registry loop so
an AlgoliaException is logged and skipped, leaving the primary index and the other
replicas configured. The degraded state is always the safe base (relevance) sort;
the failed replica catches up on the next successful run. Record the fail-safe in
ADR 0014 and cover it with a test (base replica succeeds while recency fails).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Step-by-step engineer guide: declare the settings key, register it in
ALGOLIA_REPLICA_CONFIG_KEYS, index the field the sort needs, define the replica's
customRanking, and map key -> settings. Covers the names-in-settings vs
definitions-in-code split, the deploy/reindex + ALGOLIA-is-replaced caveat, the
optional MFE wiring, and the inert-by-default / fail-safe properties. Cross-refs
ADR 0014 and uses a worked "price: low to high" example.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@macdiesel macdiesel force-pushed the bbeggs/newest-courses-sort-replica branch from c079f32 to da3bfa4 Compare June 15, 2026 19:23
macdiesel and others added 3 commits June 15, 2026 15:23
…art, any status)

Align the newest-courses sort with master's ENT-11386: is_new_content and the sort now
share _earliest_course_run_start, which keys off the earliest course-run start of ANY
status (the Discovery release date) rather than published runs only -- so an old course
with a recent re-publish no longer looks new or sorts as newest.

Rename to match the new semantics (the index-name key is brand-new and unreleased, so no
ops migration is needed):
  recently_published_timestamp           -> recently_released_timestamp
  get_course_recently_published_timestamp -> get_course_recently_released_timestamp
  RECENTLY_PUBLISHED_REPLICA_INDEX_NAME   -> RECENTLY_RELEASED_REPLICA_INDEX_NAME
  ALGOLIA_RECENTLY_PUBLISHED_..._SETTINGS -> ALGOLIA_RECENTLY_RELEASED_..._SETTINGS
  _earliest_published_course_run_start    -> _earliest_course_run_start

Updates tests, ADR 0014, and the how-to guide. Matching MFE rename follows in #1511.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- incremental_reindex_algolia: drop the now-dead replica_index init; the
  replica is configured by name via set_index_settings(index_name=...),
  which initializes it lazily through _get_index().
- configure_algolia_index: log a single-line WARNING when a replica fails
  to configure (set_index_settings already logs the traceback before
  re-raising), so a failed replica no longer emits two stack traces.
- configure_algolia_index: build the primary's declared replicas list
  dynamically from settings.ALGOLIA rather than an import-time global, so
  it stays consistent with the replicas actually configured and can be
  varied in tests via override_settings. Drop the ALGOLIA_REPLICAS global.
- ADR 0014: document that the virtual replica mirrors non-course records,
  which have no recently_released_timestamp and so sort last by design.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@macdiesel macdiesel marked this pull request as ready for review June 15, 2026 19:54
@macdiesel macdiesel requested review from a team as code owners June 15, 2026 19:54
Copilot AI review requested due to automatic review settings June 15, 2026 19:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds backend support for a “newest courses first” Algolia sort by introducing a recency-based replica index, while also consolidating replica handling behind a config-gated registry and making replica configuration resilient to partial failures.

Changes:

  • Adds recently_released_timestamp to course Algolia records and defines a recency-sorted replica (desc(recently_released_timestamp)).
  • Introduces a shared, config-gated replica registry (ALGOLIA_REPLICA_CONFIG_KEYS) and updates index configuration + secured-key restrictions to loop over configured replicas only.
  • Makes replica configuration fail-safe (replica failures are logged and skipped so primary configuration continues) and updates incremental reindex tooling/tests for the unified set_index_settings API.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
enterprise_catalog/settings/base.py Documents and adds the new RECENTLY_RELEASED_REPLICA_INDEX_NAME setting key.
enterprise_catalog/apps/search/management/commands/incremental_reindex_algolia.py Updates incremental reindex flow to configure replica settings via index_name=.
enterprise_catalog/apps/search/management/commands/tests/test_incremental_reindex_algolia.py Adjusts tests for lazy replica initialization via name-based settings calls.
enterprise_catalog/apps/catalog/algolia_utils.py Adds replica registry plumbing, recency replica settings, fail-safe per-replica configuration, and recently_released_timestamp computation.
enterprise_catalog/apps/catalog/tests/test_algolia_utils.py Adds coverage for recency timestamp, configured-replica gating, and safe-fail configuration behavior.
enterprise_catalog/apps/api_client/constants.py Defines ALGOLIA_REPLICA_CONFIG_KEYS as the shared replica registry.
enterprise_catalog/apps/api_client/algolia.py Implements name-based set_index_settings routing and secured-key restriction over all configured replicas.
enterprise_catalog/apps/api_client/tests/test_algolia.py Adds tests for set_index_settings(index_name=...) and restrictIndices behavior with/without the recency replica.
docs/how_to/add_an_algolia_sort_replica.rst Adds a step-by-step guide for adding future Algolia sort replicas via the registry pattern.
docs/decisions/0014-newest-courses-sort-replica.rst Adds ADR documenting the design, rollout strategy, and operational tradeoffs.

Comment thread enterprise_catalog/apps/api_client/algolia.py

@pwnage101 pwnage101 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Logic LGTM! Just have some nits and strong desires to rename things.

Comment thread enterprise_catalog/apps/catalog/algolia_utils.py Outdated
Comment thread enterprise_catalog/apps/catalog/algolia_utils.py Outdated
Comment thread enterprise_catalog/apps/catalog/algolia_utils.py Outdated
Comment thread enterprise_catalog/settings/base.py Outdated
Comment thread docs/decisions/0014-newest-courses-sort-replica.rst
- Rename _build_algolia_replicas -> _get_algolia_replica_names (it returns
  a list of replica names, not replicas) and the local replicas ->
  replica_names; add a list[str] return annotation. Update references in
  tests and the how-to guide.
- Add an int return annotation to get_course_recently_released_timestamp so
  the int return reads clearly despite the datetime-sounding name.
- settings/base.py: document each replica's purpose (the MFE video-search
  sort; the newest-courses sort) rather than the volatile customRanking
  detail, which now lives only in the *_REPLICA_INDEX_SETTINGS dicts.
- ADR 0014: move the design (what the replica does + the rollout) out of
  Context into Decision, leaving Context to background and the problem
  statement.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 16, 2026 01:56
…ilure

When set_index_settings() is called without index_name, it targets the cached
handle (self.algolia_index), which a caller may have wired to an alternate index
(the incremental reindex command's --index-name path). The failure log used
self.algolia_index_name (the configured primary name), which is stale or empty
for that caller. Derive the effective name from the handle instead, falling back
to the configured name. Addresses Copilot review feedback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment on lines +273 to +276
for config_key in ALGOLIA_REPLICA_CONFIG_KEYS:
index_name = settings.ALGOLIA.get(config_key)
if index_name:
configured.append((index_name, ALGOLIA_REPLICA_INDEX_SETTINGS_BY_CONFIG_KEY[config_key]))
Comment on lines +472 to +478
# Declare the replicas dynamically (from the current settings.ALGOLIA) rather than from an
# import-time global, so the set the primary declares stays consistent with the replicas
# _configured_replicas() actually configures below -- and so tests can vary it via
# override_settings.
primary_settings = {**ALGOLIA_INDEX_SETTINGS, 'replicas': _get_algolia_replica_names()}
algolia_client.set_index_settings(primary_settings)
for index_name, index_settings in _configured_replicas():
…initialized client

Addresses two Copilot review comments:
- _configured_replicas(): a configured replica key with no entry in
  ALGOLIA_REPLICA_INDEX_SETTINGS_BY_CONFIG_KEY (registry drift) is now skipped
  with an error log instead of raising KeyError and aborting the reindex --
  consistent with the per-replica fail-safe behavior.
- configure_algolia_index(): fail fast with ImproperlyConfigured when the
  primary index handle is missing (init_index() bailed on missing INDEX_NAME /
  credentials). Since ALGOLIA is replaced per-environment, this prevents a
  misconfigured run from silently no-op'ing and reporting success.

Adds tests for both paths.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines +24 to +27
* An Algolia *virtual* replica exists as soon as it is declared on the primary
index's settings (it mirrors the primary's records); it does not wait for a
populated record set. So once this service is deployed and ``reindex_algolia``
has run, the replica exists.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Selecting virtual replicas would be an Alternative Considered, not part of the Context section. Please move this below.

Also I don't think I agree with this as a foundation for rejecting virtual replicas. Virtual replicas simply don't support fully customizable sort orders, since there's always a built-in "relevance" factor as the top priority sort order. We'd use virtual replicas to save money and record usage, but the tradeoff is less precise and non-deterministic sorting.

Comment on lines +21 to +23
* ``ALGOLIA`` is *replaced* (not merged) from the deployment YAML, so a replica
is only live once ops sets its index-name key in ``edx-internal`` and a
``reindex_algolia`` run declares it on the primary.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AI docs do this a lot where they explain low-level details, rather than what actually matters and what the end goals are. Very few people will read this in the first place, let alone understand it and have the correct takeaway, even if it is factually true.

My version is simpler:

Suggested change
* ``ALGOLIA`` is *replaced* (not merged) from the deployment YAML, so a replica
is only live once ops sets its index-name key in ``edx-internal`` and a
``reindex_algolia`` run declares it on the primary.
* The fundamental design of the enterprise-catalog logic assumes we will only provision one replica, so there is only one setting (``ALGOLIA['REPLICA_INDEX_NAME']``) and one python function to provision that one replica.

Comment on lines +98 to +99
Consequences
------------

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is one missing consequence: cost. Another full replica (non-virtual) doubles our record count.

return settings.ALGOLIA.get('REPLICA_INDEX_NAME')

@property
def replica_index_names(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the amount of misdirection here is tripping me up. can we make it more directly settings-driven, something like:

# settings/base.py
ALGOLIA = {
    'INDEX_NAME': '',
    # Base replica: the long-standing sort the learner-portal MFE points its video search
    # (SEARCH_INDEX_IDS.VIDEOS) at -- the MFE's ALGOLIA_REPLICA_INDEX_NAME env var. Its
    # customRanking lives in ALGOLIA_REPLICA_INDEX_SETTINGS.
    'REPLICA_INDEX_NAME': '',
    'ADDITIONAL_REPLICA_INDEX_SETTINGS': {'newest_courses_index_repl': 'ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS'}, 
    'APPLICATION_ID': '',
    'API_KEY': '',
}

this defines both the additional replica index names that are enabled and which algolia index setting to use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but actually, that's still a lot of indirection, which is a code smell (with the original settings/algolia_utils.py relationship, not with your code). can we just yoink the settings values from algolia_utils into settings?

ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS = {
    'customRanking': [
        'desc(recently_released_timestamp)',
        'asc(metadata_language)',
        'asc(visible_via_association)',
        'asc(created)',
        'desc(course_bayesian_average)',
        'desc(recent_enrollment_count)',
    ],
}
ALGOLIA = {
    'INDEX_NAME': '',
    # Base replica: the long-standing sort the learner-portal MFE points its video search
    # (SEARCH_INDEX_IDS.VIDEOS) at -- the MFE's ALGOLIA_REPLICA_INDEX_NAME env var. Its
    # customRanking lives in ALGOLIA_REPLICA_INDEX_SETTINGS.
    'REPLICA_INDEX_NAME': '',
    'ADDITIONAL_REPLICA_INDEX_SETTINGS': {'newest_courses_index_repl': ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS}, 
    'APPLICATION_ID': '',
    'API_KEY': '',
}

in a later PR, we could clean up the rest of the algolia settings later to follow a similar pattern. I think that will be much more straightforward everywhere.

Copilot AI review requested due to automatic review settings June 16, 2026 14:03
…text)

Addresses Troy's ADR comments:
- Context: state the pre-existing single-replica design assumption (one setting,
  one provisioning function) as historical background, and frame the problem as
  adding a *second* replica.
- Decision: state explicitly that we declare a *virtual* replica.
- Alternatives considered: add 'standard (non-virtual) replica' -- deterministic
  newest-first but ~doubles record count/cost; we accept virtual's relevance-bias
  tradeoff to avoid that. Documents the escape hatch.
- Consequences: add the 'no added record cost' consequence (a standard replica
  would roughly double the indexed record count).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

macdiesel and others added 2 commits June 16, 2026 11:07
…A_INDEX_SETTINGS)

Per iloveagent57's review, collapse the replica registry's indirection into a single
settings-driven map:

- settings/base.py: define ALGOLIA['ADDITIONAL_REPLICA_INDEX_SETTINGS'], an
  index_name -> index settings map (config-as-code), and move the recency
  customRanking constant (ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS) here.
- settings/production.py: add 'ALGOLIA' to DICT_UPDATE_KEYS so the deployment YAML
  is merged (not replaced) -- ops can override per-env index names/credentials while
  the code-defined replica settings are preserved.
- Remove ALGOLIA_REPLICA_CONFIG_KEYS (constants.py) and
  ALGOLIA_REPLICA_INDEX_SETTINGS_BY_CONFIG_KEY (algolia_utils.py); _get_algolia_replica_names,
  _configured_replicas, and replica_index_names now read settings.ALGOLIA directly
  (base REPLICA_INDEX_NAME + ADDITIONAL_REPLICA_INDEX_SETTINGS keys). No more cross-module
  registry, so the constants.py circular-import workaround is gone and the settings-map
  drift (a KeyError risk Copilot flagged) is structurally impossible.

Behavior change: the recency replica now ships declared in code (always declared in
every env once reindexed), rather than being inert until ops set an index name. The
replica is virtual (no record cost), and user-facing exposure is still gated by the
enterprise.search_default_sort_newest waffle flag + Optimizely experiment.

Updates ADR 0014 and the how-to guide; rewrites the affected tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 16, 2026 16:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Comment on lines +453 to +457
# init_index() logs and returns without setting algolia_index when a required name
# (INDEX_NAME / REPLICA_INDEX_NAME) or credential is missing. set_index_settings() would
# then silently no-op for every index, making a misconfigured reindex look successful.
# Since ALGOLIA is replaced (not merged) per-environment, fail loudly here instead.
raise ImproperlyConfigured(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 9303a22 — removed the inaccurate "ALGOLIA is replaced (not merged)" justification (it's merged via DICT_UPDATE_KEYS). The fail-loud guard stands on its own: a missing/empty primary index name would otherwise pass as a successful reindex.

Comment on lines +110 to +113
#. The replica's name and settings ship in code (``ADDITIONAL_REPLICA_INDEX_SETTINGS``), so no
edx-internal change is required to *declare* it. If an environment needs a different index name,
ops overrides ``ALGOLIA['ADDITIONAL_REPLICA_INDEX_SETTINGS']`` in edx-internal -- because
``ALGOLIA`` is merged, only the keys ops sets are overridden.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Clarified in 9303a22 — the doc now states the ALGOLIA merge is shallow, so setting ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS in edx-internal replaces the whole map rather than overriding individual entries.

Comment on lines +4 to +8
Status
------

Proposed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed to "Accepted" in 9303a22 to match the convention in 0012/0013 (this PR implements the decision).

Comment on lines +112 to +116
* **Not covered in code:** "the replica is pointed at by the MFE but its Algolia index
does not exist yet" (e.g. the MFE env var is set before this service has been deployed
and reindexed) → the MFE would query a missing index and surface an error / empty
results rather than falling back. This is a transient, operator-controlled window
mitigated by the rollout order and the kill-switch flag, not by code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this "consequence" isn't a consequence if the decision is actually followed. Consider just dropping it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Dropped in 9303a22. The missing-index open question is still recorded in the Decision section, so we don't lose the trade-off.

Comment thread enterprise_catalog/settings/base.py Outdated
Comment on lines +468 to +470
'ADDITIONAL_REPLICA_INDEX_SETTINGS': {
'enterprise_catalog_recently_released_desc': ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS,
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Future us will really appreciate current us adding "VIRTUAL" to the setting name.

Suggested change
'ADDITIONAL_REPLICA_INDEX_SETTINGS': {
'enterprise_catalog_recently_released_desc': ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS,
},
'ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS': {
'enterprise_catalog_recently_released_desc': ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS,
},

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 9303a22 — renamed ADDITIONAL_REPLICA_INDEX_SETTINGSADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS everywhere (settings, client, utils, tests, docs). Key stays enterprise_catalog_recently_released_desc.

logged and the reindex continues with the primary index and the other replicas
intact, so a problem with one sort can never take down core search indexing.
The worst case is that one replica lags its ranking until the next run — never a
broken or empty primary index.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: all the wording on this one is just weird. If I'm reading it correctly, it's just saying that we make a best effort to configure a replica, and any errors on configuration won't crash initialization. That's an implementation detail, not a consequence. Please drop this bullet too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed — dropped in 9303a22.

Comment on lines +122 to +126
# Target a non-primary index: wire the SDK client and the primary handle directly so
# set_index_settings() resolves to this alternate index. The replica is configured by
# name, which goes through _get_index() -> self._client.init_index(replica_name), so
# the underlying client must be set (init_index() would target the production index).
algolia_client._client = sdk_client # pylint: disable=protected-access

@pwnage101 pwnage101 Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a code smell beget by another code smell. Normal usage of an API client shouldn't require re-plumbing the client internals. This seems like a hack to workaround the original code having a hack which was to create two algolia clients (sdk_client + algolia_client) to workaround the wrapper AlgoliaSearchClient not having parameterized init_index to support passing an arbitrary index name.

This is layered, so I'd understand if you wanted to defer a fix. Eventually this management command will be used in production and will need to be refactored anyway, so at least add a comment explaining that this hack should be a temporary workaround, and also point out that sdk_client = new_search_client_or_error() above is also a hack.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a comment in 9303a22 calling out that both the client-internals re-plumbing and the second new_search_client_or_error() client are temporary workarounds for AlgoliaSearchClient.init_index() not taking an arbitrary index name. Filed ENT-11982 for the real fix (parameterize init_index()) at the incremental-reindex cutover: https://2u-internal.atlassian.net/browse/ENT-11982

Comment on lines 56 to 64
parser.add_argument(
'--replica-name',
dest='replica_index_name',
default=None,
help=(
'Algolia replica index name. '
'Defaults to the index name suffixed with "_repl".'
),
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You and/or claude has decided to just give up on extending new approach to this management command. I'd like to point out that this management command is not some legacy thing, it triggers the new and improved incremental indexing approach Alex has been working on. If you don't want to enhance it right now, that's fine, but at least put in a TODO comment with an ENT ticket pointer so we don't lose track of the tech debt.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a TODO (ENT-11982) in 9303a22. ENT-11982 tracks extending this command to loop the unified replica registry (it currently configures only the base replica) and removing the two-client hack: https://2u-internal.atlassian.net/browse/ENT-11982

The settings-driven refactor removed the block that sat above _configured_replicas,
leaving a single blank line before the def. Restore the second blank line so
pycodestyle (make style) passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 18, 2026 17:31
…fixes)

Addresses outstanding review comments from pwnage101 and Copilot:

- Rename ALGOLIA['ADDITIONAL_REPLICA_INDEX_SETTINGS'] ->
  ALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS'] (the replicas are
  declared as virtual()) across settings, client, utils, tests, and docs.
- configure_algolia_index: drop the now-incorrect "ALGOLIA is replaced
  (not merged)" justification -- ALGOLIA is merged via DICT_UPDATE_KEYS.
- how-to: clarify the ALGOLIA merge is shallow, so overriding the replica
  map replaces it wholesale rather than per-key.
- settings/base.py: describe the base replica accurately as a "by duration"
  sort (its customRanking leads with desc(duration)), not "relevance".
- ADR 0014: status Proposed -> Accepted (matches 0012/0013); drop two
  Consequences bullets that were implementation details, not consequences.
- incremental_reindex_algolia: add TODO (ENT-11982) flagging that the
  command still configures only the base replica and re-plumbs client
  internals as a temporary workaround.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comment on lines +469 to +471
'ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS': {
'enterprise_catalog_recently_released_desc': ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS,
},
Comment on lines +435 to +439
# The per-environment index names and credentials (INDEX_NAME, REPLICA_INDEX_NAME, APPLICATION_ID,
# API_KEY) are populated from the deployment config (edx-internal). ALGOLIA is listed in
# DICT_UPDATE_KEYS (see settings/production.py), so the deployment YAML is *merged* into this dict
# rather than replacing it -- per-environment keys override the defaults below while code-defined
# defaults like ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS are preserved.
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.

4 participants