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-14008][registry] cache keep alive lease #14034

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Conversation

eye-gu
Copy link
Contributor

@eye-gu eye-gu commented Apr 27, 2023

Purpose of the pull request

fix #14008

Brief change log

add EtcdKeepAliveLeaseManager

Verify this pull request

This pull request is code cleanup without any test coverage.

@SbloodyS SbloodyS added bug Something isn't working backend 3.2.0 for 3.2.0 version labels Apr 28, 2023
@SbloodyS SbloodyS added this to the 3.2.0 milestone Apr 28, 2023
@eye-gu eye-gu requested a review from SbloodyS April 28, 2023 14:29
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2023

Codecov Report

Merging #14034 (e59e0f1) into dev (9ce8871) will increase coverage by 0.02%.
The diff coverage is 64.00%.

❗ Current head e59e0f1 differs from pull request most recent head 9e12842. Consider uploading reports for the commit 9e12842 to get more accurate results

@@             Coverage Diff              @@
##                dev   #14034      +/-   ##
============================================
+ Coverage     38.62%   38.65%   +0.02%     
- Complexity     4613     4619       +6     
============================================
  Files          1283     1284       +1     
  Lines         43855    43877      +22     
  Branches       4846     4846              
============================================
+ Hits          16940    16961      +21     
+ Misses        25038    25037       -1     
- Partials       1877     1879       +2     
Files Changed Coverage Δ
...inscheduler/plugin/registry/etcd/EtcdRegistry.java 48.07% <50.00%> (+0.94%) ⬆️
...lugin/registry/etcd/EtcdKeepAliveLeaseManager.java 65.21% <65.21%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonarcloud
Copy link

sonarcloud bot commented May 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

64.0% 64.0% Coverage
0.0% 0.0% Duplication

@SbloodyS
Copy link
Member

PTAL @ruanwenjun

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM.
Since each put operation will create a new leaseId, and we will schedule heartbeat task to execute put to write heartbeat info, so this will cause memory leak.

This PR will hold the mapping of key to leaseId.

@sonarcloud
Copy link

sonarcloud bot commented Jul 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

64.0% 64.0% Coverage
0.0% 0.0% Duplication

@ruanwenjun ruanwenjun merged commit d87a0d8 into apache:dev Jul 28, 2023
50 of 52 checks passed
zhuangchong pushed a commit to zhuangchong/incubator-dolphinscheduler that referenced this pull request Jul 31, 2023
zhuangchong added a commit that referenced this pull request Jul 31, 2023
biaoma-ty pushed a commit to Kasma-Inc/dolphinscheduler that referenced this pull request Aug 17, 2023
zhongjiajie pushed a commit that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.2.0 for 3.2.0 version backend bug Something isn't working ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [registry] use etcd lease memory leak
5 participants