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

[6.x.x] Fix errors on update operations #5296

Merged
merged 8 commits into from
Oct 24, 2024
Merged

[6.x.x] Fix errors on update operations #5296

merged 8 commits into from
Oct 24, 2024

Conversation

line-o
Copy link
Member

@line-o line-o commented May 2, 2024 •
edited
Loading

Backport of #5276
refs #5273

@adamretter adamretter changed the title Backport #5276 [6.x.x] Fix errors on update operations May 2, 2024
@adamretter adamretter self-requested a review May 2, 2024 11:09
@adamretter
Copy link
Contributor

adamretter commented May 2, 2024 •
edited
Loading

@line-o The outstanding issues need to be addressed first please - #5276 (comment)

@line-o
Copy link
Member Author

line-o commented May 2, 2024

The test is different from the one in the original PR targeted at develop.

@adamretter
Copy link
Contributor

The test is different from the one in the original PR targeted at develop.

Okay, but develop needs to be fixed first... Then those same commits can either be in this PR, or in a separate PR to 6.x.x.

@line-o
Copy link
Member Author

line-o commented May 2, 2024

Please see the actual test that is part of the PR.

@line-o line-o requested a review from a team May 2, 2024 19:40
@wolfgangmm wolfgangmm mentioned this pull request May 24, 2024
Open
@line-o line-o mentioned this pull request Jul 3, 2024
Closed
adamretter
adamretter suggested changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

This PR (#5296) is a backport to 6.x.x of #5276 (which targeted develop). My concern is that #5276 was rushed and merged into develop whilst reviews were still identifying issues. This has led to #5276 actually introducing problems into develop by leaking resources in its tests. I asked for this to be fixed in April 2024 in both PRs. This has not yet been done. As this PR (#5296) is a backport, it has the same leak issues as #5276 and so cannot yet be merged.

To resolve this, the author (as I have requested), or anyone, needs to:

  1. send a new PR to develop to fix the resource leak problems introduced by Fix errors on update operations #5276
  2. get that new PR through review and then merged.
  3. append the bugfix commit(s) from that new PR into this PR ([6.x.x] Fix errors on update operations #5296).
  4. we can then re-review this PR ([6.x.x] Fix errors on update operations #5296) and hopefully get it merged.

@line-o line-o force-pushed the backport/5276 branch 2 times, most recently from 88f83a9 to a71f09b Compare October 14, 2024 15:14
@line-o line-o mentioned this pull request Oct 18, 2024
Closed
@line-o
Copy link
Member Author

line-o commented Oct 18, 2024

This PR has now the needed approvals of two members of the core-team.

@line-o
Copy link
Member Author

line-o commented Oct 18, 2024

This can be considered a fix - the tests do differ between version 6 and 7 because of the breaking changes in XML-DB API
The aforementioned issue - resource leak in the test class - is addressed.

@adamretter
Copy link
Contributor

adamretter commented Oct 20, 2024 •
edited
Loading

  1. send a new PR to develop to fix the resource leak problems introduced by Fix errors on update operations #5276
  1. get that new PR through review and then merged.
  1. append the bugfix commit(s) from that new PR into this PR ([6.x.x] Fix errors on update operations #5296).

we can then re-review this PR (#5296) and hopefully get it merged

  • Done

line-o and others added 8 commits October 24, 2024 15:09
@line-o @adamretter
[test] add test that triggers defragmentation after update
1c087d5 
- add constructor to ExistXmldbEmbeddedServer that allows passing in configuration properties
- create new test that updates a document with fragmentation limit set to -1
@line-o @adamretter
[bugfix] NPE in NativeBroker.defragXMLResource
9ca293b 
A regression introduced in c553667 causes a NullPointerException to be thrown whenever an XML-resource
needs to be defragmented.
This will be triggered on frequent writes to any XML-resource and will effectively remove the file
from the database and from any indexes.
@line-o @adamretter
[bugfix] PermissionDeniedExcpetion in defragXMLResource
1e83f0b
@adamretter
[bugfix] Correct class name
f88c402
@adamretter
[bugfix] Fix class members visibility
a8fe278
@adamretter
[bugfix] Make sure not to leak XML Resource
bf4675f
@adamretter
[bugfix] Remove Collection is not needed as the test uses Temporary S…
9ef85e6 
…torage
@adamretter
[refactor] Use Embedded API directly. We should not use XML:DB API in…
6955588 
… tests unless we are intending to explicitly test the XML:DB API implementation
@adamretter adamretter self-requested a review October 24, 2024 13:11
adamretter
adamretter approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

This now with my additions looks correct.

@adamretter adamretter merged commit eaf098f into eXist-db:develop-6.x.x Oct 24, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamretter adamretter adamretter approved these changes

@dizzzz dizzzz dizzzz approved these changes

@reinhapa reinhapa reinhapa approved these changes

@wolfgangmm wolfgangmm Awaiting requested review from wolfgangmm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@line-o @adamretter @dizzzz @reinhapa