Skip to content

[Feature] Add definitionModificationDate to classes table to optimize rebuild command performance#15383

Merged
mcop1 merged 11 commits intopimcore:11.xfrom
jremmurd:classes_rebuild_data
Apr 5, 2024
Merged

[Feature] Add definitionModificationDate to classes table to optimize rebuild command performance#15383
mcop1 merged 11 commits intopimcore:11.xfrom
jremmurd:classes_rebuild_data

Conversation

@jremmurd
Copy link
Copy Markdown
Contributor

@jremmurd jremmurd commented Jun 15, 2023 •
edited
Loading

Add lastRebuildDate to classes table to optimize performance of bin/console pimcore:deployment:classes-rebuild command.

With this change the command only updates the db, if the modification date of the definition file differs from the saved timestamp in the classes table in the db.

If the modificationDate is the same in the db as in the definition file, the verbose output says, that saving the definition was skipped:

image

This change reduces the execution time from about 120s to 15-25s on AWS, if no class definition was updated. Which for us directly affects the downtown during deployments.

(only tested in Pimcore 10)

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 15, 2023

Review Checklist

  • Target branch (10.5 for bug fixes, others 11.x)
  • Bug fix: check if files are affected that were moved to a bundle - create a PR there if applicable
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@jremmurd jremmurd changed the title [Feature ] Classes rebuild data [Feature ] Add lastRebuildDate to classes and optimize rebuild command performance Jun 15, 2023
@jremmurd jremmurd changed the title [Feature ] Add lastRebuildDate to classes and optimize rebuild command performance [Feature ] Add lastRebuildDate to classes table to optimize rebuild command performance Jun 15, 2023
@jremmurd jremmurd changed the title [Feature ] Add lastRebuildDate to classes table to optimize rebuild command performance Draft: [Feature] Add lastRebuildDate to classes table to optimize rebuild command performance Jun 15, 2023
jremmurd
jremmurd commented Jun 15, 2023
View reviewed changes
models/DataObject/ClassDefinition.php
Comment on lines +469 to +381
if ($saveDefinitionFile) {
$this->setModificationDate(time());
}
Copy link
Copy Markdown
Contributor Author

@jremmurd jremmurd Jun 15, 2023

Choose a reason for hiding this comment

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

May this have any side effects? Why the modification date was previously updated, if the definition file is not saved anyways?

@jremmurd jremmurd changed the title Draft: [Feature] Add lastRebuildDate to classes table to optimize rebuild command performance [Feature] Add lastRebuildDate to classes table to optimize rebuild command performance Jun 15, 2023
andreas-gruenwald
andreas-gruenwald reviewed Jun 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@andreas-gruenwald andreas-gruenwald left a comment

Choose a reason for hiding this comment

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

See comments.
Please create a PR for Pimcore 11, as it won't be accepted for Pimcore 10 anymore.

@jremmurd jremmurd changed the base branch from 10.6 to 11.x June 16, 2023 09:34
@jremmurd jremmurd force-pushed the classes_rebuild_data branch from e3e2032 to e851d01 Compare June 16, 2023 09:37
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jun 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
4.8% 4.8% Duplication

@andreas-gruenwald
Copy link
Copy Markdown
Contributor

andreas-gruenwald commented Jun 19, 2023

@brusch fyi

@jremmurd
empty commit
86e59c7
@jremmurd
Merge remote-tracking branch 'upstream/11.x' into classes_rebuild_data
e3b61c2
@jremmurd
Merge remote-tracking branch 'upstream/11.x' into classes_rebuild_data
00de38d 
# Conflicts:
#	bundles/CoreBundle/src/Command/ClassesRebuildCommand.php
#	lib/Model/DataObject/ClassDefinition/ClassDefinitionManager.php
@jremmurd
merge from upstream
1fda2da
@blankse
Copy link
Copy Markdown
Contributor

blankse commented Nov 15, 2023

@jremmurd I'm thinking of the following scenario:

  1. I build the classes, so the lastRebuildDate is the current date.
  2. But someone else created a new class version and checked it into GIT.
  3. I pull the changes from GIT, but the classes won't rebuild, because the date is older than the rebuild date.

I think we should have at least a "force" argument to rebuild all classes.

@blankse
Copy link
Copy Markdown
Contributor

blankse commented Nov 15, 2023

@jremmurd Ah, you save the modificationDate and not the current date in the lastRebuildDate column. This should work for the scenario. But then I think the name of the column is misleading.

@jremmurd
Copy link
Copy Markdown
Contributor Author

jremmurd commented Nov 15, 2023 •
edited
Loading

@jremmurd Ah, you save the modificationDate and not the current date in the lastRebuildDate column. This should work for the scenario. But then I think the name of the column is misleading.

@blankse Good point, the column name could really be misleading. What about modelModificationDate or definitionModificationDate - wdyt?

I'll also add the force argument, to ignore the check and never skip. Thx for the feedback

@blankse
Copy link
Copy Markdown
Contributor

blankse commented Nov 15, 2023

@jremmurd I would prefer definitionModificationDate :)

@jremmurd jremmurd force-pushed the classes_rebuild_data branch from 19eea61 to 276fcbc Compare November 15, 2023 09:27
@jremmurd jremmurd changed the title [Feature] Add lastRebuildDate to classes table to optimize rebuild command performance [Feature] Add definitionModificationDate to classes table to optimize rebuild command performance Nov 15, 2023
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Nov 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
4.5% 4.5% Duplication

@ghost ghost added the Pimcore:ToDo label Jan 22, 2024
@robertSt7
Copy link
Copy Markdown
Contributor

robertSt7 commented Jan 31, 2024

@jremmurd Could you please merge 11.x in your PR to resolve the conflicts? Thanks

@brusch
Copy link
Copy Markdown
Member

brusch commented Feb 12, 2024

@jremmurd 🏓😊 Thanks!

@jremmurd
Merge remote-tracking branch 'upstream/11.x' into classes_rebuild_dat…
cd21e4d 
…a_back

# Conflicts:
#	lib/Model/DataObject/ClassDefinition/ClassDefinitionManager.php
@jremmurd
Copy link
Copy Markdown
Contributor Author

jremmurd commented Feb 27, 2024 •
edited
Loading

@jremmurd Could you please merge 11.x in your PR to resolve the conflicts? Thanks

@robertSt7 done 🔢

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Feb 27, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues

Measures
0 Security Hotspots
No data about Coverage
4.5% Duplication on New Code

See analysis details on SonarCloud

@mcop1 mcop1 added this to the 11.3.0 milestone Apr 5, 2024
@mcop1
Copy link
Copy Markdown
Contributor

mcop1 commented Apr 5, 2024

@jremmurd Could you please merge 11.x in your PR to resolve the conflicts? Thanks

@robertSt7 done 🔢

@jremmurd thank you 👍

@mcop1 mcop1 merged commit b4591f0 into pimcore:11.x Apr 5, 2024
@HageMaster3108 HageMaster3108 mentioned this pull request Jul 3, 2024
Closed
@brusch brusch mentioned this pull request Sep 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@andreas-gruenwald andreas-gruenwald andreas-gruenwald left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mcop1 mcop1

Labels

Pimcore:ToDo

Projects

None yet

Milestone

11.3.0

Development

Successfully merging this pull request may close these issues.

6 participants

@jremmurd @andreas-gruenwald @blankse @robertSt7 @brusch @mcop1