Skip to content

Conversation

@jozsefdamokos
Copy link
Member

@jozsefdamokos jozsefdamokos commented Oct 8, 2025

fixes: #10822

  • moved away from processing media in parallel messages
  • all media processing is done in MediaProcessingProcessor now

@jozsefdamokos jozsefdamokos self-assigned this Oct 8, 2025
@jozsefdamokos jozsefdamokos marked this pull request as ready for review October 9, 2025 09:19
@jozsefdamokos jozsefdamokos changed the base branch from trunk to feature/migration-logging-refactor October 15, 2025 13:24
@jozsefdamokos jozsefdamokos changed the base branch from feature/migration-logging-refactor to trunk October 15, 2025 13:25
@MalteJanz
Copy link
Contributor

MalteJanz commented Oct 17, 2025

@jozsefdamokos thanks for the PR description 👍 . Some thoughts on these points from my side 😅

  • moved away from processing media in parallel messages

Alright, no shame in doing so and it makes things a lot simpler for us to reason about which is better then race condition bugs on customer systems 👍

  • changed MediaFileProcessorService to only dispatch a single ProcessMediaMessage instead of multiple messages. Only once this message is handled a new MigrationProcessMessage is dispatched. Then the processor calls the MediaFileProcessorService which then dispatches the next ProcessMediaMessage and so on.
  • moved updating the progress of the media entities from MediaProcessingProcessor to ProcessMediaHandler

Taking a step back, does that make sense to you? To me it sounds like we don't need ProcessMediaMessage and ProcessMediaHandler at all anymore and just merge the necessary logic or refactoring it into MediaProcessingProcessor (which is called during the handling of MigrationProcessMessage) or the service that it is calling MediaFileProcessorService (which then shouldn't dispatch messages anymore). That way we don't jump between different message types and our code is much easier to reason about I think 🤔

  • changed each ProcessMediaMessage to hold 20 media files to process instead of the previous 5.

Also sounds reasonable to me. Did you somehow manage to measure how long the processing of these messages take with that "batch size", ideally downloading from a remote server like a Shopware build (ShopDev) instance? We should just make sure we stay well below the 30s request timeout limit (in case someone migrates with the admin worker instead of proper cli worker).

@jozsefdamokos
Copy link
Member Author

@MalteJanz Yes, I thought about merging them, I just chose the quicker way for now to validate that this approach works. I agree, we should merge the two now if the approach of moving away from parallel processing is fine.

@MalteJanz MalteJanz self-requested a review October 27, 2025 16:02
@jozsefdamokos jozsefdamokos force-pushed the feat/media-processing-v4 branch from 8ee96a4 to 1b06549 Compare October 28, 2025 14:24
@vintagesucks vintagesucks self-requested a review October 29, 2025 07:36
@jozsefdamokos jozsefdamokos changed the base branch from trunk to feature/migration-logging-refactor October 31, 2025 13:39
@jozsefdamokos jozsefdamokos changed the base branch from feature/migration-logging-refactor to trunk October 31, 2025 13:39
Copy link
Contributor

@MalteJanz MalteJanz left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , only found one more file that can be removed now I think 🙂

@jozsefdamokos jozsefdamokos merged commit 887f7d8 into trunk Nov 10, 2025
10 checks passed
@jozsefdamokos jozsefdamokos deleted the feat/media-processing-v4 branch November 10, 2025 14:19
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.

Migration isn't finished when it says so, include media download in processing logic

6 participants