Skip to content

[KYUUBI #6320] Fix terminated application pods not deleted issue when kyuubi server restarted #6319

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[KYUUBI #6320] Fix terminated application pods not deleted issue when kyuubi server restarted #6319

wants to merge 1 commit into from

Conversation

turboFei
Copy link
Member

@turboFei turboFei commented Apr 18, 2024 •
edited
Loading

🔍 Description

Issue References 🔗

This pull request fixes #

We found that some pods were not deleted if kyuubi server restarted

The root cause is that:
com.google.common.cache::cleanup will not trigger removalListener action, we shall use invalidateAll instead.

Testing:
Nothing print for below code:

import com.google.common.cache.{Cache, CacheBuilder, RemovalNotification}

var removed = 0
val cache: Cache[String, String] = CacheBuilder.newBuilder()
  .removalListener((notification: RemovalNotification[String, String]) => {
    removed += 1
    println("removed: " + removed)
  }).build()

(0 until 1000).foreach { i =>
  cache.put(i.toString, i.toString)
}

cache.cleanup()

Replacing cache.cleanup() with cache.invalidateAll(), the output is expected.

Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

Be nice. Be informative.

@turboFei turboFei self-assigned this Apr 18, 2024
@turboFei turboFei requested a review from pan3793 April 18, 2024 22:28
@turboFei turboFei changed the title [KUBERNETES] Invalidate all cached terminated application info to trigger pods deletion instead of cleanup [KUBERNETES] Fix terminated application pods not deleted issue when kyuubi server restarted Apr 18, 2024
@pan3793 pan3793 added this to the v1.8.2 milestone Apr 19, 2024
@pan3793 pan3793 mentioned this pull request Apr 19, 2024
Closed
3 tasks
@pan3793 pan3793 changed the title [KUBERNETES] Fix terminated application pods not deleted issue when kyuubi server restarted [KYUUBI #6320] Fix terminated application pods not deleted issue when kyuubi server restarted Apr 19, 2024
@pan3793 pan3793 closed this in 26fe59d Apr 19, 2024
pan3793 pushed a commit that referenced this pull request Apr 19, 2024
@turboFei @pan3793
[KYUUBI #6320] Fix terminated application pods not deleted issue when…
9fe210c 
… kyuubi server restarted

# 🔍 Description
## Issue References 🔗

This pull request fixes #

We found that some pods were not deleted if kyuubi server restarted

The root cause is that:
com.google.common.cache::cleanup will not trigger removalListener action, we shall use `invalidateAll` instead.

Testing:
Nothing print for below code:
```
import com.google.common.cache.{Cache, CacheBuilder, RemovalNotification}

var removed = 0
val cache: Cache[String, String] = CacheBuilder.newBuilder()
  .removalListener((notification: RemovalNotification[String, String]) => {
    removed += 1
    println("removed: " + removed)
  }).build()

(0 until 1000).foreach { i =>
  cache.put(i.toString, i.toString)
}

cache.cleanup()
```

Replacing `cache.cleanup()` with `cache.invalidateAll()`, the output is expected.

## Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

## Types of changes 🔖

- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️

#### Behavior With This Pull Request 🎉

#### Related Unit Tests

---

# Checklist 📝

- [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes #6319 from turboFei/app_k8s_leak.

Closes #6320

ff2adfc [Wang, Fei] invalidate all

Authored-by: Wang, Fei <fwang12@ebay.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 26fe59d)
Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793 pushed a commit that referenced this pull request Apr 19, 2024
@turboFei @pan3793
[KYUUBI #6320] Fix terminated application pods not deleted issue when…
b635d70 
… kyuubi server restarted

# 🔍 Description
## Issue References 🔗

This pull request fixes #

We found that some pods were not deleted if kyuubi server restarted

The root cause is that:
com.google.common.cache::cleanup will not trigger removalListener action, we shall use `invalidateAll` instead.

Testing:
Nothing print for below code:
```
import com.google.common.cache.{Cache, CacheBuilder, RemovalNotification}

var removed = 0
val cache: Cache[String, String] = CacheBuilder.newBuilder()
  .removalListener((notification: RemovalNotification[String, String]) => {
    removed += 1
    println("removed: " + removed)
  }).build()

(0 until 1000).foreach { i =>
  cache.put(i.toString, i.toString)
}

cache.cleanup()
```

Replacing `cache.cleanup()` with `cache.invalidateAll()`, the output is expected.

## Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

## Types of changes 🔖

- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️

#### Behavior With This Pull Request 🎉

#### Related Unit Tests

---

# Checklist 📝

- [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes #6319 from turboFei/app_k8s_leak.

Closes #6320

ff2adfc [Wang, Fei] invalidate all

Authored-by: Wang, Fei <fwang12@ebay.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 26fe59d)
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793
Copy link
Member

pan3793 commented Apr 19, 2024

Thanks, merged to master/1.9/1.8

Actually, we have a CronJob to clean up terminated Pod too.

@turboFei turboFei deleted the app_k8s_leak branch April 19, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pan3793 pan3793 pan3793 approved these changes

Assignees

@turboFei turboFei

Labels
module:server
Projects
None yet
Milestone
v1.8.2
Development

Successfully merging this pull request may close these issues.
2 participants
@turboFei @pan3793