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

フラグメント識別子の再設定を webUI から行えるようにする #223

Merged
merged 26 commits into from
Nov 21, 2020

Conversation

koi-chan
Copy link
Member

refs #221

全メッセージのフラグメント識別子の再設定実行機能を実装した。

ToDo (別ブランチ)

  • ConversationMessage, Message それぞれのみのフラグメント識別子の再設定を行なう
  • 一つの SQL クエリで実行する行数の選択

@koi-chan koi-chan self-assigned this Sep 10, 2020
@koi-chan
Copy link
Member Author

新規プロセス(sidekiq)を実行する必要があるため、マージされたらマイナーバージョンアップする予定です。

Copy link
Member

@ochaochaocha3 ochaochaocha3 left a comment

Choose a reason for hiding this comment

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

大きな機能追加ありがとうございます!試してみると、裏で処理を自動で進めてくれて、現在の状態もダッシュボードに分かりやすく表示してくれたので、かなり快適でした。とても良いです!

ぜひとも取り込みたいのですが、まず以下の点を考える必要があると思いました。

  • Sidekiqの起動手順の説明:最初に試してみたとき、bundle exec sidekiq が必要と知らず、ジョブがずっと待機状態になってしまいました。ジョブを円滑に実行できるようにするため、READMEにSidekiqの起動が必要なことと起動手順を書いておくと、親切だと思います。また、実運用環境ではsystemdで起動することが想定されるため、log-archiver_unicorn.service等のように、log-archiver_sidekiq.serviceを追加すると設置しやすいです。

  • Redisのデータベースを設定できるようにする:6397/tcpでlistenしているRedisが他のアプリで使われている場合もあるので、競合しないよう接続先を設定できるようにする必要があると思いました。例えば、Redis wikiの「Using Redis#Using an initializer」に書かれている、config/initializers/sidekiq.rb を使う方法が手軽そうです。

    # config/initializers/sidekiq.rb
    
    Sidekiq.configure_server do |config|
      config.redis = { url: 'redis://redis.example.com:7372/0' }
    end
    
    Sidekiq.configure_client do |config|
      config.redis = { url: 'redis://redis.example.com:7372/0' }
    end

@koi-chan
Copy link
Member Author

koi-chan commented Sep 23, 2020

ご指摘の2点を修正しました。

README.md には sidekiq の起動について追記しました。
インストールドキュメントの整備は nginx などの物も行なわれていなかったので、ひとまず空ファイルを生成するだけにしています(別 issue として切り出します)。

ochaochaocha3
ochaochaocha3 previously approved these changes Nov 3, 2020
Copy link
Member

@ochaochaocha3 ochaochaocha3 left a comment

Choose a reason for hiding this comment

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

レビューが遅れに遅れてすみませんでした。ドキュメントについて別issueで加えられるとのことなので、これで大丈夫です。ありがとうございます。

@ochaochaocha3 ochaochaocha3 dismissed their stale review November 3, 2020 03:47

疑問点が見つかった

@ochaochaocha3
Copy link
Member

古いメッセージの表示と同時に、自動的なダイジェスト値の計算と保存を

PRのテーマを考えると、これはちょっとやりすぎな感じがしました。タイトルからは、管理画面から1クリックでフラグメント識別子を再設定するジョブを追加できるようにすることがメインだと思いましたので、まずはそれだけに集中するのが良いと思います。

更新処理を入れるとしても、モデルの #fragment_id で行うのは良くないです。「古いメッセージの表示と同時に」が想定だと思いますが、例えば rails console でちょっとモデルのフラグメント識別子を確認しようとこのメソッドを実行しただけでもジョブが追加される(ということは、Redis等の設定が済んでいなければエラーになる)可能性があります。名前からはそのメッセージの情報を得るだけに見えるので、これは仕事を抱えすぎているパターンです。

上のコメントでも書きましたが、表示ごとに自動的にダイジェスト値を計算するのが必要な場合はどのくらいあるのでしょうか? 少なくとも、creで使っている私たちはv0.8.0で追加されたことを知っているので、必要ならば今回追加された画面でまとめてダイジェスト値を計算できます。また、このことを知らない方に対しても、単純に今回追加された画面に誘導することができれば十分にならないでしょうか?

@koi-chan
Copy link
Member Author

koi-chan commented Nov 9, 2020

キーワードから発言を辿るときには、fragment_id がないとアンカーが機能しません。
そのため、キーワードが登録された範囲のメッセージを表示するときには、fragment_id が表示時点で生成されていないといけません。

ただ、このPRのテーマから外れた機能であるのは仰るとおりですので、このPRからは表示ごとに自動的にダイジェスト値を計算する機能を削ろうと思います。

@ochaochaocha3
Copy link
Member

Redisがインストールされていないときに管理画面が表示できないみたいなので、インストールするようにメッセージを出すとよさそうです。作ってみます。
https://github.com/cre-ne-jp/log-archiver/runs/1402574064

@ochaochaocha3
Copy link
Member

#229 で、Redisに接続できない場合でも管理画面が表示されるようにしました。そちらでテストも通るようになりました。

ochaochaocha3 and others added 2 commits November 16, 2020 22:24
…ests-admin_without_redis

フラグメント識別子再設定:Redisに接続できない場合でも管理画面を表示できるようにする
@ochaochaocha3
Copy link
Member

これで大丈夫だと思います。修正ありがとうございました。

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.

2 participants