Evict graph from cache upon configuration updates #897

dpitera · 2018-01-18T20:50:22Z

When a graph configuration is updated or removed from the
ConfigurationGraphManagement, the related graph is evicted from the
cache on all JanusGraph nodes in the cluster. This is done to prevent
users from opening graphs with old/incorrect configurations across the
cluster and finding themselves in an inconsistent state. Note that this
is only done for users updating the ConfigurationManagementGraph by
going through the ConfiguredGraphFactory APIs; this is done for two
reasons: 1. to allow more fine-tuned control for users who wish to use
the lower-levls CMG APIs and 2. to avoid a cyclical dependency between
the CMG and the JanusGraphManager.

Issues: #590

Signed-off-by: David Pitera [email protected]

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there an issue associated with this PR? Is it referenced in the commit message?
Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

Have you written and/or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?
If this PR is a documentation-only change, have you added a [skip ci]
tag to the first line of your commit message to avoid spending CPU cycles in
Travis CI when no code, tests, or build configuration are modified?

Note:

Please ensure that once the PR is submitted, you check Travis CI for build issues and submit an update to your PR as soon as possible.

dpitera · 2018-01-18T20:51:22Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java

@@ -744,6 +746,30 @@ public IndexJobFuture updateIndex(Index index, SchemaAction updateAction) {
        return future;
    }

+    /**
+     * Upon the open managementsystem's commit, this graph will be evicted from the cache on all JanusGraph nodes in your
+     * cluster, once there are no open transactions on this graph on each respective JanusGraph node.


and assuming each node in the cluster is configured correctly to use the JGM

sjudeng

Can you reference the issue number in the PR description? Also do you think we'll eventually want to move JanusGraphManager to the janusgraph-server module?

sjudeng · 2018-01-28T01:53:39Z

...-core/src/main/java/org/janusgraph/graphdb/database/management/GraphCacheEvictionAction.java

+
+public enum GraphCacheEvictionAction {
+
+    EVICT, DO_NOT_EVICT


Split to separate lines

sjudeng · 2018-01-28T01:53:58Z

...-core/src/main/java/org/janusgraph/graphdb/database/management/GraphCacheEvictionAction.java

@@ -0,0 +1,21 @@
+// Copyright 2017 JanusGraph Authors


"2018" or "2017-2018" depending on the advice in the other PR

sjudeng · 2018-01-28T01:54:32Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementLogger.java

@@ -88,7 +92,9 @@ public void read(Message message) {
                    long typeId = VariableLong.readPositive(in);
                    schemaCache.expireSchemaElement(typeId);
                }
-                Thread ack = new Thread(new SendAckOnTxClose(evictionId, senderId, graph.getOpenTransactions()));
+                GraphCacheEvictionAction action = serializer.readObjectNotNull(in, GraphCacheEvictionAction.class);


sjudeng · 2018-01-28T01:54:36Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementLogger.java

-                Thread ack = new Thread(new SendAckOnTxClose(evictionId, senderId, graph.getOpenTransactions()));
+                GraphCacheEvictionAction action = serializer.readObjectNotNull(in, GraphCacheEvictionAction.class);
+                Preconditions.checkNotNull(action);
+                Thread ack = new Thread(new SendAckOnTxClose(evictionId, senderId, graph.getOpenTransactions(), action, graph.getGraphName()));


sjudeng · 2018-01-28T01:58:02Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementLogger.java

@@ -127,6 +134,8 @@ public void sendCacheEviction(Set<JanusGraphSchemaVertex> updatedTypes,
            assert type.hasId();
            VariableLong.writePositive(out,type.longId());
        }
+        if (evictGraphFromCache) out.writeObjectNotNull(EVICT);


Split to separate lines and add curly braces

sjudeng · 2018-01-28T01:59:08Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java

+    /**
+     * Upon the open managementsystem's commit, this graph will be asynchronously evicted from the cache on all JanusGraph nodes in your
+     * cluster, once there are no open transactions on this graph on each respective JanusGraph node
+     * and assuming each node is correctly configured to use the {@link JanusGraphManager}.


Need to import JanusGraph manager or include full path in link.

sjudeng · 2018-01-28T02:02:23Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementLogger.java

+                final JanusGraphManager jgm = JanusGraphManager.getInstance();
+                final boolean janusGraphManagerIsInBadState = null == jgm && action.equals(EVICT);
+                if (!txStillOpen && janusGraphManagerIsInBadState) {
+                    log.error("JanusGraphManager should be instantiated on this server, but it is not. " +


Can you add a test to cover this case?

Note that this would be hit asynchronously on a node other than the one the original action was made on, so this is a log action-- not the throwing of an error. I can add a test that ensures if the JGM is null that we don't remove it from the cache, but that is pretty obvious.

In terms of testing this functionality across multi-node deployments, I would like to avoid if we can as it would be ~non-trivial as there is no infra in place to do so, and there is also no infra in place to test the existing ManagementLogger.

ManagementLogger seems to be otherwise well covered in tests (see Codecov). If the test you describe hits this case I think it's worth including.

That code-coverage analysis is pretty cool!

I can hit the case with the simpler test of opening a graph without configuring the JGM and trying to evict the graph from the cache.

sjudeng · 2018-01-28T02:05:58Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementLogger.java

@@ -191,12 +208,24 @@ public void run() {
                        txStillOpen = true;
                    }
                }
-                if (!txStillOpen) {
+                final JanusGraphManager jgm = JanusGraphManager.getInstance();
+                final boolean janusGraphManagerIsInBadState = null == jgm && action.equals(EVICT);


If jgm is null couldn't that just mean that JanusGraph Server is not being used?

The only way action.equals(EVICT) is if the user actively called mgmt.evictGraphFromCache() on their graph's mangementSystem, or if it was called by the ConfiguredGraphFactory updateConfiguration or removeConfiguration.

The issue in this scenario is indeed that a user is using the JanusGraph Server (more specifically configured to use the JanusGraphManager) for one of their JG nodes but not the other node (the one that received this eviction request).

dpitera · 2018-02-07T20:16:37Z

@sjudeng Pushed new code with your suggested updates and I added 2 tests, both testing the graph management logger's graph cache eviction logic; one when the JGM is null (mostly an obvious test, but we will see the code get covered in the codecov analysis) and one when it is not (the test proving the functionality works, i.e. the log can successfully evict graphs from the cache).

dpitera · 2018-02-08T19:59:16Z

Also do you think we'll eventually want to move JanusGraphManager to the janusgraph-server module?

Sorry, I forgot to answer this question. Artificially, maybe the JanusGraphManager should live in the janusgraph-server module. I think it's easy to sound like this is the right place to live, especially when one considers that the notion of a GraphManager is a TinkerPop ideation. Concretely, this gets complicated because things like the JanusGraphFactory, ConfiguredGraphFactory, things that live in the janusgraph-core module... rely on the JanusGraphManager; this would cause a cyclical dependency between the two modules. Even theoretically I am more of the notion that the core project is indeed the best place for the JGM to reside. What do you think? Might not be a bad thing over which to open a discussion. Either way, if the community deems it is best to move, then it would be best done in a separate PR with its own refactoring issue.

sjudeng · 2018-02-09T01:30:40Z

Thanks for the explanation. It looks to me like ConfiguredGraphFactory could maybe be moved over as is. I don't see any integration to other classes in core. JGM would be more painful given integration in JanusGraphFactory and (with this PR) ManagementLogger. Can we create a separate issue to investigate further, maybe specifically for moving over ConfiguredGraphFactory if you agree that would be straight-forward, as well as for identifying/moving any other relevant classes if any?

sjudeng

Looks good.

sjudeng · 2018-02-09T01:32:15Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementLogger.java

-                if (!txStillOpen) {
+                final JanusGraphManager jgm = JanusGraphManager.getInstance();
+                final boolean janusGraphManagerIsInBadState = null == jgm && action.equals(EVICT);
+                if (!txStillOpen && janusGraphManagerIsInBadState) {


Looks like this branch still isn't getting hit (see here). It's not a big deal.

pluradj

looks good, mostly nits

pluradj · 2018-02-22T20:29:52Z

janusgraph-core/src/main/java/org/janusgraph/core/ConfiguredGraphFactory.java

+            final JanusGraph graph = open(graphName);
+            removeGraphFromCache(graph);
+        } catch (Exception e) {
+            // cannot open graph, do nothing


pluradj · 2018-02-22T20:30:03Z

janusgraph-core/src/main/java/org/janusgraph/core/ConfiguredGraphFactory.java

+            final JanusGraph graph = open(graphName);
+            removeGraphFromCache(graph);
+        } catch (Exception e) {
+            // cannot open graph, do nothing


pluradj · 2018-02-22T20:40:37Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/serialize/StandardSerializer.java

@@ -115,6 +116,7 @@ public StandardSerializer() {
        registerClassInternal(51,SchemaStatus.class, new EnumSerializer<>(SchemaStatus.class));
        registerClassInternal(52,LogTxStatus.class, new EnumSerializer<>(LogTxStatus.class));
        registerClassInternal(53,MgmtLogType.class, new EnumSerializer<>(MgmtLogType.class));
+        registerClassInternal(69,GraphCacheEvictionAction.class, new EnumSerializer<>(GraphCacheEvictionAction.class));


this probably doesn't need to be at this exact spot, so move it down to make it easier to find the highest registration number.

pluradj · 2018-02-22T20:53:26Z

docs/configuredgraphfactory.adoc

@@ -83,7 +83,7 @@ for which you have instantiated _and_ the references are stored inside the `Janu

 IMPORTANT: This is an irreversible operation that will delete all graph and index data.

-IMPORTANT: To ensure all graph representations are consistent across all JanusGraph nodes in your cluster, remove the graph from the `JanusGraphManager` graph reference tracker on all nodes in your cluster: `ConfiguredGraphFactory.close("graphName");`.
+IMPORTANT: To ensure all graph representations are consistent across all JanusGraph nodes in your cluster, this removes the graph from the `JanusGraphManager` graph cache on every node in the cluster, assuming each node has been properly configured to use the `JanusGraphManager`.


Rather than having important call-out boxes (this one and the one later on) "assuming each node has been properly configured to use the JanusGraphManager", add a section in this doc that describes how to properly configure an environment with multiple Gremlin Servers. In that section, you could discuss the extra stuff that JanusGraph does to ensure consistency across the Gremlin Servers (perhaps discuss the scenario from the original issue) and also what might go wrong if there is a misconfigured Gremlin Server in the mix.

I think this suggestion makes alot of sense; I prefer to leave the call out and link to the new section about using a multinode JG cluster. I also think the information in this PR here will be relevant, so I suggest creating a separate PR with this new doc section that includes the information relevant to both PRs; this will avoid merge-conflicts between these two PRs.

separate PR for docs update is fine, see #938

pluradj · 2018-02-22T20:54:24Z

docs/configuredgraphfactory.adoc

----
-ConfiguredGraphFactory.close("graph2");
----
+IMPORTANT: Any updates to a graph configuration results in the eviction of the relevant graph from the graph cache on every node in the JanusGraph cluster, assuming each node has been configured properly to use the `JanusGraphManager`.


see previous comment

pluradj · 2018-02-22T21:28:28Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementLogger.java


 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 import java.time.Duration;
+import java.util.HashSet;


this isn't used

dpitera · 2018-02-26T17:23:21Z

@pluradj Pushed update with suggested fixes.

amcp

@pluradj @sjudeng seems like all your comments have been addressed.

amcp · 2018-03-14T20:09:40Z

...-core/src/main/java/org/janusgraph/graphdb/database/management/GraphCacheEvictionAction.java

+
+public enum GraphCacheEvictionAction {
+
+    EVICT,


consider documenting each of the enum values with a Javadoc.

pluradj

thanks david

When a graph configuration is updated or removed from the ConfigurationGraphManagement, the related graph is evicted from the cache on all JanusGraph nodes in the cluster. This is done to prevent users from opening graphs with old/incorrect configurations across the cluster and finding themselves in an inconsistent state. Note that this is only done for users updating the ConfigurationManagementGraph by going through the ConfiguredGraphFactory APIs; this is done for two reasons: 1. to allow more fine-tuned control for users who wish to use the lower-levls CMG APIs and 2. to avoid a cyclical dependency between the CMG and the JanusGraphManager. Issues: JanusGraph#590 Signed-off-by: David Pitera <[email protected]>

Evict graph from cache upon configuration updates

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Jan 18, 2018

dpitera commented Jan 18, 2018

View reviewed changes

dpitera force-pushed the graph-cache-evictions branch 3 times, most recently from f7e5a6a to 82cbadd Compare January 19, 2018 21:32

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Jan 22, 2018

dpitera force-pushed the graph-cache-evictions branch 4 times, most recently from de34a61 to 06d519e Compare January 22, 2018 22:51

dpitera changed the title ~~WIP WIP WIP~~ Evict graph from cache upon configuration updates Jan 22, 2018

sjudeng reviewed Jan 28, 2018

View reviewed changes

dpitera force-pushed the graph-cache-evictions branch from 06d519e to aeb34e5 Compare February 7, 2018 20:13

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Feb 8, 2018

sjudeng approved these changes Feb 9, 2018

View reviewed changes

pluradj requested changes Feb 22, 2018

View reviewed changes

pluradj mentioned this pull request Feb 25, 2018

update ConfiguredGraphFactory docs for new channelizers and graph eviction #938

Closed

dpitera force-pushed the graph-cache-evictions branch from aeb34e5 to 09ffe28 Compare February 26, 2018 17:22

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Mar 1, 2018

amcp approved these changes Mar 14, 2018

View reviewed changes

pluradj approved these changes Mar 16, 2018

View reviewed changes

dpitera force-pushed the graph-cache-evictions branch from 09ffe28 to 8cbe871 Compare March 16, 2018 19:51

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Mar 19, 2018

dpitera merged commit 9071f9c into JanusGraph:master Mar 19, 2018

pluradj added this to the Release v0.3.0 milestone Mar 29, 2018

bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019

Merge pull request JanusGraph#897 from dpitera/graph-cache-evictions

48619c9

Evict graph from cache upon configuration updates

micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019

Merge pull request JanusGraph#897 from dpitera/graph-cache-evictions

c08ce72

Evict graph from cache upon configuration updates

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evict graph from cache upon configuration updates #897

Evict graph from cache upon configuration updates #897

dpitera commented Jan 18, 2018 •

edited

Loading

dpitera Jan 18, 2018

sjudeng left a comment

sjudeng Jan 28, 2018

sjudeng Jan 28, 2018

sjudeng Jan 28, 2018

sjudeng Jan 28, 2018

sjudeng Jan 28, 2018

sjudeng Jan 28, 2018

sjudeng Jan 28, 2018

dpitera Feb 6, 2018

sjudeng Feb 6, 2018 •

edited

Loading

dpitera Feb 6, 2018

sjudeng Jan 28, 2018

dpitera Feb 6, 2018 •

edited

Loading

dpitera commented Feb 7, 2018

dpitera commented Feb 8, 2018

sjudeng commented Feb 9, 2018

sjudeng left a comment

sjudeng Feb 9, 2018

pluradj left a comment

pluradj Feb 22, 2018

pluradj Feb 22, 2018

pluradj Feb 22, 2018

pluradj Feb 22, 2018

dpitera Feb 23, 2018

pluradj Feb 25, 2018

pluradj Feb 22, 2018

pluradj Feb 22, 2018

dpitera commented Feb 26, 2018

amcp left a comment

amcp Mar 14, 2018

pluradj left a comment

Evict graph from cache upon configuration updates #897

Evict graph from cache upon configuration updates #897

Conversation

dpitera commented Jan 18, 2018 • edited Loading

For all changes:

For code changes:

For documentation related changes:

Note:

Choose a reason for hiding this comment

sjudeng 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

Choose a reason for hiding this comment

sjudeng Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpitera Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

dpitera commented Feb 7, 2018

dpitera commented Feb 8, 2018

sjudeng commented Feb 9, 2018

sjudeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pluradj 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

Choose a reason for hiding this comment

dpitera commented Feb 26, 2018

amcp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pluradj left a comment

Choose a reason for hiding this comment

dpitera commented Jan 18, 2018 •

edited

Loading

sjudeng Feb 6, 2018 •

edited

Loading

dpitera Feb 6, 2018 •

edited

Loading