jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vikas Saurabh (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (OAK-3976) journal should support large(r) entries
Date Mon, 12 Dec 2016 21:23:58 GMT

    [ https://issues.apache.org/jira/browse/OAK-3976?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15743140#comment-15743140
] 

Vikas Saurabh edited comment on OAK-3976 at 12/12/16 9:23 PM:
--------------------------------------------------------------

Attaching patch - [^OAK-3976.patch]. Notable difference from earlier version:
* {{JournalEntry}} also tracks if some branch commits have been added - this is required to
avoid the case when a commit pushes the entry and before any other commit background write
tries to push the journal (thus using the same revision for the entry and logging error that
it couldn't push the entry)
** This fails a few tests which assume that DocumentNodeStore init pushes an empty journal
entry ({{JournalTest#lastRevRecoveryJournalTest, JournalTest#lastRevRecoveryJournalTestWithConcurrency,
JournalGCTest#getTailRevision}}). While we could avoid let these tests pass by remembering
last journal-revision pushes and skip same revision to be pushed instead of tracking if we
have any changes to be pushed or not. But, I prefer this way (not push if not required) -
fixed test diff at \[0]. [~mreutegg] thoughts?
* Path counter is incremented if a new {{TreeNode}} is pushed - probably we should rename
it to something like node-count
* Lock around journal push removed
* A bit of refactor of where "numChanges" crosses threshold (now in {{done()}} method)

\[0]: 
{noformat}
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalGCTest.java
b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalGCTest.java
index 1c05460..aef2c037 100644
--- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalGCTest.java
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalGCTest.java
@@ -105,7 +105,7 @@ public class JournalGCTest {
         // must collect all journal entries. the first created when
         // DocumentNodeStore was initialized and the second created
         // by the background update
-        assertEquals(2, jgc.gc(1, TimeUnit.HOURS));
+        assertEquals(1, jgc.gc(1, TimeUnit.HOURS));

         // current time, but without the increment done by getTime()
         now = c.getTime() - 1;
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java
b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java
index feb9ee1..2420065 100644
--- a/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/JournalTest.java
@@ -337,11 +337,8 @@ public class JournalTest extends AbstractJournalTest {
         DocumentNodeStore ds2 = mk2.getNodeStore();
         final int c2Id = ds2.getClusterId();

-        // should have 1 each with just the root changed
-        assertJournalEntries(ds1, "{}");
-        assertJournalEntries(ds2, "{}");
-        assertEquals(1, countJournalEntries(ds1, 10));
-        assertEquals(1, countJournalEntries(ds2, 10));
+        assertEquals(0, countJournalEntries(ds1, 10));
+        assertEquals(0, countJournalEntries(ds2, 10));

         //1. Create base structure /x/y
         NodeBuilder b1 = ds1.getRoot().builder();
@@ -383,9 +380,9 @@ public class JournalTest extends AbstractJournalTest {

         // besides the former root change, now 1 also has
         final String change1 = "{\"x\":{\"y\":{}}}";
-        assertJournalEntries(ds1, "{}", change1);
+        assertJournalEntries(ds1, change1);
         final String change2 = "{\"x\":{}}";
-        assertJournalEntries(ds2, "{}", change2);
+        assertJournalEntries(ds2, change2);


         String change2b = "{\"x\":{\"y\":{\"z\":{}}}}";
@@ -400,14 +397,14 @@ public class JournalTest extends AbstractJournalTest {
             assertEquals(head2, getDocument(ds1, "/").getLastRev().get(c2Id));

             // now 1 is unchanged, but 2 was recovered now, so has one more:
-            assertJournalEntries(ds1, "{}", change1); // unchanged
-            assertJournalEntries(ds2, "{}", change2, change2b);
+            assertJournalEntries(ds1, change1); // unchanged
+            assertJournalEntries(ds2, change2, change2b);

             // just some no-ops:
             recovery.recover(c2Id);
             recovery.recover(Iterators.<NodeDocument>emptyIterator(), c2Id);
-            assertJournalEntries(ds1, "{}", change1); // unchanged
-            assertJournalEntries(ds2, "{}", change2, change2b);
+            assertJournalEntries(ds1, change1); // unchanged
+            assertJournalEntries(ds2, change2, change2b);

         } else {

@@ -439,8 +436,8 @@ public class JournalTest extends AbstractJournalTest {
             ready.await(5, TimeUnit.SECONDS);
             start.countDown();
             assertTrue(end.await(20, TimeUnit.SECONDS));
-            assertJournalEntries(ds1, "{}", change1); // unchanged
-            assertJournalEntries(ds2, "{}", change2, change2b);
+            assertJournalEntries(ds1, change1); // unchanged
+            assertJournalEntries(ds2, change2, change2b);
             for (Exception ex : exceptions) {
                 throw ex;
             }
{noformat}


was (Author: catholicon):
Attaching patch - [^OAK-3976.patch]. Notable difference from earlier version:
* {{JournalEntry}} also tracks if some branch commits have been added - this is required to
avoid the case when a commit pushes the entry and before any other commit background write
tries to push the journal (thus using the same revision for the entry and logging error that
it couldn't push the entry)
* Path counter is incremented if a new {{TreeNode}} is pushed - probably we should rename
it to something like node-count
* Lock around journal push removed
* A bit of refactor of where "numChanges" crosses threshold (now in {{done()}} method)

> journal should support large(r) entries
> ---------------------------------------
>
>                 Key: OAK-3976
>                 URL: https://issues.apache.org/jira/browse/OAK-3976
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>          Components: documentmk
>    Affects Versions: 1.3.14
>            Reporter: Stefan Egli
>            Assignee: Vikas Saurabh
>             Fix For: 1.6, 1.5.16
>
>         Attachments: OAK-3976.patch
>
>
> Journal entries are created in the background write. Normally this happens every second.
If for some reason there is a large delay between two background writes, the number of pending
changes can also accumulate. Which can result in (arbitrary) large single journal entries
(ie with large {{_c}} property).
> This can cause multiple problems down the road:
> * journal gc at this point loads 450 entries - and if some are large this can result
in a very large memory consumption during gc (which can cause severe stability problems for
the VM, if not OOM etc). This should be fixed with OAK-3001 (where we only get the id, thus
do not care how big {{_c}} is)
> * before OAK-3001 is done (which is currently scheduled after 1.4) what we can do is
reduce the delete batch size (OAK-3975)
> * background reads however also read the journal entries and even if OAK-3001/OAK-3975
are implemented the background read can still cause large memory consumption. So we need to
improve this one way or another.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message