Skip to content

fix(server): avoid duplicate registration of StoreEventListener #2620

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.

Sign up for GitHub

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

Closed
haohao0103 wants to merge 12 commits into from
Closed

fix(server): avoid duplicate registration of StoreEventListener #2620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
08a6052
#2583
haohao0103 Jul 15, 2024
e3377df
close #2583
haohao0103 Jul 16, 2024
be42cd3
Merge branch 'refs/heads/master' into alan-master-dev1
haohao0103 Aug 6, 2024
11b5be2
close #2618
haohao0103 Aug 6, 2024
9a97f90
Merge branch 'master' into alan-master-dev1
imbajin Aug 10, 2024
8c32404
close #2618
haohao0103 Aug 12, 2024
889884f
Merge branch 'refs/heads/master' into alan-master-dev1
haohao0103 Aug 12, 2024
0b4725f
close #2618
haohao0103 Aug 13, 2024
ceba46c
close #2618
haohao0103 Aug 13, 2024
b1ec955
Update hugegraph-server/hugegraph-core/src/main/java/org/apache/hugeg…
haohao0103 Aug 15, 2024
c70e65a
Update hugegraph-server/hugegraph-core/src/main/java/org/apache/hugeg…
haohao0103 Apr 1, 2025
17d3f08
Update hugegraph-server/hugegraph-core/src/main/java/org/apache/hugeg…
haohao0103 Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...gegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
}
return false;
};
this.store().provider().listen(this.storeEventListener);
this.store().provider().listenDataCacheClear(this.storeEventListener);

Check warning on line 136 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java#L136 

Added line #L136 was not covered by tests
Copy link
Contributor

@javeme javeme Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I have read the issue, but I still don't quite understand in what scenarios.the problem is encountered.

Copy link
Contributor Author

@haohao0103 haohao0103 Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simply put,i found that registering a listener for each transaction instance is redundant because each transaction receives events and then performs a clear operation on the schema/vertex/edge cache , i think that this is redundant.

Copy link
Contributor

@javeme javeme Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note each provider has multiple listeners, which is our design intention, because each listener wants to get notifications.
This modification looks a bit special at the moment, and we will propose a solution after we figure out the problem. Could you post the error stack if there is an error?


// Listen cache event: "cache"(invalid cache item)
this.cacheEventListener = event -> {
Expand Down
3 changes: 2 additions & 1 deletion ...raph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransactionV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@
}
return false;
};
this.graphParams().loadGraphStore().provider().listen(this.storeEventListener);
this.graphParams().loadGraphStore().provider()
.listenSchemaCacheClear(this.storeEventListener);

Check warning on line 109 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransactionV2.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransactionV2.java#L108-L109 

Added lines #L108 - L109 were not covered by tests

// Listen cache event: "cache"(invalid cache item)
this.cacheEventListener = event -> {
Copy link
Contributor

@javeme javeme Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imbajin can you share some context why we added CachedSchemaTransactionV2.java? not sure it's used in what scenarios, and can we merge V1+V2 into one class?

Copy link
Contributor

@VGalaxies VGalaxies Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CachedSchemaTransactionV2 is the CachedSchemaTransaction corresponding to the SchemaTransaction of the pd-store version. We added a V2 to distinguish it from the previous version. Due to the inconsistency of the parameters required for construction, it is currently not easy to reuse, will be merged later...

Expand Down
26 changes: 26 additions & 0 deletions ...h-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
private final EventHub storeEventHub = new EventHub("store");

protected Map<String, BackendStore> stores = null;
private static volatile boolean schemaCacheClearListened = false;
private static volatile boolean vertexEdgeCacheClearListened = false;

Check warning on line 48 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L47-L48 

Added lines #L47 - L48 were not covered by tests

protected final void notifyAndWaitEvent(String event) {
Future<?> future = this.storeEventHub.notify(event, this);
Expand All @@ -70,6 +72,30 @@
this.storeEventHub.listen(EventHub.ANY_EVENT, listener);
}

@Override
public void listenSchemaCacheClear(EventListener listener) {
if (!schemaCacheClearListened) {
synchronized (AbstractBackendStoreProvider.class) {

Check warning on line 78 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L78 

Added line #L78 was not covered by tests
if (!schemaCacheClearListened) {
listen(listener);
schemaCacheClearListened = true;

Check warning on line 81 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L80-L81 

Added lines #L80 - L81 were not covered by tests
}
}

Check warning on line 83 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L83 

Added line #L83 was not covered by tests
}
}

Check warning on line 85 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L85 

Added line #L85 was not covered by tests

@Override
public void listenDataCacheClear(EventListener listener) {
if (!vertexEdgeCacheClearListened) {
synchronized (AbstractBackendStoreProvider.class) {

Check warning on line 90 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L90 

Added line #L90 was not covered by tests
if (!vertexEdgeCacheClearListened) {
listen(listener);
vertexEdgeCacheClearListened = true;

Check warning on line 93 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L92-L93 

Added lines #L92 - L93 were not covered by tests
}
}

Check warning on line 95 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L95 

Added line #L95 was not covered by tests
}
}

Check warning on line 97 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java#L97 

Added line #L97 was not covered by tests

@Override
public void unlisten(EventListener listener) {
this.storeEventHub.unlisten(EventHub.ANY_EVENT, listener);
Expand Down
6 changes: 6 additions & 0 deletions ...hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendStoreProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@

void listen(EventListener listener);

default void listenSchemaCacheClear(EventListener listener) {
Copy link
Contributor

@javeme javeme Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer listenSchemaCacheAtMostOnce

Copy link
Contributor Author

@haohao0103 haohao0103 Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks , you mean to update the method name to listenSchemaCacheAtMostOnce?

}

Check warning on line 71 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendStoreProvider.java#L71 

Added line #L71 was not covered by tests

default void listenDataCacheClear(EventListener listener) {
Copy link
Contributor

@javeme javeme Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer listenGraphCacheAtMostOnce

}

Check warning on line 74 in hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendStoreProvider.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendStoreProvider.java#L74 

Added line #L74 was not covered by tests

void unlisten(EventListener listener);

EventHub storeEventHub();
Expand Down
Loading