fix(server) Avoid duplicate registration of StoreEventListener #2620

haohao0103 · 2024-08-06T05:42:36Z

Define initialization identifiers for storeEventListeners in AbstractBackendStoreProvider to ensure that storeEventListeners are not registered repeatedly

support disable rocksdb auto compaction through configuration

imbajin · 2024-08-10T03:17:46Z

...raph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

+        if (schemaCacheClearListened.compareAndSet(false, true) ||
+            vertexEdgeCacheClearListened.compareAndSet(false, true)) {


Some questions:

we define 2 automic_param boolean A & B and try to set them to true, when to reset it?

why we need 2 flag to mark it? (in this context)

since assignment operations (=) for primitive types are atomic by default(64bit), it seems that we only need to use a volatile boolean flag to achieve the desired effect?

@Pengzna @VGalaxies

1、define 2 automic_param boolean A & B;this operation is to ensure that storeEventListener does not deliver to storeEventHub of storeProvider once in each thread. I understand that there is no need to re register during the entire Hugegraph lifecycle, so there is no reset operation; Will this miss other necessary scenarios?
2、schemaCacheClearListened for schemaCache clear , vertexEdgeCacheClearListened for vertex&edge cache;
3、 I have modified it to use the volatile+synchronized，thanks

...raph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

codecov · 2024-08-13T09:02:20Z

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 38.93%. Comparing base (a038d23) to head (ceba46c).

Files	Patch %	Lines
...ph/backend/store/AbstractBackendStoreProvider.java	87.50%	0 Missing and 2 partials ⚠️
.../hugegraph/backend/store/BackendStoreProvider.java	0.00%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (a038d23) and HEAD (ceba46c). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (a038d23) HEAD (ceba46c)

5 3

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2620      +/-   ##
============================================
- Coverage     45.17%   38.93%   -6.24%     
- Complexity      583      759     +176     
============================================
  Files           718      729      +11     
  Lines         58434    59520    +1086     
  Branches       7491     7660     +169     
============================================
- Hits          26396    23177    -3219     
- Misses        29318    33870    +4552     
+ Partials       2720     2473     -247

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

...raph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java

…raph/backend/store/AbstractBackendStoreProvider.java Co-authored-by: imbajin <[email protected]>

zyxxoo · 2024-08-15T13:37:41Z

thanks for you push code for solve the problem, this is a great code, but i have some different advise for that. ohh, the transaction monitor some event happen, meawhile, the code change the state local, every instance maybe need care the event, so we should allow repeated event register; for the problem, i think the main problem maybe is the threadlocal not correct release, that make the listen not be remove

javeme

Thanks for your contribution, some tiny comments

javeme · 2024-08-17T03:31:59Z

...gegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransactionV2.java

@@ -105,7 +105,8 @@ private void listenChanges() {
            }
            return false;
        };
-        this.graphParams().loadGraphStore().provider().listen(this.storeEventListener);
+        this.graphParams().loadGraphStore().provider()
+            .listenSchemaCacheClear(this.storeEventListener);

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


@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?

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...

javeme · 2024-08-17T04:18:04Z

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

@@ -133,7 +133,7 @@ private void listenChanges() {
            }
            return false;
        };
-        this.store().provider().listen(this.storeEventListener);
+        this.store().provider().listenDataCacheClear(this.storeEventListener);


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

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.

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?

javeme · 2024-08-17T04:21:06Z

...er/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendStoreProvider.java

@@ -67,6 +67,12 @@ public interface BackendStoreProvider {

    void listen(EventListener listener);

+    default void listenSchemaCacheClear(EventListener listener) {


prefer listenSchemaCacheAtMostOnce

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

javeme · 2024-08-17T04:21:20Z

...er/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/BackendStoreProvider.java

+    default void listenSchemaCacheClear(EventListener listener) {
+    }
+
+    default void listenDataCacheClear(EventListener listener) {


prefer listenGraphCacheAtMostOnce

haohao0103 · 2024-08-19T03:02:19Z

thanks for you push code for solve the problem, this is a great code, but i have some different advise for that. ohh, the transaction monitor some event happen, meawhile, the code change the state local, every instance maybe need care the event, so we should allow repeated event register; for the problem, i think the main problem maybe is the threadlocal not correct release, that make the listen not be remove

thanks , i try to understand what you mean , Indeed, there some issues with threadlocal processing,and my code did not address this problem. In addition ,I think it is indeed valuable to pay attention to events for each transaction instance; But in the current scenario,based on my understanding,this operation seems redundant because schema and vertex/edge cache are both global singleton, and it seems unnecessay to execute their operations on every transaction instance. of course ,if there are personalized scenarios that allow for reregistration of listener,it is indeed necessary and valuable.this is my idea, welcome to correct it.

github-actions · 2024-09-23T21:03:57Z

Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label

haohao0103 added 4 commits July 15, 2024 11:05

apache#2583

08a6052

support disable rocksdb auto compaction through configuration

close apache#2583

e3377df

Merge branch 'refs/heads/master' into alan-master-dev1

be42cd3

close apache#2618

11b5be2

dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. store Store module labels Aug 6, 2024

imbajin reviewed Aug 10, 2024

View reviewed changes

imbajin and others added 3 commits August 10, 2024 11:19

Merge branch 'master' into alan-master-dev1

9a97f90

close apache#2618

8c32404

Merge branch 'refs/heads/master' into alan-master-dev1

889884f

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Aug 12, 2024

close apache#2618

0b4725f

imbajin reviewed Aug 13, 2024

View reviewed changes

...raph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java Outdated Show resolved Hide resolved

close apache#2618

ceba46c

imbajin reviewed Aug 14, 2024

View reviewed changes

...raph-core/src/main/java/org/apache/hugegraph/backend/store/AbstractBackendStoreProvider.java Outdated Show resolved Hide resolved

Update hugegraph-server/hugegraph-core/src/main/java/org/apache/hugeg…

b1ec955

…raph/backend/store/AbstractBackendStoreProvider.java Co-authored-by: imbajin <[email protected]>

imbajin requested review from javeme and zyxxoo August 15, 2024 10:01

javeme reviewed Aug 17, 2024

View reviewed changes

github-actions bot added the inactive label Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server) Avoid duplicate registration of StoreEventListener #2620

fix(server) Avoid duplicate registration of StoreEventListener #2620

haohao0103 commented Aug 6, 2024

imbajin Aug 10, 2024 •

edited

Loading

haohao0103 Aug 12, 2024

codecov bot commented Aug 13, 2024 •

edited

Loading

zyxxoo commented Aug 15, 2024

javeme left a comment

javeme Aug 17, 2024

VGalaxies Aug 18, 2024

javeme Aug 17, 2024

haohao0103 Aug 19, 2024

javeme Aug 24, 2024

javeme Aug 17, 2024

haohao0103 Aug 19, 2024

javeme Aug 17, 2024

haohao0103 commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Sep 23, 2024

		if (schemaCacheClearListened.compareAndSet(false, true) \|\|
		vertexEdgeCacheClearListened.compareAndSet(false, true)) {

		@@ -67,6 +67,12 @@ public interface BackendStoreProvider {

		void listen(EventListener listener);

		default void listenSchemaCacheClear(EventListener listener) {

fix(server) Avoid duplicate registration of StoreEventListener #2620

Are you sure you want to change the base?

fix(server) Avoid duplicate registration of StoreEventListener #2620

Conversation

haohao0103 commented Aug 6, 2024

imbajin Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

zyxxoo commented Aug 15, 2024

javeme 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

haohao0103 commented Aug 19, 2024 • edited Loading

github-actions bot commented Sep 23, 2024

imbajin Aug 10, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

haohao0103 commented Aug 19, 2024 •

edited

Loading