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.

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

elastic search v8にアップデート #2126

Merged
merged 3 commits into from
Dec 7, 2023
Merged

elastic search v8にアップデート #2126

merged 3 commits into from
Dec 7, 2023

Conversation

ikura-hamu
Copy link
Member

elasticsearchの設定ファイル(elasticsearch.yml)をbind mountすると動かないので、compose.ymlで環境変数から指定しています。
参考: https://discuss.elastic.co/t/when-mounting-elasticsearch-yml-docker-displays-device-or-resource-busy/300981
elasticsearch.ymlは残してあるのですが、消しちゃってもいいですか?

動作確認は下のように行いました。

  1. v7(master)の状態でいくつかメッセージを投稿して、esにデータが入ることを確認
  2. v8(このブランチ)にして、make up
  3. メッセージを検索して問題ないことを確認
  4. メッセージを投稿して問題ないことを確認

go-elaasticsearchのv8からはtypedapiといういい感じに型がついたAPIクライアントもあるのですが、

  • メモリリークでたまにtraQが落ちていてそこそこ影響があるし、対応が遅くなってしまったので早めにやりたい。
  • バグったときにtypedapiにしたことによるバグなのかesを上げたことによるバグなのかわかりにくい。
    のでこれへの書き換えはまた別の機会でいいでしょうか?

ports:
- "9200:9200"
- "9300:9300"
volumes:
- ./dev/elasticsearch.yml:/usr/share/elasticsearch/config/elasticsearch.yml
Copy link
Member

Choose a reason for hiding this comment

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

この元ファイルは消さなくてもいいんですか?

Copy link
Member Author

Choose a reason for hiding this comment

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

これが議論中でそのうち使えるように戻るかもしれなかったので残すか迷ってます。
elastic/elasticsearch#85463 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

正直、ほぼ中身無いから消しちゃっていいかなと思います
開発環境なので別にその辺の体裁気にすることないかなぁと

@pikachu0310
Copy link
Contributor

go-elaasticsearchのv8からはtypedapiといういい感じに型がついたAPIクライアントもあるのですが、

  • メモリリークでたまにtraQが落ちていてそこそこ影響があるし、対応が遅くなってしまったので早めにやりたい。
  • バグったときにtypedapiにしたことによるバグなのかesを上げたことによるバグなのかわかりにくい。

のでこれへの書き換えはまた別の機会でいいでしょうか?

賛成です。そこそこ重そうですし、別でやるべきだと思います。

pikachu0310
pikachu0310 previously approved these changes Dec 4, 2023
Copy link
Member

@logica0419 logica0419 left a comment

Choose a reason for hiding this comment

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

実装は良さそうです!ReadSeekerへの変更に対応するの、ありがとうございます
compose.yamlの方に少し修正して欲しいポイントがあるので、お願いします

compose.yaml Outdated
restart: always
environment:
- discovery.type=single-node
- cluster.name=docker-cluster
# elastic search v8でhttpsがデフォルトで有効になったので、ローカルで動かすとき必要
- xpack.security.enabled=false
Copy link
Member

Choose a reason for hiding this comment

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

これはhttpsに関係ないので、コメントの上に書いてくれると嬉しいです
(認証の設定をするときに消します)

Copy link
Member Author

Choose a reason for hiding this comment

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

認証の設定をするときっていうのは、本番環境のesをいじるってことですか?

Copy link
Member

Choose a reason for hiding this comment

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

それもそうなんだけど、traQ側からユーザー名とパスワードを渡して認証する機能をつけるってことですね

compose.yaml Outdated
- cluster.name=docker-cluster
# elastic search v8でhttpsがデフォルトで有効になったので、ローカルで動かすとき必要
- xpack.security.enabled=false
- xpack.security.audit.enabled=false
Copy link
Member

Choose a reason for hiding this comment

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

audit設定はfalseにしなくても動いたんですが、これ追加した意図ってなんかありますか?
何か重要な理由が無い限り、本番環境とできるだけ同じ設定で動かしたいので消してもらえると嬉しいです

Copy link
Member Author

Choose a reason for hiding this comment

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

理由はこれと同じです。こちらも動いたので消しておきます。
#2126 (comment)

compose.yaml Outdated
Comment on lines 70 to 75
- discovery.type=single-node
- cluster.name=docker-cluster
# elastic search v8でhttpsがデフォルトで有効になったので、ローカルで動かすとき必要
- xpack.security.enabled=false
- xpack.security.audit.enabled=false
- xpack.security.transport.ssl.enabled=false
Copy link
Member

Choose a reason for hiding this comment

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

ここの環境変数のところ、他のserviceと記法を統一したいので、「{key}: {value}」の形に直してもらえるととても助かります
(keyは小文字のままで大丈夫そうでした)

compose.yaml Outdated
# elastic search v8でhttpsがデフォルトで有効になったので、ローカルで動かすとき必要
- xpack.security.enabled=false
- xpack.security.audit.enabled=false
- xpack.security.transport.ssl.enabled=false
Copy link
Member

Choose a reason for hiding this comment

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

ssl.enabledをfalseにしなくてもこっちの環境だと通ったんですけど、↑の3つの設定どこから引っ張ってきました?
元があれば教えてほしい & そっちの環境でエラー出るなら教えてほしいです

Copy link
Member Author

Choose a reason for hiding this comment

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

これを参考に設定しました。
https://blog.goo.ne.jp/dak-ikd/e/50e75332922a75be1391eace5c479141
この行を消しても動いたので消しておきます。

@logica0419
Copy link
Member

あと、typedapiへの書き換えはikura-hamuくんの提案通り別PRでやってもらえると嬉しいです

@logica0419 logica0419 linked an issue Dec 5, 2023 that may be closed by this pull request
@ikura-hamu
Copy link
Member Author

@logica0419 @pikachu0310
修正したので、再確認お願いします。

Copy link
Member

@logica0419 logica0419 left a comment

Choose a reason for hiding this comment

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

LGTM、ありがとう
大変助かりました
elastic.yml消し忘れてるけど、まぁそれはどっちでもいいか

@logica0419 logica0419 merged commit c0eef75 into master Dec 7, 2023
5 checks passed
@logica0419 logica0419 deleted the update/es-8 branch December 7, 2023 13:41
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.

ES Libraryのアップデート
4 participants