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.

Sign up for GitHub

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

Jump to bottom

XWIKI-22613: Updating the history can take a very long time when there are a lot of objects #3788

Merged
merged 9 commits into from
Jan 31, 2025
Merged

XWIKI-22613: Updating the history can take a very long time when there are a lot of objects #3788

merged 9 commits into from
Jan 31, 2025

Conversation

tmortagne
Copy link
Member

@tmortagne tmortagne commented Jan 3, 2025 •
edited
Loading

Jira URL

https://jira.xwiki.org/browse/XWIKI-22613: Updating the history can take a very long time when there are a lot of objects
https://jira.xwiki.org/browse/XWIKI-22752: Allows indicating in the Hibernate hbm configuration files that an entity should be compressed
https://jira.xwiki.org/browse/XWIKI-22747: Introduce the concept of XWiki Hibernate adapter

Changes

Description

The main goal of this change is to stop storing diff in the document history table.

The "stop storing diff" part is easy, but it implies trying to optimize the RCS table to try to reduce its size as much as possible by compressing it. Since the way to do this is not handled by Hibernate and quite different depending on the database engine, I also used this opportunity to improve how we deal with the difference between database engines with a new HibernateAdapater concept.

Clarifications

See the following for more details and discussion around each subject:

Executed Tests

New tests:

  • XWikiDocumentArchiveTest now contains several unit tests checking both the new (always full patch) and old behavior (full every 5 versions)

The main goal of this pull request is improving document save performances, which is not something which is easy to accurately test in an integration test.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches: none (too dangerous, at worst everything this change is automating can be done by hand in older versions)

@tmortagne tmortagne marked this pull request as draft January 3, 2025 15:29
@tmortagne tmortagne self-assigned this Jan 8, 2025
@tmortagne tmortagne force-pushed the feature-deploy-nodiffrcs branch 2 times, most recently from e6629fe to 4e62720 Compare January 10, 2025 10:40
@tmortagne tmortagne marked this pull request as ready for review January 10, 2025 10:42
@tmortagne tmortagne force-pushed the feature-deploy-nodiffrcs branch 3 times, most recently from f7ebb8d to aa16eef Compare January 13, 2025 15:17
@tmortagne tmortagne force-pushed the feature-deploy-nodiffrcs branch 5 times, most recently from 844f05d to 1940f27 Compare January 29, 2025 09:04
@tmortagne
XWIKI-22613: Updating the history can take a very long time when ther…
07ce23a 
…e are a lot of objects

XWIKI-22752: Allows indicating in the Hibernate hbm configuration files that an entity should be compressed
XWIKI-22747: Introduce the concept of XWiki Hibernate adapter
@tmortagne tmortagne force-pushed the feature-deploy-nodiffrcs branch from 1940f27 to 07ce23a Compare January 29, 2025 14:36
surli
surli reviewed Jan 29, 2025
View reviewed changes
...atform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/doc/XWikiDocumentArchive.java
int nodesPerFull = context.getWiki() == null ? 5
: Integer.parseInt(context.getWiki().getConfig().getProperty("xwiki.store.rcs.nodesPerFull", "5"));
int nodesPerFull = context.getWiki() == null ? 1
: Integer.parseInt(context.getWiki().getConfig().getProperty("xwiki.store.rcs.nodesPerFull", "1"));
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if it's not the right moment to also put and document that config option in xwiki.cfg? AFAIR it's not at all documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but my feeling is more that we want to bury that option as deep as possible and eventually completely remove that concept.

Copy link
Member

Choose a reason for hiding this comment

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

well, I understand but on the other hand you'll indicate in the RN that this breaking change might be reverted with the same property... maybe it's cleaner if we document all of it in xwiki.cfg indicating values, that it's possible to change it but it's not advised to do it and it's even considered as a deprecated property?

eventually completely remove that concept.

I guess if we start documenting it as a way to revert the old behaviour it makes complicated to also open a proposal for removing it, at least not before 19.x to have a full LTS cycle

Copy link
Member Author

@tmortagne tmortagne Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

that this breaking change

I'm not sure how this is breaking, it just stores a full version every 1 version instead of every 5 versions by default. This does not change the format in any way (i.e. you could still read this database with a previous version of XWiki just fine).

Copy link
Member

Choose a reason for hiding this comment

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

indeed not a breaking change but I assumed when reading your forum proposal that you were going to document the possibility to revert this behaviour, and so this property.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to see it documented

Copy link
Member Author

@tmortagne tmortagne Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do plan to indicate a way to go back to the previous behavior in the release note retro compatibility section, but I would really prefer to keep this option hidden in general, since the goal is really not for users to start using something we would like to get rid of one day.

@tmortagne
XWIKI-22747: Introduce the concept of XWiki Hibernate adapter
ed17136 
* fix various typos
* missing `@Unstable`
* try to make the design more clear though some renames, examples and javadoc
@tmortagne
XWIKI-22747: Introduce the concept of XWiki Hibernate adapter
7f02893 
* more javadoc
@tmortagne tmortagne requested a review from surli January 30, 2025 08:03
@tmortagne
XWIKI-22747: Introduce the concept of XWiki Hibernate adapter
35bd3db 
* fix typo
* add missing `@Unstable`
@tmortagne
XWIKI-22747: Introduce the concept of XWiki Hibernate adapter
d887653 
* add unit tests
@tmortagne
XWIKI-22747: Introduce the concept of XWiki Hibernate adapter
5892a5b 
* introduce a configuration to control if compression is enabled or not and disable it for Oracle by default (because the feature is not enabled by default)
@tmortagne tmortagne merged commit 173c679 into master Jan 31, 2025
@surli surli deleted the feature-deploy-nodiffrcs branch January 31, 2025 14:44
surli
surli reviewed Feb 3, 2025
View reviewed changes
...rm-tools/xwiki-platform-tool-configuration-resources/src/main/resources/hibernate.cfg.xml.vm
@@ -304,6 +304,10 @@ jdbc:hsqldb:file:${environment.permanentDirectory}/database/xwiki_db;shutdown=tr
<property name="hibernate.connection.useUnicode">true</property>
<property name="hibernate.connection.characterEncoding">utf8</property>
<!-- To enable if basic compression feature is enabled in Oracle
<property name="xwiki.compressionAllowed">true</property>
-->
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm this comment is not good, it's already inside another comment so it's breaking the file. Going to fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, did not notice. Problem is that it's not that easy to fix, ideally when you uncomment the Oracle section, this part should be commented.

All I can think of right now is either:

Copy link
Member

Choose a reason for hiding this comment

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

I actually chose to always put the property but with a false value, and add the info in the notes, see: 72e10a1. Should work, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I commented your change, IMO this is a bad idea.

Sereza7 pushed a commit to Sereza7/xwiki-platform that referenced this pull request Feb 4, 2025
@tmortagne @Sereza7
XWIKI-22613: Updating the history can take a very long time when ther…
4e3bcb9 
…e are a lot of objects (xwiki#3788)

XWIKI-22752: Allows indicating in the Hibernate hbm configuration files that an entity should be compressed
XWIKI-22747: Introduce the concept of XWiki Hibernate adapter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surli surli surli left review comments

@manuelleduc manuelleduc manuelleduc left review comments

Assignees

@tmortagne tmortagne

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tmortagne @manuelleduc @surli