Skip to content

[ENG-10788] add nodes and preprints files reinding on account merging#11727

Open
mkovalua wants to merge 7 commits into
CenterForOpenScience:feature/pbs-26-9from
mkovalua:fix/ENG--10788
Open

[ENG-10788] add nodes and preprints files reinding on account merging#11727
mkovalua wants to merge 7 commits into
CenterForOpenScience:feature/pbs-26-9from
mkovalua:fix/ENG--10788

Conversation

@mkovalua

@mkovalua mkovalua commented May 5, 2026

Copy link
Copy Markdown
Contributor

Ticket

Purpose

User merge does not seem to be working for files

Changes

Adding nodes and preprints files reinding on account merging

Also some updates for SHARE is good to have

CenterForOpenScience/SHARE#892

Side Effects

File indexing does not work for local setup, so maybe it is needed add something else because something may be missed because of a bit blame fix

image

QE Notes

CE Notes

Documentation

@mkovalua mkovalua changed the title add nodes and preprints files reinding on account merging [ENG-10788] add nodes and preprints files reinding on account merging May 5, 2026

@antkryt antkryt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a small import fix

Comment thread osf/models/user.py
@antkryt

antkryt commented May 8, 2026

Copy link
Copy Markdown
Contributor

LGTM

@cslzchen cslzchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix looks good but I have two notes:

  • Is this feature testable by unit tests? If so, please update/add.
  • Are there any possible impact to the celery queue if too many update_share(file), could it crowd/block the celery queue?

@mkovalua

mkovalua commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

The fix looks good but I have two notes:

  • Is this feature testable by unit tests? If so, please update/add.
  • Are there any possible impact to the celery queue if too many update_share(file), could it crowd/block the celery queue?

Hi @cslzchen

  1. Have updated unittests

  2. Hard to say from my side (had problems with local files share indexing as shown in PR description), I suppose that it may be possible. If yes - maybe the solution is to create low priority tasks for files and setup several workers and it may be helpful. Maybe @mfraezz and @aaxelb have ideas. Thanks

@mkovalua mkovalua requested a review from cslzchen June 3, 2026 20:49
@aaxelb

aaxelb commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator
  • Are there any possible impact to the celery queue if too many update_share(file), could it crowd/block the celery queue?
  1. Hard to say from my side (had problems with local files share indexing as shown in PR description), I suppose that it may be possible. If yes - maybe the solution is to create low priority tasks for files and setup several workers and it may be helpful. Maybe mfraezz and aaxelb have ideas. Thanks

+1 for putting bulk tasks like these on a lower-priority queue -- currently always high priority

over in SHARE there's task routing to prioritize certain tasks called with urgent=True -- could do similar in OSF with (or replacing) the is_backfill kwarg to task__update_share (which indirectly decides urgency in SHARE already), and allowing something like update_share(obj, urgent=False) for updates not directly spurred by user interaction

@cslzchen cslzchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The approach should work 👍 .

I'd like to see if we can improve and generalize it so that this can be used by other tasks and that it doesn't conflict with other tasks.

Afterwards, make sure we have good docstring and unit tests.

Comment thread api/share/utils.py Outdated
Comment thread api/share/utils.py Outdated
Comment thread api/share/utils.py Outdated
Comment thread framework/celery_tasks/routers.py Outdated
@mkovalua mkovalua force-pushed the fix/ENG--10788 branch 2 times, most recently from 4c28e54 to 11c0f5f Compare June 8, 2026 14:19

@Ostap-Zherebetskyi Ostap-Zherebetskyi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@aaxelb

aaxelb commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

now that the task can be routed to the low-priority queue, should (perhaps as a followup ticket -- not trying to creep scope, just speaking generally) consider doing so everywhere else update_share is called with is_backfill=True or in a potentially-numerous loop

at a glance...

  • admin.users.views.UserShareReindex
  • osf.metrics.reporters.public_item_usage.PublicItemUsageReporter.followup_task
  • osf.models.user.OSFUser.merge_user
  • every def bulk_update_search(
  • osf.management.commands.recatalog_metadata.recatalog_chunk (oh wait... this already chooses a queue with task.apply_async(queue='...')... so that might have been a slightly easier path... oh well)

@cslzchen cslzchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good ⭐

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.

5 participants