gh-2457 double caching issue #2595

GCHQDev404 · 2022-03-02T20:39:58Z

Related Issue

Resolve FederatedGraphStore double-caching causes desync issues in replicated deployment #2457

…ers & Tests

…TION_OPTIONS_GRAPH_IDS.

…ionChain

…ng & Comments

…equired tests.

…aining MIDPUT generic

…federatedstore-federated-operation-remove-midput # Conflicts: # store-implementation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/operation/FederatedOperation.java

… to IFederation

…nges.

…e changes.

…ingAdminUsage changes.

…ingAdminUsage rename.

… + Test

…ching-issue

…hanging GraphSerialisable causes backwards compatability issues.

…ching-issue

GCHQDeveloper314

I can see quite a few comments of TODO FS. Could it be clarified if this PR is ready for merge? Some of the comment do look like they can be addressed later.
Instead of leaving these comments in the code, a follow up issue could be raised - especially for the comments in the tests.

GCHQDeveloper314 · 2022-10-31T09:36:28Z

...n/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/FederatedGraphStorage.java

            } catch (final Exception e) {
-                throw new StorageException("Error adding graph " + graphId + " to storage due to: " + e.getMessage(), e);
+                throw new StorageException("Error adding graph " + graphId + (nonNull(e.getMessage()) ? (" to storage due to: " + e.getMessage()) : "."), e);


Using String.Format would be better than concatenation.

...ion/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/FederatedStoreCache.java

GCHQDev404 · 2022-10-31T10:36:05Z

I can see quite a few comments of TODO FS. Could it be clarified if this PR is ready for merge? Some of the comment do look like they can be addressed later.
Instead of leaving these comments in the code, a follow up issue could be raised - especially for the comments in the tests.

These are left in for when I'm showing team the code.
It's so people know what I'm thinking and where I'm thinking it, before a ticket is made.
Most of these TODOs will be addressed during Alpha stages.
The comments do not affect reviewing of code.

GCHQDeveloper314

No comments requiring changes, therefore approved.

t92549 · 2022-10-31T10:45:15Z

...n/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/FederatedGraphStorage.java

    public static final boolean DEFAULT_DISABLED_BY_DEFAULT = false;
    public static final String ERROR_ADDING_GRAPH_TO_CACHE = "Error adding graph, GraphId is known within the cache, but %s is different. GraphId: %s";
    public static final String USER_IS_ATTEMPTING_TO_OVERWRITE = "User is attempting to overwrite a graph within FederatedStore. GraphId: %s";
    public static final String ACCESS_IS_NULL = "Can not put graph into storage without a FederatedAccess key.";
    public static final String GRAPH_IDS_NOT_VISIBLE = "The following graphIds are not visible or do not exist: %s";
-    private final Map<FederatedAccess, Set<Graph>> storage = new HashMap<>();


Have there been any tests to see how much removing the local cache impacts performance?

none.
but the code was written so that the graph creation from GraphSerialisable happens in the last moment to where an operation actually requires execution.
Its possible that a transient Graph object could be placed in the GraphSerialisable but a code review would be required to work out how many times a GraphSerialisable object gets re-used.

gh-2457-double-caching-issue

GCHQDev404 and others added 30 commits February 5, 2021 07:26

gh-2357 FederatedStore FederatedOperation Operation/Graph/Merge

d723c86

gh-2357 FederatedStore FederatedOperation Handlers

a5ca20d

gh-2357 FederatedStore FederatedOperation IFederationOperation, Handl…

af378dc

…ers & Tests

gh-2357 FederatedStore FederatedOperation FederatedStore

f479fee

gh-2357 FederatedStore FederatedOperation removal of option KEY_OPERA…

0d55966

…TION_OPTIONS_GRAPH_IDS.

gh-2357 FederatedStore FederatedOperation GetSchema & GetTraits Changes

72f91ee

gh-2357 FederatedStore FederatedOperation IFederatedOperation

a880437

gh-2357 FederatedStore FederatedOperation deletion of FederatedOperat…

d3ef8d0

…ionChain

gh-2357 FederatedStore FederatedOperation viewValidation

a126c21

gh-2357 FederatedStore FederatedOperation Merging of results

1d1e980

gh-2357 FederatedStore FederatedOperation Tests, CodeStyle, Refactori…

f7c1a29

…ng & Comments

gh-2357 FederatedStore FederatedOperation.v02 Builder IO focus

5816568

gh-2357 FederatedStore FederatedOperation.v02. Removing suspected unr…

45b2869

…equired tests.

gh-2357 FederatedStore FederatedOperation.v02. Resolving TODOs

14befc0

gh-2357 FederatedStore FederatedOperation.v02. Removing overly constr…

3cecb28

…aining MIDPUT generic

Merge branch 'gh-2357-federatedstore-federated-operation' into gh-2357-…

d713f6d

…federatedstore-federated-operation-remove-midput # Conflicts: # store-implementation/federated-store/src/main/java/uk/gov/gchq/gaffer/federatedstore/operation/FederatedOperation.java

gh-2357 FederatedStore FederatedOperation.v02. Resolving TODOs

629b17c

gh-2357 FederatedStore FederatedOperation.v02. checkstyle

45d0b25

gh-2357 FederatedStore FederatedOperation.v02. Resolving TODOs.

9d0faeb

gh-2357 FederatedStore FederatedOperation.v02 with errors

467fca5

gh-2357 FederatedStore FederatedOperation.v02 isRequestingAdmin added…

d8788a2

… to IFederation

gh-2357 FederatedStore FederatedOperation.v02 IFederation builder cha…

cb28d68

…nges.

gh-2357 FederatedStore FederatedOperation.v02 IFederation shallowClon…

d44d171

…e changes.

gh-2357 FederatedStore FederatedOperation.v02 IFederation userRequest…

0839f87

…ingAdminUsage changes.

gh-2357 FederatedStore FederatedOperation.v02 IFederation userRequest…

bcde199

…ingAdminUsage rename.

gh-2357 FederatedStore FederatedOperation.v02 bugfix.

b514e1a

gh-2357 FederatedStore FederatedOperation.v02 removing TODOs.

83b92ef

gh-2422 FederatedStore ChangeGraphId & ChangeGraphAccess update cache…

15998b0

… + Test

gh-2357 FederatedStore FederatedOperation.v02 removing TODOs.

5cff563

gh-2357 FederatedStore FederatedOperation.v02 removing TODOs. BugHunt

df74b9c

Merge remote-tracking branch 'origin/v2-alpha' into gh-2457-double-ca…

d61eb7e

…ching-issue

t92549 linked an issue Oct 26, 2022 that may be closed by this pull request

FederatedGraphStore double-caching causes desync issues in replicated deployment #2457

Closed

GCHQDev404 added the federated-store Specific to/touches the federated-store module label Oct 27, 2022

GCHQDev404 added this to the v2.0.0-alpha-0.4 milestone Oct 27, 2022

GCHQDev404 added 6 commits October 27, 2022 21:32

Merge remote-tracking branch 'origin/v2-alpha' into gh-2457-double-ca…

991f113

…ching-issue

gh-2457 GraphSerialisable not being able to Mock has failing tests. c…

d5326d8

…hanging GraphSerialisable causes backwards compatability issues.

gh-2447 Fixed GraphSerialisable equals.

ecfeffe

gh-2447 need to ignore "final class" checkstyle

720603a

gh-2457 checkstyle

7eec033

Merge remote-tracking branch 'origin/v2-alpha' into gh-2457-double-ca…

6bb3655

…ching-issue

GCHQDeveloper314 reviewed Oct 31, 2022

View reviewed changes

GCHQDeveloper314 previously approved these changes Oct 31, 2022

View reviewed changes

t92549 reviewed Oct 31, 2022

View reviewed changes

gh-2457 PR requests.

2d9efa7

GCHQDev404 dismissed GCHQDeveloper314’s stale review via 2d9efa7 October 31, 2022 10:45

t92549 previously approved these changes Oct 31, 2022

View reviewed changes

GCHQDeveloper314 removed this from the v2.0.0-alpha-0.4 milestone Oct 31, 2022

GCHQDev404 added 3 commits October 31, 2022 17:28

gh-2457 PR requests.

543378e

Merge remote-tracking branch 'origin/gh-2457-double-caching-issue' into

c51cb9c

gh-2457-double-caching-issue

gh-2457 PR requests.

14f39de

GCHQDev404 dismissed t92549’s stale review via 14f39de October 31, 2022 17:33

t92549 approved these changes Nov 1, 2022

View reviewed changes

GCHQDeveloper314 approved these changes Nov 2, 2022

View reviewed changes

lb324567 self-requested a review November 2, 2022 11:46

lb324567 approved these changes Nov 2, 2022

View reviewed changes

GCHQDev404 merged commit 3ee6a5f into v2-alpha Nov 2, 2022

n3101 deleted the gh-2457-double-caching-issue branch November 2, 2022 13:13

n3101 mentioned this pull request Nov 2, 2022

FederatedGraphStore double-caching causes desync issues in replicated deployment #2457

Closed

GCHQDev404 mentioned this pull request Feb 16, 2023

Cache Service Static Instance Bug #2890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-2457 double caching issue #2595

gh-2457 double caching issue #2595

GCHQDev404 commented Mar 2, 2022 •

edited by github-actions bot

GCHQDeveloper314 left a comment

GCHQDeveloper314 Oct 31, 2022

GCHQDev404 commented Oct 31, 2022

GCHQDeveloper314 left a comment

t92549 Oct 31, 2022

GCHQDev404 Nov 1, 2022

gh-2457 double caching issue #2595

gh-2457 double caching issue #2595

Conversation

GCHQDev404 commented Mar 2, 2022 • edited by github-actions bot

Related Issue

GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

GCHQDeveloper314 Oct 31, 2022

Choose a reason for hiding this comment

GCHQDev404 commented Oct 31, 2022

GCHQDeveloper314 left a comment

Choose a reason for hiding this comment

t92549 Oct 31, 2022

Choose a reason for hiding this comment

GCHQDev404 Nov 1, 2022

Choose a reason for hiding this comment

GCHQDev404 commented Mar 2, 2022 •

edited by github-actions bot