issue #1482 initial contribution of production in-memory backend #1483

dk-github · 2019-03-07T17:26:06Z

We would like to contribute an improved implementation of in-memory backend for JanusGraph.

Please see issue #1482 for rationale and some comparative analysis vs existing "test-only" in-memory backend.

NOTE: This PR currently suggests adding the new backend rather than modifying the existing one, as it seemed cleaner and easier to compare performance within one codebase branch.
However, having read recent discussions about adding more backends into main repository vs maintenance costs, I am starting to think that actually modifying the existing in-memory backend could be a better idea.
This is because:

the modifications are fairly straightforward and do not change the general structure of existing backend
don’t add too much extra code or any external dependencies
don’t require any additional documentation or setup instructions, at least initially
all existing (and a few extra) integrity tests pass and using it in place of current version is unlikely to create any issues for current (test-only) uses
the same backend will simply be fit for production use, no new backends to maintain

Thoughts?

janusgraph-bot · 2019-03-07T17:27:06Z

Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.

dk-github · 2019-03-07T17:45:36Z

Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.

In fact I should be listed as per this: JanusGraph/legal#128 - so not sure what it complains about...

pluradj · 2019-03-07T19:46:44Z

The problem is likely that the Author email on your commit does not match the Signed-off-by email. You can follow these instructions to amend it.

Author: Dmitry Kovalev <Dmitry.Kovalev@gs.com>
Date:   Fri Mar 1 13:56:37 2019 +0000

    initial contribution of CompactInMemoryStoreManager - an improved in-memory backend for JanusGraph, which offers significantly reduced memory footprint compared to "classic" one, and is suitable for production use
    
    Signed-off-by: Dmitry Kovalev <dk.global@gmail.com>

mbrukman · 2019-03-07T21:30:39Z

Yes, there's a mismatch between Author and Signed-off-by, but interestingly enough, the DCO check passed, but it is the CLA check from @janusgraph-bot that failed, and it currently does not look at DCO (there's an outstanding feature request to add it, but since it's not yet there, we're using an external service).

There's potentially an issue because the Author and Committer are different here (though that should not invalidate the CLA check); I'm looking into this right now.

mbrukman · 2019-03-07T22:32:43Z

I think I see what the problem is; the CLA and the commit are not using the same capitalization for the email address. I anticipated this would happen when I saw your CLA submitted with upper-cased first-name/last-name in your emails, as I suspected some folks would actually lowercase their emails (and some folks on that thread already did).

In fact, I anticipated it so well that I already implemented case-insensitive email matching for CLA authors and committers (see google/code-review-bot#16); the issue here was that I hadn't actually rebuilt the binary used by @janusgraph-bot to include this latest change.

I just pushed a new build, so if my analysis is correct, the CLA issue should be resolved shortly.

dk-github · 2019-03-07T22:42:42Z

It worked indeed, thanks @mbrukman! There were internal reasons for capitals in the emails - apologies for causing trouble. But as per e-mail standard the addresses should be case-insensitive so IMO it was a generally useful fix anyway.

dk-github · 2019-03-22T11:29:01Z

just a gentle weekly reminder - would be grateful if someone could review/approve

porunov

@dk-github Thank you for the contribution! It is a valuable improvement.

I just did a first very short glance on the code. There are places where Codacy commented. I agree with them. Also, there are some small suggestions from me. I didn't check the whole code and didn't check the logic. I think it may take some time.

...sgraph-core/src/main/java/org/janusgraph/diskstorage/keycolumnvalue/inmemory/BufferPage.java

...ain/java/org/janusgraph/diskstorage/keycolumnvalue/inmemory/InMemoryKeyColumnValueStore.java

...core/src/main/java/org/janusgraph/diskstorage/keycolumnvalue/inmemory/SharedEntryBuffer.java

...ph-test/src/test/java/org/janusgraph/diskstorage/keycolumnvalue/inmemory/BufferPageTest.java

dk-github · 2020-01-07T00:11:36Z

many thanks @porunov for taking time to revisit this PR

I have only a small one (possibly nitpick) question here #1483 (comment)

replied to that comment, hopefully it answers the question

Also, could you please squash you commits into a single one?

done. it has woken up Codacy again though. the same code which it deemed to be "ok" when it was in the form of 2 commits, it now again thinks is "not up to standard" :)

please let me know if you want me to address any of its comments specifically - preferably which ones exactly...

Other then that, I don't have any more concerns.

great, does that mean that you are ready to approve?

porunov

@dk-github There are some Codacy complains which are really easy to resolve. Like remove Logger dependencies where it isn't used. Also, I see it complains on Assertion dependencies (even so assertions are used). I think, it might expect import static org.junit.jupiter.api.Assertions.assertEquals; instead of import static org.junit.jupiter.api.Assertions.*; (not sure about it. You can try it, but I am OK with both implementations .* and .assertEquals ).
Complains about long methods are fair enough, but sometimes they are hard to be re-factored. If you can split those methods on smaller methods, that would be good. If it is quite complicated, I am OK with current implementation.
A point about raw exceptions is valid also, but we use a lot of them right now. Because we don't have much style right now, I think it is optional as of committers decision (you can use raw exceptions or create custom exceptions).

So, overall it LGTM. Those tiny flows mentioned by Codacy are optional as for me. You may fix them or leave them as per your opinion.

porunov · 2020-01-08T19:57:29Z

@amcp @farodin91 @FlorianHockmann will you be able to re-review this PR?

FlorianHockmann · 2020-01-09T13:35:19Z

@amcp @farodin91 @FlorianHockmann will you be able to re-review this PR?

I think @farodin91 already made a first pass through this and if he and you both accept this PR, then I'd say that we just merge it. I probably won't have the time to review this massive PR adequately. So, I don't want to block this.
Given that we have only recommended the in-memory backend for testing purposes until now, this can't break any production usages. Since all tests also still pass with this, the in-memory backend seems to still fulfil its original purpose. Therefore this PR isn't as critical as it may seem at first. (With critical I don't mean that it's not useful, only that it shouldn't break anything.)

farodin91

@dk-github Thank you for this huge contribution.

nit: It would good to remove all unnecessary imports.

Signed-off-by: Dmitry Kovalev <dk.global@gmail.com>

FlorianHockmann · 2020-01-10T22:34:31Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 11
           

Complexity increasing per file
==============================
- janusgraph-inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/BufferPageTest.java  2
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryStoreManager.java  2
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/BufferPage.java  14
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryKeyColumnValueStore.java  9
- janusgraph-inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/MultiPageEntryBufferTest.java  1
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/SinglePageEntryBuffer.java  7
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/SharedEntryBufferFragmentationReport.java  1
- janusgraph-inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/InMemoryColumnValueStoreTest.java  9
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/MemoryEntryList.java  2
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryKeyColumnValueStoreFragmentationReport.java  4
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryColumnValueStore.java  9
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/BufferPageUtils.java  5
- janusgraph-inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/TestPagedBufferColumnValueStore.java  1
- janusgraph-inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/InMemoryStoreManagerTest.java  2
- janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/MultiPageEntryBuffer.java  22
         

Clones added
============
- janusgraph-inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/InMemoryColumnValueStoreTest.java  7

See the complete overview on Codacy

FlorianHockmann · 2020-01-10T22:34:47Z

janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/BufferPage.java

+        return newPages;
+    }
+
+    public List<BufferPage> merge(Entry[] add, int iaddp, int addLimit, Entry[] del, int idelp, int delLimit, int maxPageSize) {


Issue found: The method 'merge(Entry, int, int, Entry, int, int, int)' has an NPath complexity of 728

FlorianHockmann · 2020-01-10T22:34:48Z

...-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryKeyColumnValueStore.java

+                log.debug("finished writing chunk " + filePath + " " + Thread.currentThread().getName());
+            }
+        } catch (IOException ex) {
+            throw new RuntimeException(ex);


Issue found: Avoid throwing raw exception types.

FlorianHockmann · 2020-01-10T22:34:49Z

janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/MultiPageEntryBuffer.java

+    }
+
+    @Override
+    public void mutate(Entry[] add, Entry[] del, int maxPageSize) {


Issue found: The method 'mutate(Entry, Entry, int)' has an NPath complexity of 4476

FlorianHockmann · 2020-01-10T22:34:50Z

...inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/InMemoryColumnValueStoreTest.java

+    public static final String VERY_END = "zz";
+
+    @Test
+    public void testRoundtrip()


Issue found: Avoid really long methods.

FlorianHockmann · 2020-01-10T22:34:51Z

janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/MultiPageEntryBuffer.java

+    }
+
+    @Override
+    public EntryList getSlice(KeySliceQuery query) {


Issue found: The method 'getSlice(KeySliceQuery)' has an NPath complexity of 2364

FlorianHockmann · 2020-01-10T22:34:52Z

...aph-inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/InMemoryStoreManagerTest.java

+public class InMemoryStoreManagerTest
+{
+    @Test
+    public void testStoreCycle() throws Exception


Issue found: Avoid really long methods.

FlorianHockmann · 2020-01-10T22:34:53Z

janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryStoreManager.java

+            try {
+                newStores.put(storePath.getFileName().toString(), InMemoryKeyColumnValueStore.readFrom(storePath, storePath.getFileName().toString(), parallelOperationsExecutor));
+            } catch (IOException ex) {
+                throw new RuntimeException(ex);


Issue found: Avoid throwing raw exception types.

FlorianHockmann · 2020-01-10T22:34:54Z

janusgraph-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/MultiPageEntryBuffer.java

+    }
+
+    @Override
+    public void mutate(Entry[] add, Entry[] del, int maxPageSize) {


Issue found: Avoid really long methods.

FlorianHockmann · 2020-01-10T22:34:55Z

...-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryKeyColumnValueStore.java

@@ -104,17 +135,225 @@ public void close() throws BackendException {
        kcv.clear();
    }

+    public InMemoryKeyColumnValueStoreFragmentationReport createFragmentationReport(StoreTransaction txh) throws BackendException {


Issue found: The method 'createFragmentationReport(StoreTransaction)' has an NPath complexity of 202

FlorianHockmann · 2020-01-10T22:34:56Z

...inmemory/src/test/java/org/janusgraph/diskstorage/inmemory/InMemoryColumnValueStoreTest.java

+    }
+
+    @Test
+    public void testPagingAndFragmentation() throws TemporaryLockingException


Issue found: Avoid really long methods.

FlorianHockmann · 2020-01-10T22:34:57Z

...-inmemory/src/main/java/org/janusgraph/diskstorage/inmemory/InMemoryKeyColumnValueStore.java

+
+            return numKcvs;
+        } catch (Exception ex) {
+            throw new RuntimeException("Problem while reading chunk " + filePath + " of store " + store.getName(), ex);


Issue found: Avoid throwing raw exception types.

I believe your previous comments were resolved / answered, so I am dismissing request changes

dk-github · 2020-01-10T22:41:13Z

Thank you @porunov @farodin91 . I have removed the unused imports/changed wildcards to specific imports - this seems to have pacified codacy a little bit.

Please let me know if I need to address anything else, or it is good to be merged.

porunov · 2020-01-10T22:41:47Z

@dk-github I think you can merge this PR after Travis tests are finished

dk-github · 2020-01-11T17:13:30Z

Ok, the build passed and I have merged it now.

Many thanks to everyone who took time to review it, hope it will add some value to the project!

I will let it rest for a while, and then raise a small PR to update documentation about in-memory backend production usage and limitations.

Also later this year may raise a number of small PRs with minor refactorings/improvements as/when we get to work on upgrading our system to janusgraph 0.5.0 and switching from (slightly more feature-rich) custom in-house backend to the contributed version.

* Issue JanusGraph#1871: Close graph instance at end of mapper run. Signed-off-by: Ted Wilmes <ted.wilmes@experoinc.com> * Fixed broken dist docker-compose (updated to supported ES version) Signed-off-by: Michal Podstawski <mpodstawski@gmail.com> * Minor code cleanup (NPEs etc) Signed-off-by: Michal Podstawski <mpodstawski@gmail.com> * Spelling fixes * actually * amend * assumed * backend * cassandra * centric * check * cohabitors * configured * conjunction * connections * containing * control * currently * default * disabled-the * exhaust * existing * explicitly * generation * geoshape * graph-class * graph * gremlin * implement * increment * information * initial * instance * interfaces * it's * janus * labels * levenshtein * logies * message * nonexistent * overridden * overriding * parameterized * params * parsable * partitioner * password * payload * persistent * preceded * propagate * pseudo * recommended * requires * sequence * submission * temporary * truststore * unknown * upgrading * version * writing Signed-off-by: Josh Soref <jsoref@users.noreply.github.com> * Add cell ttl support to BerkeleyDB Signed-off-by: Pavel Ershov <owner.mad.epa@gmail.com> * Improve added relations containers JanusGraph#1700 Signed-off-by: Pavel Ershov <owner.mad.epa@gmail.com> * Refactor in ES module Signed-off-by: Michał Podstawski <mpodstawski@gmail.com> * Add fixes for TP tests on berkeley backend Signed-off-by: Pavel Ershov <owner.mad.epa@gmail.com> * Add fixes for TP tests on berkeley backend Signed-off-by: Pavel Ershov <owner.mad.epa@gmail.com> * Add log4j.properties to inmemory Signed-off-by: Jan Jansen <jan.jansen@gdata.de> * JANUSGRAPH-1866 Filter out only system vertices in Hadoop Vertex deserializer Remove erroneously added unused import. Test that schema vertices are skipped. Hadoop vertices deserialization should skip schema vertices that are created implicitly when defining schema elements like labels. Correct tests for HBase Snapshot input format. Snapshot should be taken before reading the graph in order to have anything to read from. Signed-off-by: Evgeniy Ignatiev <yevgeniy.ignatyev@gmail.com> * Update Copyright year in documentation CTR [doc only] Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> * Extract JanusGraph Gremlin driver requirements * Predicates * Geoshape * RelationIdenitifier Signed-off-by: Jan Jansen <jan.jansen@gdata.de> * * Improve the CQLIterator performance by using getPagingStateUnsafe ( this should avoid md5sum calculation of resultset) Signed-off-by: Ganesh Guttikonda <gguttikonda@snapfish-llc.com> * Update to TinkerPop 3.4.4 Fixes JanusGraph#1617 Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> * upgrading inmemory backend storage layout to reduce memory footprint (JanusGraph#1483) Signed-off-by: Dmitry Kovalev <dk.global@gmail.com> * Add testcontainers support for cassandra [full build] Fixes JanusGraph#1475 * Update jacoco * Cleanup pom.xml * Introduce profiles for Cassandra * Update TESTING.md Signed-off-by: Jan Jansen <jan.jansen@gdata.de> * Add 'Getting Started' guide to documentation [doc only] Signed-off-by: Florian Grieskamp <florian.grieskamp@gdata.de> * Fix installation docs missing hadoop-2 in examples CTR [doc only] Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> * JanusGraph release 0.3.3 [full build] Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> * JanusGraph release 0.4.1 [full build] Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> * [doc only] Updated in-memory backend documentation (JanusGraph#1934) to explain possible production use cases, limitations and alternatives (issue JanusGraph#1929) Signed-off-by: Dmitry Kovalev <dk.global@gmail.com> * Split up hadoop implementations [full build] Signed-off-by: Jan Jansen <jan.jansen@gdata.de> * Fix inmemory docs format CTR [doc only] Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> * Bump jackson2.version from 2.6.6 to 2.10.2 Fixes JanusGraph#1307 Signed-off-by: Jan Jansen <jan.jansen@gdata.de> * Bump v0.3 branch to 0.3.4-SNAPSHOT CTR [doc only] Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> * Bump v0.4 branch to 0.4.2-SNAPSHOT CTR [doc only] Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com> Co-authored-by: Ted Wilmes <twilmes@gmail.com> Co-authored-by: micpod <57301006+micpod@users.noreply.github.com> Co-authored-by: Josh Soref <jsoref@users.noreply.github.com> Co-authored-by: Pavel <owner.mad.epa@gmail.com> Co-authored-by: Oleksandr Porunov <alexandr.porunov@gmail.com> Co-authored-by: Jan Jansen <farodin91@users.noreply.github.com> Co-authored-by: Evgeniy Ignatiev <YevIgn@users.noreply.github.com> Co-authored-by: gani8780 <gguttikonda@snapfish-llc.com> Co-authored-by: Dmitry Kovalev <dk.global@gmail.com> Co-authored-by: rngcntr <7890887+rngcntr@users.noreply.github.com>

janusgraph-bot added the cla: no This PR is not compliant with the CLA label Mar 7, 2019

dk-github force-pushed the issue1482-compact-inmemory-backend branch from 7fd3a75 to 63544e3 Compare March 7, 2019 17:28

dk-github mentioned this pull request Mar 7, 2019

New contribution - production-worthy in-memory backend with no external dependencies #1482

Closed

mbrukman self-assigned this Mar 7, 2019

janusgraph-bot added cla: yes This PR is compliant with the CLA and removed cla: no This PR is not compliant with the CLA labels Mar 7, 2019

mbrukman removed their assignment Mar 7, 2019

JanusGraph deleted a comment Mar 15, 2019

dk-github force-pushed the issue1482-compact-inmemory-backend branch from 3ead653 to a7554ea Compare March 15, 2019 00:11

JanusGraph deleted a comment Mar 15, 2019

porunov reviewed Mar 30, 2019

View reviewed changes

dk-github force-pushed the issue1482-compact-inmemory-backend branch 2 times, most recently from 86cfaa1 to 59a03da Compare January 7, 2020 00:03

porunov approved these changes Jan 7, 2020

View reviewed changes

farodin91 approved these changes Jan 10, 2020

View reviewed changes

upgrading inmemory backend storage layout to reduce memory footprint

988c272

Signed-off-by: Dmitry Kovalev <dk.global@gmail.com>

dk-github force-pushed the issue1482-compact-inmemory-backend branch from 59a03da to 988c272 Compare January 10, 2020 22:31

JanusGraph deleted a comment from farodin91 Jan 10, 2020

FlorianHockmann reviewed Jan 10, 2020

View reviewed changes

dk-github merged commit a5a403f into JanusGraph:master Jan 11, 2020

dk-github deleted the issue1482-compact-inmemory-backend branch January 11, 2020 18:17

porunov mentioned this pull request Jan 12, 2020

Document production in memory storage #1929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #1482 initial contribution of production in-memory backend #1483

issue #1482 initial contribution of production in-memory backend #1483

dk-github commented Mar 7, 2019 •

edited

janusgraph-bot commented Mar 7, 2019

dk-github commented Mar 7, 2019

pluradj commented Mar 7, 2019

mbrukman commented Mar 7, 2019

mbrukman commented Mar 7, 2019

dk-github commented Mar 7, 2019

dk-github commented Mar 22, 2019

porunov left a comment

dk-github commented Jan 7, 2020

porunov left a comment

porunov commented Jan 8, 2020

FlorianHockmann commented Jan 9, 2020

farodin91 left a comment

FlorianHockmann commented Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

FlorianHockmann Jan 10, 2020

dk-github commented Jan 10, 2020 •

edited

porunov commented Jan 10, 2020

dk-github commented Jan 11, 2020

issue #1482 initial contribution of production in-memory backend #1483

issue #1482 initial contribution of production in-memory backend #1483

Conversation

dk-github commented Mar 7, 2019 • edited

janusgraph-bot commented Mar 7, 2019

dk-github commented Mar 7, 2019

pluradj commented Mar 7, 2019

mbrukman commented Mar 7, 2019

mbrukman commented Mar 7, 2019

dk-github commented Mar 7, 2019

dk-github commented Mar 22, 2019

porunov left a comment

Choose a reason for hiding this comment

dk-github commented Jan 7, 2020

porunov left a comment

Choose a reason for hiding this comment

porunov commented Jan 8, 2020

FlorianHockmann commented Jan 9, 2020

farodin91 left a comment

Choose a reason for hiding this comment

FlorianHockmann commented Jan 10, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dk-github commented Jan 10, 2020 • edited

porunov commented Jan 10, 2020

dk-github commented Jan 11, 2020

dk-github commented Mar 7, 2019 •

edited

dk-github commented Jan 10, 2020 •

edited