Merge lp://staging/~adeuring/charmworld/1206659-simpler-es-mapping into lp://staging/~juju-jitsu/charmworld/trunk

Proposed by Abel Deuring
Status: Merged
Approved by: Abel Deuring
Approved revision: 353
Merged at revision: 357
Proposed branch: lp://staging/~adeuring/charmworld/1206659-simpler-es-mapping
Merge into: lp://staging/~juju-jitsu/charmworld/trunk
Diff against target: 263 lines (+144/-10)
5 files modified
charmworld/migrations/versions/016_fixed_ES_mapping_for_charms.py (+8/-0)
charmworld/migrations/versions/tests/test_migrations.py (+20/-0)
charmworld/search.py (+51/-4)
charmworld/tests/test_models.py (+6/-5)
charmworld/tests/test_search.py (+59/-1)
To merge this branch: bzr merge lp://staging/~adeuring/charmworld/1206659-simpler-es-mapping
Reviewer Review Type Date Requested Status
Abel Deuring (community) Approve
Curtis Hovey (community) code Approve
Review via email: mp+181100@code.staging.launchpad.net

Commit message

Define a (mostly) static mapping for charms.

Description of the change

This branch is a first step to fix bug 1206659: Charmworld indexes far too
many fields due to Elasticsearch dynamic mapping.

Limitations:

  - It defines a static mapping only for charms, not for bundles
  - The attributes "requires" and "provides" of a charm still have
    a dynamic mapping.

Details:

The method ElasticSearchClient.put_mapping() now defines a static mapping
for charms. All fields needed for text searching, for filtering and ordering
are indexed, but no other fields.

charm['requires'] and charm['provides'] are dictionaries with user-defined
keys, hence it is impossible to define a static mapping. Fixing bug
1194907
will allow to define a static mapping for these fields too.

test_mapping_changes_during_indexing() shows that indexing a charm
changes only the mapping for requires/provides.

TestCharmSource.test_save_deletes_on_error() needed a change: Attempts
to index two charm with different types of a property named "b" now just
work because charm['b'] is now ignored by ES.

Similary, TestCharmSource.test_sync_index_script() needed a change
because the field "maintainer" is no longer indexed. (When I tried to
fix this test, I noticed bug 1214328.)

Testing the new migration showed that ES considers the new static mapping
to be compatible with the old dynamic mapping: If
ElasticSearchClient.put_mapping() is called when the index already contains
data with the previous dynamic mapping, the new index specifications are
simply accepted but the mapping stays dynamic. Hence I added the parameter
force_reindex to charmworld.search.update().

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Able.

I think our intent is to treat charm_text_index, charm_integer_index as constants. Can you capitalise these identifiers and change them from lists to tuples (to ensure nothing can mutate them)?

Please place an XXX in front of the comment on line 230 because we intend to fix this issue soon.

I think this is good to land.

review: Approve (code)
353. By Abel Deuring

implemented reviewer's comments.

Revision history for this message
Abel Deuring (adeuring) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches