OAK-10921 : fixed race condition where fullGC database variables gets… #1562

rishabhdaim · 2024-06-27T13:43:50Z

… overridden if they are reset by external components

rishabhdaim · 2024-06-27T14:16:06Z

...ment/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java

@@ -94,7 +94,6 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
-import static org.junit.Assume.assumeThat;


removed it, cause it was an unused import.

...ment/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java

stefan-egli · 2024-06-27T15:53:14Z

...ment/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java

+            public Iterable<NodeDocument> getModifiedDocs(long fromModified, long toModified, int limit, @NotNull String fromId,
+                                                          final @NotNull Set<String> includePaths, final @NotNull Set<String> excludePaths) {
+                // reset fullGC variables externally while GC is running
+                store1.getDocumentStore().remove(SETTINGS, SETTINGS_COLLECTION_ID);


It looks like we have two variations of reset :

[reset|https://github.com/apache/jackrabbit-oak/blob/65e2ac78fb618915d6783033c61cd724fac54b36/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java#L382-L384]

[reset fullGC|https://github.com/apache/jackrabbit-oak/blob/65e2ac78fb618915d6783033c61cd724fac54b36/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollector.java#L386-L391]

IIUC then this just tests one of the two. Might be good to actually test both (whether in same or different test method)?

I see the new resetFullGCFromOakRunWhileRunning is virtually identical to resetGCFromOakRunWhileRunning except for reset() vs resetFullGC() - which was the idea of course - but I would suggest to consider refactoring these two to avoid (test) code duplication - drafted this (without testing) in #1572 to illustrate

Joscorbe

Looks good to me, assuming there will be another test case as @stefan-egli already proposed.

rishabhdaim · 2024-07-04T10:00:09Z

...ment/src/test/java/org/apache/jackrabbit/oak/plugins/document/VersionGarbageCollectorIT.java

@@ -191,7 +190,7 @@ public GCCounts(FullGCMode mode, int deletedDocGCCount, int deletedPropsCount,
    public DocumentStoreFixture fixture;

    @Parameter(1)
-    public FullGCMode fullGcMode;
+    FullGCMode fullGcMode;


reduced the scope for FullGCMode to avoid leaking this object.

stefan-egli · 2024-07-23T13:46:56Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java

-                    SETTINGS_COLLECTION_FULL_GC_TIMESTAMP_PROP, stats.oldestModifiedDocTimeStamp));
-            setStringSetting(SETTINGS_COLLECTION_FULL_GC_DOCUMENT_ID_PROP, stats.oldestModifiedDocId);
+            setVGCSetting(SETTINGS_COLLECTION_OLDEST_TIMESTAMP_PROP, scope.toMs);
+            updateVGCSetting(new HashMap<>() {{


subclassing HashMap seems heavy, what about using Map.of() as was done in some other cases?

stefan-egli · 2024-07-23T13:47:20Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java

-    private void setLongSetting(String propName, long val) {
-        setLongSetting(of(propName, val));
+    private void setVGCSetting(final String propName, final Object val) {
+        setVGCSetting(new HashMap<>() {{


(same as previous)
subclassing HashMap seems heavy, what about using Map.of() as was done in some other cases?

Map.of() doesn't allow null values and during some test cases, some values happen to be null and cause the test failures.

Got it. Alternatively, you could of course create the hasmap first, then add the key value. Would be slightly more code but avoid the subclass.

oh, didn't know the performance aspect of this. Will make the change.

stefan-egli · 2024-07-23T14:05:42Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/VersionGCRecommendations.java

+        });
+    }
+
+    private void updateVGCSetting(final Map<String, Object> propValMap) {


Noticed a couple issues with this:

it is not clear in which situation updateVGCSetting is used. It seems to be targeted for evaluate. But that method has a complex (if-) structure and it's not easy to figure out in which cases updateVGCSetting is called. Maybe this is more of a readability comment. Perhaps adding a comment would be a good improvement thouhg.

VersionGCInitTest.lazyInitialize currently fails since updateVGCSetting is invokved but with a condition that fullGCTimeStamp and fullGCId are set. Except it seems those are not initialized. So the initialization of these must somehow be better covered, I'd suggest first in a separate test case perhaps, and then in the code.

… overridden if they are reset by external components

stefan-egli · 2024-08-12T13:35:21Z

Started rereviewing and must admit that it's perhaps not immediately obvious why we use update in one case and set in the other.

Then I noticed that the jira as well doesn't much go into details as to what exactly the race condition is. The PR does :

(fixed) race condition where fullGC database variables gets overridden if they are reset by external components

What about adding a comment (or the actual description) to the jira that explains what goes wrong when "the race condition" happens? I guess it is : when you do a reset using oak-run while an oak instance is executing a regular fullGC (not dry run), then the reset would be ignored? And at the same time perhaps why dry run is different from wet run in this regard? Just might make code reading a bit easier.

PS: also noticed that due to the force-push I can't review the diff between my last review and this one - so can't follow what has changed since.

stefan-egli · 2024-08-12T15:01:41Z

Related to that, currently resetGCFromOakRunWhileRunning and resetFullGCFromOakRunWhileRunning test doing a reset (in the 2 variants) against a wet fullGC run. Would having a a combination with a dry fullGC run (and the 2 reset variants) be good to have as well?

Perhaps in combination with explaining why dry and wet are different when they race with the different resets, could make the code a bit easier to understand.

rishabhdaim · 2024-08-13T05:33:23Z

The flow is like this:

Full GC is running normally.
We reset the variables externally let's say via oak-run.
Oak run removes the variables fullGCTimeStamp & fullGCId from the database.
Once the normal cycle of fullGC finishes, it will again add/update those variables, which would nullify the effect of reset.

To fix this, we need to replace the updateAndCreate API (to update full GC variables after each full GC cycle) with findAndModify, this will ensure that in case they are already removed, they won't get added again.

Once the new cycle of GC starts, it will add them again with values of the oldest modified document & its ID.

Note: This race condition won't come in case of dry run because dry run is not supported in normal flow. We can dry run with only the oak run command.

stefan-egli

+1, thx for the explanation in the jira, helpful indeed.

rishabhdaim requested review from Joscorbe and stefan-egli June 27, 2024 13:43

rishabhdaim commented Jun 27, 2024

View reviewed changes

stefan-egli reviewed Jun 27, 2024

View reviewed changes

Joscorbe approved these changes Jun 28, 2024

View reviewed changes

rishabhdaim force-pushed the OAK-10921 branch from 39cf615 to 7c88a16 Compare July 4, 2024 09:58

rishabhdaim commented Jul 4, 2024

View reviewed changes

rishabhdaim requested a review from stefan-egli July 4, 2024 11:09

rishabhdaim force-pushed the OAK-10921 branch 2 times, most recently from 0c80fd9 to 9f6680c Compare July 4, 2024 17:16

stefan-egli reviewed Jul 23, 2024

View reviewed changes

Rishabh Kumar added 6 commits August 5, 2024 10:33

OAK-10921 : fixed race condition where fullGC database variables gets…

813e1c1

… overridden if they are reset by external components

OAK-10921 : fixed unit cases

de4280d

OAK-10921 : added unit case for resetFullGC & resetGC both

d75f63d

OAK-10921 : undo the scope change for FullGcMode

d34ba07

OAK-10921 : extracted out common code

4479723

OAK-10921 : added javadocs for update settings method

4e2009e

rishabhdaim force-pushed the OAK-10921 branch from 9f6680c to 4e2009e Compare August 5, 2024 08:34

OAK-10921 : removed hashmap subclass with normal hashmap creation

7f560c6

rishabhdaim requested a review from stefan-egli August 6, 2024 09:21

stefan-egli approved these changes Aug 13, 2024

View reviewed changes

rishabhdaim merged commit dcb39ef into trunk Aug 13, 2024
1 of 2 checks passed

rishabhdaim deleted the OAK-10921 branch August 13, 2024 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAK-10921 : fixed race condition where fullGC database variables gets… #1562

OAK-10921 : fixed race condition where fullGC database variables gets… #1562

rishabhdaim commented Jun 27, 2024

rishabhdaim Jun 27, 2024 •

edited

Loading

stefan-egli Jun 27, 2024

stefan-egli Jul 4, 2024

Joscorbe left a comment

rishabhdaim Jul 4, 2024

stefan-egli Jul 23, 2024

stefan-egli Jul 23, 2024

rishabhdaim Jul 30, 2024

stefan-egli Aug 1, 2024

rishabhdaim Aug 5, 2024

stefan-egli Jul 23, 2024

stefan-egli commented Aug 12, 2024 •

edited

Loading

stefan-egli commented Aug 12, 2024

rishabhdaim commented Aug 13, 2024 •

edited

Loading

stefan-egli left a comment

OAK-10921 : fixed race condition where fullGC database variables gets… #1562

OAK-10921 : fixed race condition where fullGC database variables gets… #1562

Conversation

rishabhdaim commented Jun 27, 2024

rishabhdaim Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joscorbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-egli commented Aug 12, 2024 • edited Loading

stefan-egli commented Aug 12, 2024

rishabhdaim commented Aug 13, 2024 • edited Loading

stefan-egli left a comment

Choose a reason for hiding this comment

rishabhdaim Jun 27, 2024 •

edited

Loading

stefan-egli commented Aug 12, 2024 •

edited

Loading

rishabhdaim commented Aug 13, 2024 •

edited

Loading