169395632 v7 2 changes #69
Conversation
in v7.0, the 'total' on a search results will return an object instead of an integer so we need to be able to handle that along with the older integer value
- update elasticsearch gem for compatibility with target ES version
- handle 'total' object (instead of scalar)
- allow for optional timestamp format without colon (":")
- expose refresh_index to force writes because flush_index will no force writes
- option param 'include_type_name_on_create' that control the :include_type_name argument used when creating indexes
davidbl
force-pushed
the
169395632_v7_2_changes
branch
from
b2480d2 to
47b1370
Compare
davidbl
force-pushed
the
169395632_v7_2_changes
branch
from
1ef2835 to
ddc351e
Compare
davidbl
force-pushed
the
169395632_v7_2_changes
branch
from
edab08e to
c1bbaa3
Compare
davidbl
force-pushed
the
169395632_v7_2_changes
branch
from
c1bbaa3 to
192133d
Compare
| @@ -71,10 +72,16 @@ def remap!(retry_delete_on_recoverable_errors: true, retry_delay: 30, max_delay: | |||
| end | |||
|
|
|||
| # Flushes the index, forcing any writes | |||
| # note that v7 no longer forces any writes on flush | |||
| def flush_index | |||
There was a problem hiding this comment.
would emitting a deprecation log warning be useful here, or is flush_index still have value if it doesn't force the writes?
There was a problem hiding this comment.
flush is still a valid thing
https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-flush.html#flush-api-desc
it just want have the side-effect of refreshing the index which made new documents visible to searches (the side-effect that is has pre v7)
| def build_index_name | ||
| ts = String.new | ||
| if @use_new_timestamp_format == true | ||
| ts = Time.now.utc.strftime("%Y%m%d%H%M%S%6N") |
There was a problem hiding this comment.
Any chance we could add some delimiters to make this easier to parse by humans reading the index names? Something like 2019_10_27_211410 (not sure if _ is an acceptable delimiter, but whatever is could be useful here.
There was a problem hiding this comment.
yeah, we could. but this format should be familiar to rails devs
https://github.com/doximity/doximity/tree/master/db/migrate (which is why I picked it)
There was a problem hiding this comment.
and I was a bit scared of some future upgrade declaring other chars off limits
There was a problem hiding this comment.
Yeah - I was thinking that same thing about the chars too. I'm good either way, just figured I'd mention it.
There was a problem hiding this comment.
👍
fwiw, most human interaction is usually via the aliases anyway
|
@divoxx yes. I made the v7-specific stuff opt-in so, by default, things should be compatible with older ES versions. But we probably need to consider sun-setting support for truly old versions |
|
The testing of these changes is complete (see https://github.com/doximity/doximity/pull/30797) And I have removed the |
PT story https://www.pivotaltracker.com/story/show/169395632
Associated doximity PR https://github.com/doximity/doximity/pull/30797
Overview
There are breaking changes in Elasticsearch v7.0 that we need to address in order to upgrade
Technical Details
Additional information available here https://docs.google.com/document/d/1pGgAdDeHEclh7nr_MX_RkryCgyf2IEtUQGyfbYMAh9k/edit#heading=h.oymnw3nlvwib