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

feat(backend): fetch the first page of outbox when resolving Person #11130

Draft
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

saschanaz
Copy link
Member

@saschanaz saschanaz commented Jul 6, 2023

What

Fixes #9652

Why

ユーザーを最初照会した場合にそのタイムラインが空っぽに見えるのを防止

Additional info (optional)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added packages/backend Server side specific issue/PR packages/backend:test labels Jul 6, 2023
@github-actions github-actions bot requested review from syuilo and tamaina July 6, 2023 01:16
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.15% ⚠️

Comparison is base (814e284) 78.88% compared to head (a926e84) 78.74%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #11130      +/-   ##
===========================================
- Coverage    78.88%   78.74%   -0.15%     
===========================================
  Files          925      174     -751     
  Lines        97589    22783   -74806     
  Branches      7746      511    -7235     
===========================================
- Hits         76986    17940   -59046     
+ Misses       20603     4843   -15760     

see 751 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

collection;

// Perform activity but only the first 100 ones
await this.apInboxService.performActivity(user, firstPage, 100);
Copy link
Member

Choose a reason for hiding this comment

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

100はちょっと多い気がしたけどどうだろう

Copy link
Member Author

Choose a reason for hiding this comment

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

50? 20? 適当に100にしましたが多すぎるかも

Copy link
Member

Choose a reason for hiding this comment

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

100 でいいんじゃね

Copy link
Contributor

@tamaina tamaina Jul 7, 2023

Choose a reason for hiding this comment

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

30ぐらいでいいんじゃない?

Copy link
Contributor

Choose a reason for hiding this comment

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

15でもいいかも

Copy link
Contributor

Choose a reason for hiding this comment

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

15にしようぜ

Copy link
Member Author

Choose a reason for hiding this comment

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

Misskeyのoutbox limitが20ですのでせめて20はしたいです

@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

なんかsignedGetでCPU使用率がめちゃくちゃやばい

@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

const act = await resolver.resolve(item);

resolver.resolveをアイテム分だけ呼び出してるけどどうにか1回のリクエストで取得できないの

@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

1回ならまあなんとかなる

@tamaina tamaina marked this pull request as draft July 7, 2023 06:38
@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

これ、#11161 は変わらないけど

{ message: "Duplicated note", code: "duplicated", id: "d301f236-d4c1-4f1c-8007-b8985e9a6d93" }

にならない?

@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

outbox読むのにsignatureって必要なのかしら?→フォロワー向け投稿周りで必要なのか

@saschanaz
Copy link
Member Author

どういう条件でそうなるのでしょうか。同じユーザーは再び照会しても二つ目はローカルからロードするはずですので、同じactivityは再びperformされないものなのではないかと

@saschanaz
Copy link
Member Author

saschanaz commented Jul 7, 2023

outbox読むのにsignatureって必要なのかしら?→フォロワー向け投稿周りで必要なのか

あとsignature必修なサーバーとか(フォロワーは最初照会の時は関係ないはず)

@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

エラーの話は手違いの可能性が高い、すみませんでした

@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

とにかくsignedGetでCPU使用率がどうしようもなくなるのでsignedGetを1回きりにする必要がある

@tamaina
Copy link
Contributor

tamaina commented Jul 7, 2023

エラーはこのPRのせいだった

どういう条件でそうなるのでしょうか。

ap/showで、未知のユーザーの未知のノートを取得するとこうなる。

image

ap/showで色々呼んでいる途中の

const actor = await this.apPersonService.resolvePerson(getOneApId(note.attributedTo!), resolver) as RemoteUser;

が問題で、これの後に作業中のノートをthis.noteCreateService.createで改めて挿入してしまうためコンフリクトが発生する

@saschanaz
Copy link
Member Author

ap/showで色々呼んでいる途中の

const actor = await this.apPersonService.resolvePerson(getOneApId(note.attributedTo!), resolver) as RemoteUser;

が問題で、これの後に作業中のノートをthis.noteCreateService.createで改めて挿入してしまうためコンフリクトが発生する

この問題、featuredノートをfetchした場合はどうなるんでしょう

  1. Note Aをfetch
  2. そのactorをfetch
  3. featuredノートをfetch (note Aが含まれている)
  4. Note Aをinsert <- ?

@saschanaz
Copy link
Member Author

saschanaz commented Jul 8, 2023

とにかくsignedGetでCPU使用率がどうしようもなくなるのでsignedGetを1回きりにする必要がある

updateOutboxFirstPage自体は最大2回signedGetしますので、その場合はそれに含まれるノートに新しいものが沢山入ってるのかも

@saschanaz saschanaz marked this pull request as ready for review July 9, 2023 21:34
@github-actions github-actions bot requested a review from syuilo July 9, 2023 21:34
@tamaina
Copy link
Contributor

tamaina commented Jul 14, 2023

conflicted

@saschanaz saschanaz marked this pull request as draft July 15, 2023 02:14
@saschanaz
Copy link
Member Author

(#11130 (comment) の対応をしてなかったので)

@tamaina
Copy link
Contributor

tamaina commented Jul 15, 2023

うーーん、リプライとかを永久に引っ張ってきておりデータベースが肥大化してきている

performActivityでフェッチする数をdefault.ymlで調整できるようにする必要がある(0で無効化)

@saschanaz
Copy link
Member Author

saschanaz commented Jul 15, 2023

resolvePersonじゃなくtimeline APIとかでtimelineが空いてたらfetchするようにしたらいいかも

元々リモートでも空いてるアカウントとなんとか区別しないと相手サーバーにfetchし続けるようになるのでそれは困難

@tamaina
Copy link
Contributor

tamaina commented Jul 15, 2023

とりあえず設定できるようにした

@saschanaz
Copy link
Member Author

placeholderノート作りたい(実際必要な場合にだけfetchするように)

@tamaina
Copy link
Contributor

tamaina commented Jul 15, 2023

placeholderにしておいてpack(?)が呼ばれたらresolveするようにということかしら

@saschanaz
Copy link
Member Author

そうやってfetch回数をへらしたり、dropboxのplaceholderファイルのように古くて使わないリモートのノートをplaceholderに変えて容量を確報するとか、使い道がいろいろありそうです

@misskey-dev misskey-dev deleted a comment from wederuten6 Jul 17, 2023
@tamaina
Copy link
Contributor

tamaina commented Aug 24, 2023

conflicted

(p1.a9z.devへマージしているため解消する必要がある)

@tamaina
Copy link
Contributor

tamaina commented Aug 24, 2023

解消

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend:test packages/backend Server side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants