Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
AO3-4815 Update Elasticsearch and filter interface #3104
Conversation
WendyBeth
added some commits
Aug 9, 2017
WendyBeth
added some commits
Sep 28, 2017
| require 'spec_helper' | ||
| -describe WorkSearch do | ||
| +deprecate_old_elasticsearch_test do | ||
| + describe WorkSearch do |
| end | ||
| - describe "when searching restricted works" do | ||
| + describe '#search_results' do |
| + | ||
| + # ES UPGRADE TRANSITION # | ||
| + # Remove $rollout activation & unless block | ||
| + $rollout.activate :start_new_indexing |
| + # Remove $rollout activation & unless block | ||
| + $rollout.activate :start_new_indexing | ||
| + | ||
| + unless elasticsearch_enabled?($elasticsearch) |
| + $rollout.activate :start_new_indexing | ||
| + | ||
| + unless elasticsearch_enabled?($elasticsearch) | ||
| + $rollout.activate :stop_old_indexing |
| + | ||
| + unless elasticsearch_enabled?($elasticsearch) | ||
| + $rollout.activate :stop_old_indexing | ||
| + $rollout.activate :use_new_search |
| - Bookmark.tire.index.delete | ||
| - Bookmark.create_elasticsearch_index | ||
| + ['work', 'bookmark', 'pseud', 'tag'].each do |index| |
| +# ES UPGRADE TRANSITION # | ||
| +# Remove method | ||
| +def elasticsearch_enabled?(elasticsearch_instance) | ||
| + elasticsearch_instance.cluster.health rescue nil |
| +# ES UPGRADE TRANSITION # | ||
| +# Remove method | ||
| +def deprecate_old_elasticsearch_test | ||
| + deprecate_unless(elasticsearch_enabled?($elasticsearch)) do |
| +def update_and_refresh_indexes(klass_name) | ||
| + # ES UPGRADE TRANSITION # | ||
| + # Remove block | ||
| + if elasticsearch_enabled?($elasticsearch) |
| + indexer = indexer_class.new(klass_name.capitalize.constantize.all.pluck(:id)) | ||
| + indexer.index_documents if klass_name.capitalize.constantize.any? | ||
| + | ||
| + $new_elasticsearch.indices.refresh(index: "ao3_test_#{klass_name}s") |
| +def delete_index(index) | ||
| + # ES UPGRADE TRANSITION # | ||
| + # Remove block | ||
| + if elasticsearch_enabled?($elasticsearch) |
| + | ||
| + # NEW ES | ||
| + index_name = "ao3_test_#{index}s" | ||
| + if $new_elasticsearch.indices.exists? index: index_name |
| + # NEW ES | ||
| + index_name = "ao3_test_#{index}s" | ||
| + if $new_elasticsearch.indices.exists? index: index_name | ||
| + $new_elasticsearch.indices.delete index: index_name |
| +end | ||
| + | ||
| +def delete_test_indices | ||
| + indices = $new_elasticsearch.indices.get_mapping.keys.select { |key| key.match("test") } |
| +def delete_test_indices | ||
| + indices = $new_elasticsearch.indices.get_mapping.keys.select { |key| key.match("test") } | ||
| + indices.each do |index| | ||
| + $new_elasticsearch.indices.delete(index: index) |
ariana-paris
changed the title from
Ao3 4815 update elasticsearch and filter interface
to
AO3-4815 Update Elasticsearch and filter interface
Oct 13, 2017
redsummernight
referenced this pull request
Oct 13, 2017
Open
AO3-3468 Changing User Name doesn't accurately update Search results #2762
| @@ -644,17 +644,3 @@ function thermometer() { | ||
| } | ||
| }); | ||
| } | ||
| - | ||
| -function updateCachedTokens() { |
sarken
Oct 14, 2017
Owner
I think recent changes to the kudos function got overridden above (lines 457-468) as well, as did some recent CSS changes, which I'll comment on separately.
| @@ -41,10 +41,6 @@ http://otwcode.github.com/docs/front_end_coding/patterns/supertypes | ||
| background: url("/images/ao3_logos/logo-ruby.png") top right no-repeat; | ||
| } | ||
| -#main.session { |
| @@ -226,7 +226,6 @@ li.blurb:after, .blurb .blurb:after { | ||
| } | ||
| .claim .datetime { | ||
| - margin-left: 65px; |
| @@ -212,8 +212,10 @@ SUBSECTIONS: | ||
| /* 2. ZONE: SYSTEM: SESSIONS (login and signup)*/ | ||
| -.session #signin { |
| @@ -90,10 +90,6 @@ body .narrow-shown { | ||
| background-position: center; | ||
| } | ||
| -#main.session { |
sarken
Oct 14, 2017
Owner
It looks like some recent changes got clobbered in a merge conflict -- the stuff that's being removed here should stay.
sarken
added
the
Awaiting review
label
Oct 15, 2017
| @@ -14,6 +14,9 @@ def self.perform(name) | ||
| REDIS.del(name) | ||
| end | ||
| + def self.index(klass, ids, priority) |
elzj
Oct 18, 2017
Owner
Looks like this is missing some things - I just submitted a PR to your branch to flesh that out, since I was doing some batch indexing to test things out locally.
| @@ -77,6 +79,73 @@ def process_options | ||
| self.options.delete_if { |key, value| value.blank? } | ||
| end | ||
| + # Useful for inspecting the differences between the old ES query and the new | ||
| + # ES query, when debugging problems between ES versions | ||
| + def search_query |
elzj
Oct 18, 2017
Owner
I always meant to split this out, and I agree that it's useful, but even for temp code, can we avoid duplicating this? I think the only difference between this code and the query in search_results is the lack of pagination here - is there any way to add that back and then just call search_query within search_results?
| mapping do | ||
| indexes :id, index: :not_analyzed | ||
| - indexes :name, analyzer: 'snowball', boost: 100 | ||
| + indexes :name#, analyzer: 'snowball', boost: 100 |
| ) | ||
| end | ||
| def pseud_ids | ||
| creatorships.pluck :pseud_id | ||
| end | ||
| + def user_ids | ||
| + Pseud.where(id: pseud_ids).pluck(:id) |
| + # Useful for inspecting the differences between the old ES query and the new | ||
| + # ES query, when debugging problems between ES versions | ||
| + def search_query | ||
| + self.options ||= {} |
WendyBeth
added some commits
Oct 18, 2017
| + } | ||
| + } | ||
| + | ||
| + $new_elasticsearch.reindex(body: request_body) |
| + end | ||
| + | ||
| + def get_settings(index) | ||
| + request = JSON.parse($elasticsearch.perform_request('GET', "#{index}/_settings").to_json) |
| + def make_mappings_compatible_with_new_es(mappings) | ||
| + new_mappings = mappings.dup | ||
| + | ||
| + new_mappings.each do |type, type_properties| |
houndci-bot
Oct 18, 2017
Unused block argument - type. If it's necessary, use _ or _type as an argument name to indicate that it won't be used.
| + properties = type_properties["properties"] | ||
| + | ||
| + properties.keys.each do |key| | ||
| + |
| + if properties[key] == "string" | ||
| + properties[key] = "text" | ||
| + end | ||
| + |
| + | ||
| + new_mappings | ||
| + end | ||
| + |
| @@ -16,9 +16,38 @@ def successful_reindex(ids) | ||
| def enqueue_to_index | ||
| if Rails.env.test? | ||
| - update_index and return | ||
| + reindex_document and return |
| + def reindex_document | ||
| + # ES UPGRADE TRANSITION # | ||
| + # Remove `update_index rescue nil` | ||
| + update_index rescue nil |
| + # ES UPGRADE TRANSITION # | ||
| + # Remove outer conditional | ||
| + if self.class.use_new_search? | ||
| + index_name = self.is_a?(Tag) ? 'tag' : self.class.to_s.downcase |
| + # Remove outer conditional | ||
| + if self.class.use_new_search? | ||
| + index_name = self.is_a?(Tag) ? 'tag' : self.class.to_s.downcase | ||
| + doc_type = self.is_a?(Tag) ? 'tag' : self.class.document_type |
| + index = { | ||
| + index: "ao3_#{Rails.env}_#{index_name}s", | ||
| + type: doc_type, | ||
| + id: self.id, |
| + index: "ao3_#{Rails.env}_#{index_name}s", | ||
| + type: doc_type, | ||
| + id: self.id, | ||
| + body: self.document_json |
| + body: self.document_json | ||
| + } | ||
| + | ||
| + if self.is_a?(Bookmark) |
| + } | ||
| + | ||
| + if self.is_a?(Bookmark) | ||
| + index.merge!( |
houndci-bot
Oct 18, 2017
Use index[:routing] = "#{self.bookmarkable_type}-#{self.bookmarkable_id}" instead of `index.merge!(
| + | ||
| + if self.is_a?(Bookmark) | ||
| + index.merge!( | ||
| + routing: "#{self.bookmarkable_type}-#{self.bookmarkable_id}" |
| + | ||
| + # ES UPGRADE TRANSITION # | ||
| + # Replace $new_elasticsearch with $elasticsearch | ||
| + $new_elasticsearch.index index |
| + work_search = self | ||
| + | ||
| + search = Tire.search 'work' do | ||
| + query do |
| + must { terms :pseud_ids, search_opts[:pseud_ids] } | ||
| + end | ||
| + | ||
| + [:rating_ids, :warning_ids, :category_ids, :fandom_ids, :character_ids, :relationship_ids, :freeform_ids].each do |id_list| |
| + end | ||
| + end | ||
| + | ||
| + [:filter_ids, :collection_ids].each do |id_list| |
| + end | ||
| + | ||
| + [:filter_ids, :collection_ids].each do |id_list| | ||
| + if search_opts[id_list].present? |
| + end | ||
| + end | ||
| + | ||
| + [:word_count, :hits, :kudos_count, :comments_count, :bookmarks_count, :revised_at].each do |countable| |
| +# ES UPGRADE TRANSITION # | ||
| +# Remove $new_elasticsearch definition, since ELASTICSEARCH_1_URL should point | ||
| +# to new ES instance |
| +# Remove $new_elasticsearch definition, since ELASTICSEARCH_1_URL should point | ||
| +# to new ES instance | ||
| +$elasticsearch = Elasticsearch::Client.new host: ArchiveConfig.ELASTICSEARCH_1_URL |
| @@ -9,7 +9,7 @@ | ||
| step %{I start a new bookmark for "#{title}"} | ||
| fill_in("bookmark_tag_string", with: DEFAULT_BOOKMARK_TAGS) | ||
| step %{I press "Create"} | ||
| - Bookmark.tire.index.refresh | ||
| + step %{the bookmark indexes are updated} |
| @@ -179,6 +179,7 @@ | ||
| step %{I follow "Collection Settings"} | ||
| step %{I uncheck "This collection is anonymous"} | ||
| step %{I press "Update"} | ||
| + step %{the work indexes are updated} |
| @@ -1,12 +1,34 @@ | ||
| +require "cucumber/rspec/doubles" |
| + es_update(klass) | ||
| + # ES UPGRADE TRANSITION # | ||
| + # Remove unless block | ||
| + unless !elasticsearch_enabled?($elasticsearch) |
houndci-bot
Oct 18, 2017
Favor if over unless for negative conditions.
Do not introduce global variables.
| + end | ||
| +end | ||
| + | ||
| + |
| - [Work, Bookmark, Pseud, Tag].each do |klass| | ||
| - klass.import | ||
| - klass.tire.index.refresh | ||
| + ['work', 'bookmark', 'pseud', 'tag'].each do |klass| |
| +Given /^the (\w+) indexes are reindexed$/ do |model| | ||
| + # ES UPGRADE TRANSITION # | ||
| + # Change $new_elasticsearch to $elasticsearch | ||
| + $new_elasticsearch.indices.refresh index: "ao3_test_#{model}s" |
| end | ||
| +Given /^all search indexes are reindexed$/ do | ||
| + ['work', 'bookmark', 'pseud', 'tag'].each do |model| |
| + | ||
| +When /^(\w+) can use the new search/ do |login| | ||
| + user = User.find_by(login: login) | ||
| + $rollout.activate_user(:use_new_search, user) |
|
Okay, merging this and we can submit new pull requests with further updates. |
elzj
merged commit 40cd564
into
otwcode:master
Oct 20, 2017
|
Currently, the new elasticsearch is not installed on the vagrant machine, so all tests fail on it. |
added a commit
that referenced
this pull request
Nov 27, 2017
added a commit
that referenced
this pull request
Nov 27, 2017
added a commit
that referenced
this pull request
Nov 30, 2017
added a commit
to ariana-paris/otwarchive
that referenced
this pull request
Dec 3, 2017
added a commit
to ariana-paris/otwarchive
that referenced
this pull request
Dec 3, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
WendyBeth commentedOct 13, 2017
Pull Request Checklist
AO3-1234 Fix thing)Issue
https://otwarchive.atlassian.net/browse/AO3-4815
Purpose
What does this PR do?
This PR
Testing
How can the Archive's QA team verify that this is working as you intended? (If you have access, please copy this into the JIRA ticket for them!)
Due to the extensive nature of this change, testing will require a full run-through of all search-related functionality on the site. This includes creating, updating, and deleting indices for all four searched models (Work, Bookmark, Pseud, and Tag) and all searching and filtering.
References
Are there any other relevant issues / PRs / mailing lists discussions related to this?
Credit
What name do you want us to use to credit your work in the Archive of Our Own's Release Notes?
Wendy Randquist - Littlelines
What pronouns do you prefer (she/her, he/him, zie/hir etc)?
she/her
(if you do not fill this in, we will use your GitHub account name and will use "they" to avoid making assumptions about your gender)