Skip to content

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

@sonarqubecloud
Copy link

sonarqubecloud 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

@eye-gu eye-gu force-pushed the Fix-14008 branch 3 times, most recently from 4750b7a to f5a8413 Compare July 20, 2023 11:18
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.

@sonarqubecloud
Copy link

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