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

OAK-10739 : Provide support for FullGC in Mongo DocumentStore #1454

Merged
merged 164 commits into from
May 28, 2024
Merged

OAK-10739 : Provide support for FullGC in Mongo DocumentStore #1454

merged 164 commits into from
May 28, 2024

Conversation

rishabhdaim
Copy link
Contributor

No description provided.

stefan-egli and others added 30 commits March 15, 2024 10:45
@stefan-egli
OAK-10199 : initial sketch of detail gc skeleton
661fcd0
@stefan-egli
OAK-10199 : disable the detailGc in tearDown to avoid side-effects
d27a96b
OAK-10199 : provided support for feature toggle & osgi config for det…
ffeb6ff 
…ailed gc
OAK-10199 : override getModifiedDocs() for RDB and added unit cases f…
5c07e69 
…or deletedProps
OAK-10199 : used bulk findAndModify api to perform garbage cleanup
43fd4aa
OAK-10199 : ignore documents which doesn't have _modified field in mo…
345064b 
…ngo while fetching modifiedDocs
OAK-10199 : fixed code smells as suggested by Sonar
9d308fb
OAK-10199 : updated logic to fetch nodes by sorting them on the basis…
e7c4f9c 
… of _modified & _id
OAK-10199 : minor refactoring
43a1bc8
OAK-10199 : added test cases to fetch 5000+ modified docs in loop and…
3335ca2 
… verify them
OAK-10199 : added check to include oldestId when running detailedGc v…
317a9af 
…ery first time
OAK-10199 : fixed the test case with different revision for DetailedGC
f857fa0
OAK-10199 : fixed the VGC IT test cases failures for RDB
e1e8f51
OAK-10199 : fixed the VG INIT test
114b1c0
OAK-10199 : fixed logic to include previously garbage collected docum…
1d448c5 
…ents if updated recently
OAK-10199 : added test case to continue with GC even if there is crash
17361ae
OAK-10199 : handled escaped properties while deleting them
294fc59
OAK-10199 : added unit cases to handle concurrent prop update and esc…
0af60d8 
…aped properties update
OAK-10199 : added logic to skip non garbage documents
e90a3d4
OAK-10199 : fixed query to avoid skipping documents with greater _mod…
d6c7e79 
…ified timestamp
OAK-10199 : refactored RDBVersionGCSupport code
21cca4b
OAK-10199 : added review comment to make minor adjustment for naming …
835441d 
…conventions and fixes
OAK-10199 : added bundled properties while calculating the properties…
92d055b 
… retain set as well
OAK-10199 : added unit cases for bundled properties getting garbaged …
f740a66 
…collected as well
OAK-10199 : renamed method name to avoid confusion with other variabl…
ec91fea 
…e of same name
OAK-10199 : incorporated review comments to remove <p/> XHTML tags
39c890f
OAK-10199 : made detailedGC non-configurable on RDBDocumentStore
647e8f3
OAK-10199 : removed DetailedGC code changes for RDBDocumentStore
d409897
@rishabhdaim
Merge pull request #1079 from apache/OAK-10378
7a66fc1 
OAK-10378 : added metrics to collect detailedGC cycle & deleted properties data
@stefan-egli
OAK-8646 : Clean up changes from orphaned branch commits, as part of …
a7ae4a6 
…detailed gc
stefan-egli and others added 11 commits May 14, 2024 18:17
@stefan-egli
Merge pull request #1459 from apache/OAK-10739-1454-1
90be2f0 
OAK-10739 : revert unrelated or unnecessary changes
OAK-10792 : rename DetailedGC to FullGC
5d17124
@rishabhdaim
Merge pull request #1460 from apache/OAK-10792
918bd17 
OAK-10792 : rename DetailedGC to FullGC
OAK-10739 : reverted static import of System.currentTimeMillis()
7c78c98
OAK-10739 : replaced isEmpty check with isNull
946f3eb
OAK-10739 : removed static imports from VersionGCRecommendations
b22c312
OAK-10739 : removed unintendent changes from VersionGarbageCollector
9f0c22b
OAK-10739 : removed unintendent changes from VersionGCSupport & Versi…
c505b3d 
…onGC Test classes
@rishabhdaim
Merge pull request #1462 from apache/OAK-10739
3fcf9d8 
OAK-10739 : reverted static import of System.currentTimeMillis()
@stefan-egli
OAK-10739 : revert unrelated or unnecessary changes
4cb436a
@stefan-egli
Merge pull request #1467 from apache/OAK-10739-1454-2
5bb9446 
OAK-10739 : revert unrelated or unnecessary changes
@stefan-egli
Copy link
Contributor

I believe most of the cosmetic changes have been reverted and the PR is ready for review again.

@mbaedke
Copy link
Contributor

mbaedke commented May 21, 2024

@stefan-egli, is it really a good idea to do a merge commit? These are 160 commits and I would assume that many of them are intermediate changes or even reverted later. If we do no delete the branch, nothing gets lost, even with a squeeze commit.

@stefan-egli
Copy link
Contributor

@mbaedke yea I had second thought there as well. I'm fine with squeeze as well. @rishabhdaim , @Joscorbe ?

@rishabhdaim
Copy link
Contributor Author

@mbaedke yea I had second thought there as well. I'm fine with squeeze as well. @rishabhdaim , @Joscorbe ?

I am fine with squashing too.

@stefan-egli
Copy link
Contributor

previous, direct commit was just a fix for a previous revert - aka a detail

Rishabh Kumar and others added 2 commits May 23, 2024 15:02
reschke
reschke reviewed May 23, 2024
View reviewed changes
...rc/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentNodeStoreBuilder.java
@@ -26,6 +26,8 @@
import org.apache.jackrabbit.oak.plugins.document.DocumentStore;
import org.apache.jackrabbit.oak.plugins.document.MissingLastRevSeeker;
import org.apache.jackrabbit.oak.plugins.document.VersionGCSupport;
import org.apache.jackrabbit.oak.spi.toggle.Feature;
import org.jetbrains.annotations.Nullable;

Copy link
Contributor

Choose a reason for hiding this comment

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

just checking: why do we need the "Feature" import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need it for overridding public RDBDocumentNodeStoreBuilder setDocStoreFullGCFeature(@Nullable Feature docStoreFullGC) method.

reschke
reschke approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

The PR looks much better now. I'm +1 on merging now to avoid having to deal with bit rot again.

@Joscorbe
Copy link
Member

I'm fine with squashing too @stefan-egli @rishabhdaim.

@stefan-egli
Copy link
Contributor

@Joscorbe I see NodeDocumentRevisionCleanerTest.testRecentRevisionsArePreserved failing - would it be much effort to fix that before the merge?

@Joscorbe Joscorbe merged commit e5cffd3 into trunk May 28, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbaedke mbaedke mbaedke left review comments

@reschke reschke reschke approved these changes

@Joscorbe Joscorbe Joscorbe approved these changes

@stefan-egli stefan-egli stefan-egli approved these changes

@mreutegg mreutegg Awaiting requested review from mreutegg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@rishabhdaim @stefan-egli @reschke @mbaedke @Joscorbe @t-rana