Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FileGraphLibrary no longer works with FederatedStore #3129

Closed
t92549 opened this issue Dec 4, 2023 · 4 comments · Fixed by #3140
Closed

FileGraphLibrary no longer works with FederatedStore #3129

t92549 opened this issue Dec 4, 2023 · 4 comments · Fixed by #3140
Assignees
Labels
bug Confirmed or suspected bug
Milestone

Comments

@t92549
Copy link
Contributor

t92549 commented Dec 4, 2023

Describe the bug
You can no longer use a FileGraphLibrary with a FederatedStore. If you add a graph and restart the store, it will crash.

This may be due to changes with how getSchema works. When the FederatedStore is started, a file in the library is made with its empty schema. When a graph is added, the getSchema changes now mean that it will return the merged schema. When the FederatedStore is restarted, this schema now conflicts with the empty schema file, causing a crash.

To Reproduce
Steps to reproduce the behaviour:

  1. Create a FederatedStore that uses FileGraphLibrary and a persistent cache
  2. Add a graph to it
  3. Turn off the FederatedStore
  4. Turn it back on again, it will crash on startup

Expected behaviour
This behaviour should be possible and worked in Gaffer 1.

Additional context
I think this problem not being caught indicates we need better tests for turning the FederatedStore on and off, and how this interacts with persistent caches and the FileGraphLibrary.

Also, why isn't there a way to use the cache for the graph library? A file does not seem like the best way top store the graph library.

@t92549 t92549 added the bug Confirmed or suspected bug label Dec 4, 2023
@GCHQDeveloper314
Copy link
Member

I've had a look at this and can reproduce it with a simple test within Gaffer which attempts to create a second store instead of restarting. Debugging reveals this is indeed caused by the getSchema changes.

When creating a new Graph with the builder, there's a call on line 841 which leads to line 997; where if the schema is null or empty it is replaced with a schema fetched from the store with getSchema(). This is the cause of the problem in this issue, because for the Federated Store the schema returned is no longer an empty schema. The builder then calls the GraphLibrary checkExisting method which compares the blank schema from the FileGraphLibrary file with the schema returned by the Federated Store (which is not blank), this causes the below exception:

uk.gov.gchq.gaffer.commonutil.exception.OverwritingException: GraphId federatedGraph already exists with a different schema:
existing schema:
{"types":{}}
new schema:
{....

This could be fixed in various ways.

  • A change relating to line 977: This check is not ideal for the Federated Store, because setting the internal schema field isn't useful for Stores which don't have a schema of their own. On the other hand, it's harmless. To change this, Graph would need to become aware of Stores which don't have a schema of their own, or it could call a different method in Store which could be overridden to return empty for schemaless Stores.
  • A change to GraphLibrary method at line 273: This is where the exception comes from. It could be changed to have awareness of schemaless Stores and handle them differently. This could work for a quick fix but it's probably better to make a change to Graph or Store instead.

Overall, I'd recommend refactoring Store to better accommodate schemaless stores and then make a small change to Graph to avoid the problem on line 977. E.g. adding a flag to indicate if a store used internal schema, this could then be checked by Graph on 977.

@t92549
Copy link
Contributor Author

t92549 commented Dec 18, 2023

@GCHQDeveloper314

if (schema == null || schema.getGroups().isEmpty()) {

Could this be changed to getSchema() == null?

@GCHQDeveloper314
Copy link
Member

Could this be changed to getSchema() == null?

No, it is used to set the schema if none was supplied and that call uses getSchema(). This change would mean always setting schema to null only if the store had a null schema which doesn't make sense to me. If you mean just changing the second part, then I would question why we are looking at the groups at all. It was introduced in this PR and feels like an afterthought. If you wanted to use the schema from the store you wouldn't pass in an empty schema when you could pass in null or use a method that doesn't ask for a schema at all.

@GCHQDeveloper314
Copy link
Member

The fix for this issue was to only get the Schema from the Store if it is null. Previously this would also be done if it were empty, this is a problem for the Federated Store because it has a dynamic schema which starts empty and then changes to include graphs added to the store.

The crash happened because when the store was restarted the schema from the FileGraphLibrary schema file (empty) would be compared with a schema from the store (not empty) and this would cause a conflict when checked. This check doesn't make sense for Federated Stores because it is normal for their schemas to change and not an error.

A workaround was to delete the schema file created by FileGraphLibrary before restarting (which recreates the Federated Store), this ensured the failing check passed as there was then no existing empty schema to compare against. This means the FileGraphLibrary schema file will then contain the Federated Store's schema as of the time of restarting (a non-empty schema if graphs had been added previously). As this file would no longer be an empty schema further restarts would work correctly as the failing check only occurred if the file did not exist or had an empty schema.

As Federated Stores do not have schemas in the usual sense, it doesn't make sense to be checking or fetching them at all. To better accommodate this in future, I would suggest changing Graph.java/Store.java/GraphLibrary to better manage Stores which don't have a schema of their own. For example by adding a field which flags this and allows various schema checks to be skipped for these stores.

tb06904 added a commit that referenced this issue Jan 24, 2024
GCHQDeveloper314 added a commit that referenced this issue Feb 1, 2024
* Only use Store's schema if supplied schema is null
The supplied schema will be empty for schemaless stores

* Remove unnecessary Mockito stubbings
Various stubbings now unnecessary. These were a consequence of taking schema from the Store when empty.

* Remove Empty Schema specific test
Test no longer required as schema will no longer be fetched from store in this situation

* Remove redundant graphId check, add test for this and improve doc
This graph id null check is never used because this is already checked when initialising the store

* Remove redundant code for setting NoGraphLibrary
This exact code is run inside GraphConfig and is not required in Graph.java

* Fix FederatedStore getSchema method Javadoc
This incorrectly stated that an empty scheam would always be returned, this will usually be the case but not if a default context/user is valid for a graph

* Add IT to test restarting a Federated Store using a FileGraphLibrary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed or suspected bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants