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

fix(server) Fix cacheEventListener in CachedSystemTransactionV2 #2619

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

haohao0103
Copy link
Contributor

close #2617

Use the listening mechanism of server ->pd to clear the local schema cache upon receiving schema change events from pd;

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. pd PD module labels Aug 6, 2024
@VGalaxies VGalaxies self-requested a review August 6, 2024 04:47
@haohao0103 haohao0103 changed the title fix[server] Fix cacheEventListener in CachedSystemTransactionV2 fix(server) Fix cacheEventListener in CachedSystemTransactionV2 Aug 6, 2024
Copy link

codecov bot commented Aug 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 35.99%. Comparing base (ba5b89e) to head (ffd4d16).
Report is 232 commits behind head on master.

Files with missing lines Patch % Lines
...in/java/org/apache/hugegraph/meta/MetaManager.java 0.00% 11 Missing ⚠️
...graph/backend/cache/CachedSchemaTransactionV2.java 0.00% 4 Missing ⚠️

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

HEAD has 2 uploads less than BASE
Flag BASE (ba5b89e) HEAD (ffd4d16)
5 3
Additional details and impacted files
@@              Coverage Diff              @@
##             master    #2619       +/-   ##
=============================================
- Coverage     46.84%   35.99%   -10.86%     
+ Complexity      343      302       -41     
=============================================
  Files           719      719               
  Lines         58465    58450       -15     
  Branches       7495     7491        -4     
=============================================
- Hits          27387    21037     -6350     
- Misses        28306    35087     +6781     
+ Partials       2772     2326      -446     

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

Comment on lines -145 to -147
if (!schemaEventHub.containsListener(Events.CACHE)) {
schemaEventHub.listen(Events.CACHE, this.cacheEventListener);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting these codes will cause the test to report the error: Not found listener for cache...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, i can't find the specific error information. could you tell me how to locate wehre the error occurred ?

Copy link
Contributor

Choose a reason for hiding this comment

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

For detailed information, see the failed test case here 👉 https://github.com/apache/incubator-hugegraph/actions/runs/10431704040/job/28891653277?pr=2619

image

schemaEventHub.listen(Events.CACHE, this.cacheEventListener);
}
// Listen cache event: "cache.clear", ...
MetaManager.instance().listenSchemaCacheClear((e) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

removing this.cacheEventListener will cause resource leak when unlistenChanges()

@@ -242,7 +251,19 @@ public <T> void listenGraphClear(Consumer<T> consumer) {
}

public <T> void listenSchemaCacheClear(Consumer<T> consumer) {
this.graphMetaManager.listenSchemaCacheClear(consumer);
if (isListenerInitialized(SCHEMA_CLEAR_KEY)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why registering listener when isListenerInitialized(), do you mean isListenerNotInitialized()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes,it is isListenerNotInitialized()

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive pd PD module size:M This PR changes 30-99 lines, ignoring generated files.
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

[Bug] cacheEventListener in CachedSchemaTransactionV2 Work does not meet expectations
3 participants