JanusGraph / janusgraph

JanusGraph: an open-source, distributed graph database
https://janusgraph.org
Other
5.28k stars 1.16k forks source link

Not able to delete ghost vertices caused by stale index #1099

Open anamika121 opened 6 years ago

anamika121 commented 6 years ago

Over the period of time we are observing some ghost vertices on our production setup where if we do gremlin query for having msid =18038893 shows ghost vertex. gremlin> g.V().has('msid',18038893) ==>v[7244255240]

but if we try to delete this vertex it shows that Vertex with id 7433638024 was removed. gremlin> g.V().has('msid',18038893).drop() Vertex with id 7244255240 was removed. Type ':help' or ':h' for help. Display stack trace? [

gremlin> g.V().has('msid',18869037).valueMap() Vertex with id 7433638024 was removed. gremlin> g.V().has('msid',18133305) ==>v[7228928032]

I am not able to recreate vertex with given msid again in the graph. It seems that index vertex has already been deleted but index on msid has not been updated correctly and has been corrupted. Kindly suggest how to remove such cases.

Miroka96 commented 6 years ago

Similar issue for me:

gremlin> v = g.V(2598101162).next() ==>v[2598101162] gremlin> g.V(v).outE() ==>e[iq0ledl-16yubm2-3cwl-37s][2598101066-badHost->4168] gremlin> g.V(v).outE().outV() ==>v[2598101066] gremlin> g.V(v).outE().outV().next() == v ==>true

I think, my (unique) index is broken, because the same vertex is reachable from two different vertex IDs. And if I step from a vertex out to an edge and back to the outVertex, I get one which is equal but has a different vertex ID.

Maybe my unique Index prevents the duplicates from being found. I have 400k vertices in my graph, created by only 160k unique data elements. They have been inserted through Spark in parallel with 96 threads. My backend is Cassandra.

yosemiteAGM commented 6 years ago

Have been having a similar issue: We've been running a homegrown dataGrooming tool for over a year to look for ghost/phantom vertices and remove them from our graph. Most of them seem to be caused by updates and deletes coming in on different db nodes (we run JanusGraph on HBase) at the same time.

Up until recently, once the vertex was identified, we could just do vertex.remove() to get rid of it. But now, while we get no errors running the remove() command and committing it, the vertices are still in the graph the next time I look. (Like animika121's issue above).

I tried using g.V().drop() just as a change of pace, but that did not help. Short of re-loading the db from a GraphSON snapshot, is there a way to get these vertices out of our graph? It seems like if a vertex can be retrieved, there should be a way to remove it even if it is in a ghost-like (ie. corrupted) state.

yosemiteAGM commented 6 years ago

NOTE - We have found a work-around for now. Not only can we retrieve a "removed" vertex, but can still update it. If we first add this node's indexed properties back, commit the update and then do the remove again, the node is successfully removed from the db. So it appears that there must have been an index pointing at the node that was not getting cleared when the node was removed. Still hoping that there is a more elegant way to do this?

pankajydv commented 6 years ago

Thanks @yosemiteAGM - we can now at least delete these ghosts. Basically the steps we followed is as follows:

g.V().has('msid',18038893).valueMap() ==>[] g.V().has('msid',18038893).property('msid', 18038894) g.V().has('msid',18038894).valueMap() ==>[msid:[18038894]] g.V().has('msid',18038894).drop()

mad commented 5 years ago

JG has special job for removing ghost vertices GhostVertexRemover that job was fixed in here #1731

li-boxuan commented 3 years ago

As @mad pointed out, GhostVertexRemover job can be used to delete ghost vertices. Using it in MapReduce would need some custom work, though. Created https://github.com/JanusGraph/janusgraph/issues/2555 to enhance it so that it can be used out-of-the-box.

porunov commented 2 years ago

@li-boxuan I'm re-opening this issue because I think GhostVertexRemover doesn't fix all possible issues. It looks like sometimes a composite index may be somehow corrupted and point to non-existent vertex (removed vertex). If so, GhostVertexRemover will not fix those indices. Neither reindex process fixes it. I guess it could be caused by situations when you remove vertices in bulk transactions and suddenly your server crashes at the middle of the removal. That's only my initial assumption but I didn't verify if that's really the case. I think during the crash we could remove the vertex from the storage backend but don't update an index due to the crash. Another reason I couldn't think of is a simultaneous update of the index.

In our situation I do bump into such vertices from time to time even on master branch. I can assure that there are not simultaneous updates conducted to the vertex which is being removed, so I guess it's more likely due to the server crash in the middle of vertices removal.

I think it would be nice to be able to repair a composite index or to remove a specific vertex from the index without removing it from the underlying storage backend.

li-boxuan commented 2 years ago

It looks like sometimes a composite index may be somehow corrupted and point to non-existent vertex (removed vertex).

@porunov I agree with you and I think I've seen this issue too. I would call it "stale index" rather than "ghost vertex" because, in JanusGraph's philosophy, only primary storage is the source of truth.

That being said, I can see this issue is exactly about "stale index" rather than the "ghost vertex" I thought about, so you are right, this issue should be kept open.

porunov commented 2 years ago

this issue is exactly about "stale index" rather than the "ghost vertex"

Agree with you. stale index better fits this issue rather than ghost vertex

porunov commented 2 years ago

As a workaround solution I added StaleIndexRecordUtil.java tool which may be used to fix the above problems with stale indices. The tool will be available starting from JanusGraph version 1.0.0.

To fix stale indices for composite vertex indices you may use the next method: https://github.com/JanusGraph/janusgraph/blob/deb1f206da1dfaff05957f9237a711966afa3be0/janusgraph-core/src/main/java/org/janusgraph/graphdb/database/util/StaleIndexRecordUtil.java#L157

To fix stale indices for composite edge or vertex indices you may use the next method (slightly more complex but JavaDoc with examples is added to this method): https://github.com/JanusGraph/janusgraph/blob/deb1f206da1dfaff05957f9237a711966afa3be0/janusgraph-core/src/main/java/org/janusgraph/graphdb/database/util/StaleIndexRecordUtil.java#L227

baritra commented 2 years ago

@porunov Thanks for this too, as we are also facing similar index staleness issues. However, how do we prevent it in the first place?

baritra commented 2 years ago

For your earlier description -- one possible prevention mechanism could be to minimize shutdowns during update -- by adding more graceful shutdowns. What are potential other prevention methods @porunov

porunov commented 2 years ago

What are potential other prevention methods @porunov

It's an interesting topic to investigate and I believe solutions might change from storage backend to a storage backend. As for Cassandra I would suspect (not confirmed) that if your transaction is bigger that your configured batch size then your atomicity isn't guaranteed. If so, in case you persisted part of the transaction (the first batch CQL request) but then your JanusGraph server died for any reason then there is a chance that changes won't be persisted into the relative index which will result in a stale index. In this case I think the best way to fix a problem is to make sure that your batch requests are big enough to fit your transactions. If anyone has any other suspects why stale index could happen I would also like to hear your thoughts.

baritra commented 2 years ago

Thanks again @porunov

Does that mean I should set storage.cql.atomic-batch-mutate to true and storage.cql.batch-statement-size to a value that's greater than the maximum number of vertices mutated in my transactions?

porunov commented 2 years ago

Does that mean I should set storage.cql.atomic-batch-mutate to true and storage.cql.batch-statement-size to a value that's greater than the maximum number of vertices mutated in my transactions?

Yes, I think that's almost right. I'm not sure that the amount of changed vertices will correlate to batch statement size. I guess finding correct batch statements size is trickier but otherwise yes, that's right. storage.cql.atomic-batch-mutate will have some overhead (see logged / unlogged batch in Cassandra) but it should protect you from such scenarios.

baritra commented 2 years ago

Thanks a lot @porunov !! I will try those. I will also look at #statements for our largest transactions (not sure how to do that yet)

We are unfortunately stuck with Janusgraph 0.5 (can't update for other reasons). How can we get your StaleIndexRecordUtil in 0.5

porunov commented 2 years ago

We are unfortunately stuck with Janusgraph 0.5 (can't update for other reasons). How can we get your StaleIndexRecordUtil in 0.5

I had to refactor some part of the code to add that utility. I didn't had a plan to add it to 0.5 branch. I'm OK if anyone wants to contribute that feature into 0.5 branch.

baritra commented 2 years ago

@porunov I experimented with the batch size and batch atomicity properties, and had the following findings, and follow up questions.

  1. It seems that if I turn on batch-atomicity, batch size is ignored, and batches of CQL statements will grow as large as they need to. The implementation ignores batch size for atomic batches: https://github.com/JanusGraph/janusgraph/blob/4ce3e4b9c14ded0bc2f78a69cb0d9077dc54aa77/janusgraph-cql/src/main/java/org/janusgraph/diskstorage/cql/function/mutate/CQLExecutorServiceMutateManyLoggedFunction.java

  2. For a transaction that adds multiple vertices and edges, even with batch atomicity ON, I see multiple CQL batch statements are being sent to Cassandra. If that's the case, even though the individual batches are atomic, I can't guarantee the atomicity of the transaction as a whole, right? Do you think turning on batch atomicity would still help avoid this issue?

baritra commented 2 years ago

@porunov @li-boxuan -- will be really grateful if you could shed some light on the above.

li-boxuan commented 2 years ago

The implementation ignores batch size for atomic batches

I think this makes sense. After all, people want atomicity with storage.cql.atomic-batch-mutate. Splitting the statements into batches and having atomicity for each batch does not make much sense to me. I think the doc could clarify a bit. I guess @porunov remembered it wrong.

I see multiple CQL batch statements are being sent to Cassandra

Do you have a chance to sample these statements? If you could paste them here, it would be very helpful for debugging! One possibility is that your number of statements is larger than storage.buffer-size and thus they get split.

If possible, the quickest way to figure it out is to use IDE's debugger to inspect the CQL statements by yourself.

baritra commented 2 years ago

@porunov @li-boxuan

I think atomic batches are working completely fine. However, I think there might be a bug with the unlogged (non-atomic) batching of CQL statements. Here's why.

For non-atomic batches, I found that if a transaction requires, say 200 mutation statements, Janusgraph does not generate 10 batch CQL statements (with 20 mutations each). Instead, there are many more batch statements with lower batch-size, some even containing just one statement.

I think the reason is that the following batching code only batches requests for the same keys. Batching does not happen on mutation statements across keys: https://github.com/JanusGraph/janusgraph/blob/v0.5/janusgraph-cql/src/main/java/org/janusgraph/diskstorage/cql/CQLStoreManager.java#L496-L529 ( I am quoting this from 0.5 branch, but it also exists in 0.6)

    private void mutateManyUnlogged(final Map<String, Map<StaticBuffer, KCVMutation>> mutations, final StoreTransaction txh) throws BackendException {
        final MaskedTimestamp commitTime = new MaskedTimestamp(txh);

        final Future<Seq<ResultSet>> result = Future.sequence(this.executorService, Iterator.ofAll(mutations.entrySet()).flatMap(tableNameAndMutations -> {
            final String tableName = tableNameAndMutations.getKey();
            final Map<StaticBuffer, KCVMutation> tableMutations = tableNameAndMutations.getValue();

            final CQLKeyColumnValueStore columnValueStore = Option.of(this.openStores.get(tableName))
                    .getOrElseThrow(() -> new IllegalStateException("Store cannot be found: " + tableName));
            return Iterator.ofAll(tableMutations.entrySet()).flatMap(keyAndMutations -> {
                final StaticBuffer key = keyAndMutations.getKey();
                final KCVMutation keyMutations = keyAndMutations.getValue();

                final Iterator<Statement> deletions = Iterator.of(commitTime.getDeletionTime(this.times))
                        .flatMap(deleteTime -> Iterator.ofAll(keyMutations.getDeletions()).map(deletion -> columnValueStore.deleteColumn(key, deletion, deleteTime)));
                final Iterator<Statement> additions = Iterator.of(commitTime.getAdditionTime(this.times))
                        .flatMap(addTime -> Iterator.ofAll(keyMutations.getAdditions()).map(addition -> columnValueStore.insertColumn(key, addition, addTime)));

                return Iterator.concat(deletions, additions)
                        .grouped(this.batchSize)
                        .map(group -> Future.fromJavaFuture(this.executorService,
                                this.session.executeAsync(
                                        new BatchStatement(Type.UNLOGGED)
                                                .addAll(group)
                                                .setConsistencyLevel(getTransaction(txh).getWriteConsistencyLevel()))));
            });
        }));

        result.await();
        if (result.isFailure()) {
            throw EXCEPTION_MAPPER.apply(result.getCause().get());
        }
        sleepAfterWrite(txh, commitTime);
    }

In contrast, when atomic (logged) batching is turned on, batching is performed across keys, which confirms that batching across keys is not necessarily wrong and the unlogged batching implementation is potentially incorrect. Here's the logged (atomic) batching logic:

    private void mutateManyLogged(final Map<String, Map<StaticBuffer, KCVMutation>> mutations, final StoreTransaction txh) throws BackendException {
        final MaskedTimestamp commitTime = new MaskedTimestamp(txh);

        final BatchStatement batchStatement = new BatchStatement(Type.LOGGED);
        batchStatement.setConsistencyLevel(getTransaction(txh).getWriteConsistencyLevel());

        batchStatement.addAll(Iterator.ofAll(mutations.entrySet()).flatMap(tableNameAndMutations -> {
            final String tableName = tableNameAndMutations.getKey();
            final Map<StaticBuffer, KCVMutation> tableMutations = tableNameAndMutations.getValue();

            final CQLKeyColumnValueStore columnValueStore = Option.of(this.openStores.get(tableName))
                    .getOrElseThrow(() -> new IllegalStateException("Store cannot be found: " + tableName));
            return Iterator.ofAll(tableMutations.entrySet()).flatMap(keyAndMutations -> {
                final StaticBuffer key = keyAndMutations.getKey();
                final KCVMutation keyMutations = keyAndMutations.getValue();

                final Iterator<Statement> deletions = Iterator.of(commitTime.getDeletionTime(this.times))
                        .flatMap(deleteTime -> Iterator.ofAll(keyMutations.getDeletions()).map(deletion -> columnValueStore.deleteColumn(key, deletion, deleteTime)));
                final Iterator<Statement> additions = Iterator.of(commitTime.getAdditionTime(this.times))
                        .flatMap(addTime -> Iterator.ofAll(keyMutations.getAdditions()).map(addition -> columnValueStore.insertColumn(key, addition, addTime)));

                return Iterator.concat(deletions, additions);
            });
        }));
        final Future<ResultSet> result = Future.fromJavaFuture(this.executorService, this.session.executeAsync(batchStatement));

        result.await();
        if (result.isFailure()) {
            throw EXCEPTION_MAPPER.apply(result.getCause().get());
        }
        sleepAfterWrite(txh, commitTime);
    }

Is there a reason why batching across keys are not done for non-atomic batches? I have a fix to enable non-atomic batching across keys. Confirming if that's safe to put in ...

li-boxuan commented 2 years ago

@baritra Sorry I forgot to reply. Yes, I think that design is intentional. Quoting from https://docs.datastax.com/en/cql-oss/3.x/cql/cql_reference/cqlBatch.html:

A batch can also target multiple partitions, when atomicity and isolation is required. Multi-partition batches may decrease throughput and increase latency. Only use a multi-partition batch when there is no other viable option, such as asynchronous statements.

Single partition batch operations are unlogged by default, and are the only unlogged batch operations recommended.

sumandas0 commented 1 year ago

In my case what is happening, I have a composite index comprised of uid and name. Now I'm encountering a few ghost vertices searchable via name and uid. I tried to rename and drop name property and was then was unable to find that vertex by name(Which I want to do, I don't want to find vertex by name). But when I'm searching it via uid and name both, it returns result. I think it is due to the composite index creating a problem here, tried to rename those properties and drop but no luck. Unable to make that vertex unsearchable via those properties for my use case. It would be better if that vertex could be removed but at least if I can make those unsearchable then also fine.

porunov commented 1 year ago

In my case what is happening, I have a composite index comprised of uid and name. Now I'm encountering a few ghost vertices searchable via name and uid. I tried to rename and drop name property and was then was unable to find that vertex by name(Which I want to do, I don't want to find vertex by name). But when I'm searching it via uid and name both, it returns result. I think it is due to the composite index creating a problem here, tried to rename those properties and drop but no luck. Unable to make that vertex unsearchable via those properties for my use case. It would be better if that vertex could be removed but at least if I can make those unsearchable then also fine.

You may want to use StaleIndexRecordUtil class which will be available in JanusGraph 1.0.0. You may also switch to unreleased version of JanusGraph (i.e. latest master branch) and use that tool. See example here: https://docs.janusgraph.org/master/advanced-topics/stale-index/

sumandas0 commented 1 year ago

@porunov we can't really switch into latest master as of now due to code constraint and we are using v0.5.

Is there any workaround to solve this issue via Gremlin shell?

https://github.com/JanusGraph/janusgraph/issues/1099#issuecomment-397262796 is not quite working for composite index.

porunov commented 1 year ago

@porunov we can't really switch into latest master as of now due to code constraint and we are using v0.5.

Is there any workaround to solve this issue via Gremlin shell?

#1099 (comment) is not quite working for composite index.

v0.5 is EOL. We don't plan to contribute bugfixes to that branch anymore. I don't know an easy solution to achieve this using 0.5.x versions of JanusGraph, but what you can try to consider is:

  1. Reingest your data into a new graph. Use full-scan during data retrieval from JanusGraph and upload that data into a new fresh graph with schema initialized (including your indices). This will automatically fix all stale indexes.
  2. Use ReflationAPI to implement necessary logic for stale index records removal. You can use StaleIndexRecordUtil.java for inspiration, but you will need to use ReflactionAPI because some of the methods were not available until the next PR: #3024
  3. No need to upgrade all you JanusGraph nodes to the latest version. You can spin up a single private JanusGraph instance of the newest version and use StaleIndexRecordUtil to resolve your stale index problems. After resolving it you may remove that node from the cluster.
sumandas0 commented 1 year ago

Can we implement this using 0.6 version. Is it possible to fix this bug in 0.6 without Re-ingesting.

porunov commented 1 year ago

Can we implement this using 0.6 version. Is it possible to fix this bug in 0.6 without Re-ingesting.

I think it's possible but for this to happen you will need to backport refactoring commit as well as two commits which implement this util.

sumandas0 commented 1 year ago

Can you provide me the PRs and commit which I needed to merge to get this function up and working. Thanks in Advance.

porunov commented 1 year ago

Can you provide me the PRs and commit which I needed to merge to get this function up and working. Thanks in Advance.

Here are the commits ordered by how they were lending in the master branch:

  1. Refactoring PR: #3024 (commit)
  2. Functionality for composite indices: #3022 (commit)
  3. Functionality for mixed indices: #3327 (commit)
shivamc7y commented 11 months ago

We are also facing the stale index issue with our janusgraph cluster having bigtable backend.The main issue which we are facing is to find out the all the stale indexes so we can remove that. We identified this issue recently so we have the vertex ids where we are getting this issue but due to log rotation we are uncertain about the occurrence of the issue before that. @porunov is there a way we can scan the index and figure out what are all the stale indices.