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.

Sign up for GitHub

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

Jump to bottom

Upgrade to ES v7 #55

Merged
merged 5 commits into from Sep 7, 2022
Merged

Upgrade to ES v7 #55

merged 5 commits into from Sep 7, 2022

Conversation

denisacostaq
Copy link
Contributor

@denisacostaq denisacostaq commented Mar 14, 2022 •
edited

Description

What

Please be specific and try to describe your thought process. State the obvious, since this might be the first time the reviewer is looking at the code

Why

https://financialtimes.atlassian.net/browse/UPPSF-2702

Anything, in particular, you'd like to highlight to reviewers

Mention here sections of code which you would like reviewers to pay extra attention to .E.g

  • Migrate schema to ES 7.10
  • Document type concept have been removed in ES 6

Scope and particulars of this PR (Please tick all that apply)

  • Tech hygiene (dependency updating & other tech debt)
  • Bug fix
  • Feature
  • Documentation
  • Breaking change
  • Minor change (e.g. fixing a typo, adding config)

This Pull Request follows the rules described in our Pull Requests Guide

@denisacostaq denisacostaq requested a review from a team as a code owner March 14, 2022 11:01
@denisacostaq denisacostaq self-assigned this Mar 14, 2022
@denisacostaq denisacostaq marked this pull request as draft March 14, 2022 11:01
@denisacostaq denisacostaq marked this pull request as ready for review March 14, 2022 14:52
@coveralls
Copy link

coveralls commented Mar 15, 2022 •
edited

Coverage Status

Coverage increased (+0.01%) to 95.633% when pulling 15700b7 on feature/elasticsearch7-update into b117ae8 on master.

@denisacostaq denisacostaq force-pushed the feature/elasticsearch7-update branch 18 times, most recently from 7932fe0 to 6b28c42 Compare March 16, 2022 16:11
atanasdinov
atanasdinov suggested changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

Let's clear up the commit history by rebasing.

go.mod Outdated Show resolved Hide resolved
pkg/es/service.go
s.mu.Lock()
defer s.mu.Unlock()
return s.ElasticClient.Index().
Index(s.IndexName).
Type(conceptType).
Copy link
Contributor

Choose a reason for hiding this comment

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

So having the type as part of the model is equivalent to the old .Type() call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, in ES 6 the "type concept" was removed, but we still need to use this, so this is a way to mimic (differentiate among different document types)
https://www.elastic.co/blog/removal-of-mapping-types-elasticsearch

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thank you!

pkg/mapper/mapper.go Outdated Show resolved Hide resolved
configs/migrate.sh
@@ -0,0 +1,79 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the script that generated the changes to referenceSchema.json? Is it reliable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, it was at some point, but for the last changes was too difficult to express it in jq, maybe is a good idea to keep it as a documentation that represent a bit the changes, maybe not because is not totally compatible.

@denisacostaq denisacostaq force-pushed the feature/elasticsearch7-update branch 2 times, most recently from 7ede4dd to f050c92 Compare March 17, 2022 09:58
@atanasdinov atanasdinov force-pushed the feature/elasticsearch7-update branch from cfb797c to f050c92 Compare March 17, 2022 10:30
@denisacostaq denisacostaq force-pushed the feature/elasticsearch7-update branch 2 times, most recently from 8573aed to 3e58dc0 Compare March 21, 2022 09:28
@denisacostaq denisacostaq force-pushed the feature/elasticsearch7-update branch from 3e58dc0 to 666d9fd Compare April 6, 2022 19:35
atanasdinov
atanasdinov suggested changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

Let's update the Helm configuration to use the new ES endpoint from the global config: aws.content.elasticsearch.v2.endpoint.

Also we need to rebase the changes from the master.

pkg/es/service.go Outdated Show resolved Hide resolved
@atanasdinov atanasdinov requested a review from a team April 14, 2022 15:29
@denisacostaq denisacostaq force-pushed the feature/elasticsearch7-update branch from 051fa86 to 281cef8 Compare May 4, 2022 11:38
@atanasdinov atanasdinov force-pushed the feature/elasticsearch7-update branch 5 times, most recently from 3b3bd0b to 7ddfcbe Compare May 31, 2022 09:10
@atanasdinov atanasdinov force-pushed the feature/elasticsearch7-update branch from 7ddfcbe to 70220e0 Compare May 31, 2022 09:22
denisacostaq and others added 3 commits May 31, 2022 12:29
@denisacostaq @atanasdinov
Migrate to elasticsearch7
6dd50b1 
Cherri-pick Conflicts:
	go.mod
	go.sum
	pkg/message/message_handler_test.go
@denisacostaq @atanasdinov
Fix schema mapping
be6904f
@atanasdinov
Set ES V2 API endpoint in config
6ae9838
@atanasdinov atanasdinov force-pushed the feature/elasticsearch7-update branch from 70220e0 to 6ae9838 Compare May 31, 2022 09:30
@atanasdinov atanasdinov changed the title Feature/elasticsearch7 update Upgrade to ES v7 Sep 7, 2022
@atanasdinov atanasdinov merged commit a0ae8fd into master Sep 7, 2022
@atanasdinov atanasdinov deleted the feature/elasticsearch7-update branch September 7, 2022 08:54
@MiroslavGatsanoga
Copy link

🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atanasdinov atanasdinov atanasdinov approved these changes

@martin-stanchev martin-stanchev martin-stanchev approved these changes

Assignees

@denisacostaq denisacostaq

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@denisacostaq @coveralls @MiroslavGatsanoga @atanasdinov @martin-stanchev