Skip to content

FEATURE: Simplify marketplace structure #610

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

Merged
merged 6 commits into from
Jun 25, 2025

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented Jun 24, 2025

Merges source and dist node properties into version nodes and removes them.
This reduces nodes in CR by >60%

To be merged after #608

Sebobo added 5 commits June 23, 2025 22:44
After deployment this change first needs to be migrated with
```
./flow nodemigration:execute 20250623134952
```

Then remove the deprecated childnodes with
```
./flow structureadjustments:fix —node-type Neos.MarketPlace:Version
```
@Sebobo
Copy link
Member Author

Sebobo commented Jun 25, 2025

My Schema linter doesn't like the new migrations, have to adjust this

@Sebobo Sebobo marked this pull request as ready for review June 25, 2025 09:06
@Sebobo Sebobo moved this to Reviews in PostCon Sprint 2025 Jun 25, 2025
@bwaidelich
Copy link
Member

This also overlaps #608 which makes it quite hard to review it. I'll wait until the other one is merged. Alternatively we could stack the depending PRs

@Sebobo Sebobo changed the base branch from main to task/phpstan June 25, 2025 09:32
@Sebobo
Copy link
Member Author

Sebobo commented Jun 25, 2025

Sorry, retargeted

@bwaidelich
Copy link
Member

Thanks!
I did a first skim over the changes and before getting deeper into it I wonder: What was the issue with the lastSync property?
Or, related to that: Why do we need to introduce a cache and can't compare $packageNode->timings->lastModified?

@Sebobo
Copy link
Member Author

Sebobo commented Jun 25, 2025

Thanks! I did a first skim over the changes and before getting deeper into it I wonder: What was the issue with the lastSync property? Or, related to that: Why do we need to introduce a cache and can't compare $packageNode->timings->lastModified?

The last sync is an unnecessary write operation to the CR for each package. But we need a custom sync date to update stats (favs, downloads) of packages that haven't been updated by the maintainer in a while. And to skip package if we batch sync.

@bwaidelich
Copy link
Member

bwaidelich commented Jun 25, 2025

I understand. But with $packageNode->timings->lastModified we could determine how long a node wasn't updated. Wouldn't that be enough?

I think I got it now: We update the last sync date every time, even if no changes happened..

But I still don't fully understand why we need this check if we make sure that the cronjob is only executed once a day?

@Sebobo
Copy link
Member Author

Sebobo commented Jun 25, 2025

I understand. But with $packageNode->timings->lastModified we could determine how long a node wasn't updated. Wouldn't that be enough?

I think I got it now: We update the last sync date every time, even if no changes happened..

But I still don't fully understand why we need this check if we make sure that the cronjob is only executed once a day?

The cron job was running hourly until the Neos 9 upgrade and we can let it run hourly or every few hours again after we have all the optimisations done, as the full sync will only takes 10-15 minutes.

@bwaidelich
Copy link
Member

The cron job was running hourly until the Neos 9 upgrade and we can let it run hourly or every few hours again after we have all the optimisations done

Makes sense, but again: Can't we just skip nodes that were updated within the last days without introducing a separate storage layer by just looking at the last modification timestamp?

@Sebobo
Copy link
Member Author

Sebobo commented Jun 25, 2025

We want to quickly sync new releases so they show up in the feeds etc. but don't need to update favs and downloads all the time. So IMO this is still the way to go. The alternative is to find out what would change, but that again would take more time to compute.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

OK, I guess I don't fully understand the implications.. But I don't want to block this one and I assume that it improves things!
So let's get this merged and we can always fine tune later if it needs adjustments

@Sebobo Sebobo merged commit a0d8b18 into task/phpstan Jun 25, 2025
1 check passed
@github-project-automation github-project-automation bot moved this from Reviews to Done in PostCon Sprint 2025 Jun 25, 2025
@Sebobo Sebobo deleted the feature/simplify-marketplace-structure branch June 25, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants