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.

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

[Bug] Close KyuubiSession when OperationLog not initialized may lead log file leak #5832

Closed
3 of 4 tasks
Flyangz opened this issue Dec 8, 2023 · 4 comments
Closed
3 of 4 tasks

[Bug] Close KyuubiSession when OperationLog not initialized may lead log file leak #5832

Flyangz opened this issue Dec 8, 2023 · 4 comments
Labels
kind:bug This is a clearly a bug priority:major

Comments

@Flyangz
Copy link
Contributor

Flyangz commented Dec 8, 2023 •
edited
Loading

Code of Conduct

Search before asking

  • I have searched in the issues and found no similar issues.

Describe the bug

I added some debug logs and got the below pictures.
截屏2023-12-08 11 09 01
We can see kyuubi received a close session request and closed corresponding operation logs. Let say thread A done this work. One log was closed before initialized, so the the OperationLog#close call did not close writer.

kyuubi/kyuubi-common/src/main/scala/org/apache/kyuubi/operation/log/OperationLog.scala

Lines 234 to 235 in 1d6e653

def close(): Unit = synchronized {
if (!initialized) return

After that, OperationLog#write was called by other thread B. Finally, the thread A called SessionManager#deleteOperationLogSessionDir and cause log file leak.
截屏2023-12-08 11 15 04

Affects Version(s)

1.7.1 with #5211 patch

Kyuubi Server Log Output

No response

Kyuubi Engine Log Output

No response

Kyuubi Server Configurations

No response

Kyuubi Engine Configurations

No response

Additional context

Maybe we can delete if (!initialized) return to fix it?

Are you willing to submit PR?

  • Yes. I would be willing to submit a PR with guidance from the Kyuubi community to fix.
  • No. I cannot submit a PR at this time.
@Flyangz Flyangz added kind:bug This is a clearly a bug priority:major labels Dec 8, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 8, 2023

Hello @Flyangz,
Thanks for finding the time to report the issue!
We really appreciate the community's efforts to improve Apache Kyuubi.

@yaooqinn
Copy link
Member

yaooqinn commented Dec 8, 2023

Thanks @Flyangz for reporting this issue, also cc @turboFei @pan3793

@Flyangz Flyangz mentioned this issue Dec 14, 2023
Closed
18 tasks
@pan3793
Copy link
Member

pan3793 commented Dec 14, 2023

Seems solved by #5211

@Flyangz
Copy link
Contributor Author

Flyangz commented Dec 14, 2023

Seems solved by #5211

Sorry that I have not mentioned the #5211 had been patched. We applied it when we first discovered the leak, but it didn't work.

pan3793 pushed a commit that referenced this issue Dec 15, 2023
@Flyangz @pan3793
[KYUUBI #5832] Always perform closing action in OperationLog to avoid…
f16cfc1 
… fd leak

# 🔍 Description
## Issue References 🔗

This pull request fixes #5832

## Describe Your Solution 🔧

We should always close writer regardless of whether OperationLog is initialized or not, to prevent other threads from continuing to write.

## Types of changes 🔖

- [x] 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

---

# Checklists
## 📝 Author Self Checklist

- [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [x] I have performed a self-review
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

## 📝 Committer Pre-Merge Checklist

- [x] Pull request title is okay.
- [x] No license issues.
- [x] Milestone correctly set?
- [ ] Test coverage is ok
- [x] Assignees are selected.
- [x] Minimum number of approvals
- [x] No changes are requested

**Be nice. Be informative.**

Closes #5854 from Flyangz/fix-op-log-leak.

Closes #5832

2549928 [liuyang] fix fd leak when closing KyuubiSession but OperationLog not initialized

Authored-by: liuyang <flyang116@126.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 64ee629)
Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793 pushed a commit that referenced this issue Dec 15, 2023
@Flyangz @pan3793
[KYUUBI #5832] Always perform closing action in OperationLog to avoid…
081efcd 
… fd leak

# 🔍 Description
## Issue References 🔗

This pull request fixes #5832

## Describe Your Solution 🔧

We should always close writer regardless of whether OperationLog is initialized or not, to prevent other threads from continuing to write.

## Types of changes 🔖

- [x] 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

---

# Checklists
## 📝 Author Self Checklist

- [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [x] I have performed a self-review
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

## 📝 Committer Pre-Merge Checklist

- [x] Pull request title is okay.
- [x] No license issues.
- [x] Milestone correctly set?
- [ ] Test coverage is ok
- [x] Assignees are selected.
- [x] Minimum number of approvals
- [x] No changes are requested

**Be nice. Be informative.**

Closes #5854 from Flyangz/fix-op-log-leak.

Closes #5832

2549928 [liuyang] fix fd leak when closing KyuubiSession but OperationLog not initialized

Authored-by: liuyang <flyang116@126.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 64ee629)
Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793 added a commit that referenced this issue Feb 22, 2024
@pan3793
[KYUUBI #6074] Add a script to simplify the process of creating relea…
2c70c67 
…se notes

# 🔍 Description
## Issue References 🔗

Currently, we use a rather primitive way to manually write release notes from scratch, and some of the mechanical and repetitive work can be simplified by the scripts.

## Describe Your Solution 🔧

Adds a script to simplify the process of creating release notes.

Note: it just simplifies some processes, the release manager still needs to tune the outputs by hand.

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

```
RELEASE_TAG=v1.8.1 PREVIOUS_RELEASE_TAG=v1.8.0 build/release/pre_gen_release_notes.py
```

```
$ head build/release/commits-v1.8.1.txt
[KYUUBI #5981] Deploy Spark Hive connector with Scala 2.13 to Maven Central
[KYUUBI #6058] Make Jetty server stop timeout configurable
[KYUUBI #5952][1.8] Disconnect connections without running operations after engine maxlife time graceful period
[KYUUBI #6048] Assign serviceNode and add volatile for variables
[KYUUBI #5991] Error on reading Atlas properties composed of multi values
[KYUUBI #6045] [REST] Sync the AdminRestApi with the AdminResource Apis
[KYUUBI #6047] [CI] Free up disk space
[KYUUBI #6036] JDBC driver conditional sets fetchSize on opening session
[KYUUBI #6028] Exited spark-submit process should not block batch submit queue
[KYUUBI #6018] Speed up GetTables operation for Spark session catalog
```

```
$ head build/release/contributors-v1.8.1.txt
* Shaoyun Chen        -- [KYUUBI #5857][KYUUBI #5720][KYUUBI #5785][KYUUBI #5617]
* Chao Chen           -- [KYUUBI #5750]
* Flyangz             -- [KYUUBI #5832]
* Pengqi Li           -- [KYUUBI #5713]
* Bowen Liang         -- [KYUUBI #5730][KYUUBI #5802][KYUUBI #5767][KYUUBI #5831][KYUUBI #5801][KYUUBI #5754][KYUUBI #5626][KYUUBI #5811][KYUUBI #5853][KYUUBI #5765]
* Paul Lin            -- [KYUUBI #5799][KYUUBI #5814]
* Senmiao Liu         -- [KYUUBI #5969][KYUUBI #5244]
* Xiao Liu            -- [KYUUBI #5962]
* Peiyue Liu          -- [KYUUBI #5331]
* Junjie Ma           -- [KYUUBI #5789]
```
---

# Checklist 📝

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

**Be nice. Be informative.**

Closes #6074 from pan3793/release-script.

Closes #6074

3d5ec20 [Cheng Pan] credits
1765279 [Cheng Pan] Add a script to simplify the process of creating release notes

Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this issue Mar 21, 2024
@Flyangz @zhaohehuhu
[KYUUBI apache#5832] Always perform closing action in OperationLog to…
b23d1d4 
… avoid fd leak

# 🔍 Description
## Issue References 🔗

This pull request fixes apache#5832

## Describe Your Solution 🔧

We should always close writer regardless of whether OperationLog is initialized or not, to prevent other threads from continuing to write.

## Types of changes 🔖

- [x] 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

---

# Checklists
## 📝 Author Self Checklist

- [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [x] I have performed a self-review
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

## 📝 Committer Pre-Merge Checklist

- [x] Pull request title is okay.
- [x] No license issues.
- [x] Milestone correctly set?
- [ ] Test coverage is ok
- [x] Assignees are selected.
- [x] Minimum number of approvals
- [x] No changes are requested

**Be nice. Be informative.**

Closes apache#5854 from Flyangz/fix-op-log-leak.

Closes apache#5832

2549928 [liuyang] fix fd leak when closing KyuubiSession but OperationLog not initialized

Authored-by: liuyang <flyang116@126.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this issue Mar 21, 2024
@pan3793 @zhaohehuhu
[KYUUBI apache#6074] Add a script to simplify the process of creating…
9f3ab85 
… release notes

# 🔍 Description
## Issue References 🔗

Currently, we use a rather primitive way to manually write release notes from scratch, and some of the mechanical and repetitive work can be simplified by the scripts.

## Describe Your Solution 🔧

Adds a script to simplify the process of creating release notes.

Note: it just simplifies some processes, the release manager still needs to tune the outputs by hand.

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

```
RELEASE_TAG=v1.8.1 PREVIOUS_RELEASE_TAG=v1.8.0 build/release/pre_gen_release_notes.py
```

```
$ head build/release/commits-v1.8.1.txt
[KYUUBI apache#5981] Deploy Spark Hive connector with Scala 2.13 to Maven Central
[KYUUBI apache#6058] Make Jetty server stop timeout configurable
[KYUUBI apache#5952][1.8] Disconnect connections without running operations after engine maxlife time graceful period
[KYUUBI apache#6048] Assign serviceNode and add volatile for variables
[KYUUBI apache#5991] Error on reading Atlas properties composed of multi values
[KYUUBI apache#6045] [REST] Sync the AdminRestApi with the AdminResource Apis
[KYUUBI apache#6047] [CI] Free up disk space
[KYUUBI apache#6036] JDBC driver conditional sets fetchSize on opening session
[KYUUBI apache#6028] Exited spark-submit process should not block batch submit queue
[KYUUBI apache#6018] Speed up GetTables operation for Spark session catalog
```

```
$ head build/release/contributors-v1.8.1.txt
* Shaoyun Chen        -- [KYUUBI apache#5857][KYUUBI apache#5720][KYUUBI apache#5785][KYUUBI apache#5617]
* Chao Chen           -- [KYUUBI apache#5750]
* Flyangz             -- [KYUUBI apache#5832]
* Pengqi Li           -- [KYUUBI apache#5713]
* Bowen Liang         -- [KYUUBI apache#5730][KYUUBI apache#5802][KYUUBI apache#5767][KYUUBI apache#5831][KYUUBI apache#5801][KYUUBI apache#5754][KYUUBI apache#5626][KYUUBI apache#5811][KYUUBI apache#5853][KYUUBI apache#5765]
* Paul Lin            -- [KYUUBI apache#5799][KYUUBI apache#5814]
* Senmiao Liu         -- [KYUUBI apache#5969][KYUUBI apache#5244]
* Xiao Liu            -- [KYUUBI apache#5962]
* Peiyue Liu          -- [KYUUBI apache#5331]
* Junjie Ma           -- [KYUUBI apache#5789]
```
---

# Checklist 📝

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

**Be nice. Be informative.**

Closes apache#6074 from pan3793/release-script.

Closes apache#6074

3d5ec20 [Cheng Pan] credits
1765279 [Cheng Pan] Add a script to simplify the process of creating release notes

Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
beryllw pushed a commit to beryllw/incubator-kyuubi that referenced this issue Jun 7, 2024
@Flyangz @wForget
[KYUUBI apache#5832] Always perform closing action in OperationLog to…
d62488f 
… avoid fd leak

# 🔍 Description
## Issue References 🔗

This pull request fixes apache#5832

## Describe Your Solution 🔧

We should always close writer regardless of whether OperationLog is initialized or not, to prevent other threads from continuing to write.

## Types of changes 🔖

- [x] 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

---

# Checklists
## 📝 Author Self Checklist

- [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [x] I have performed a self-review
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

## 📝 Committer Pre-Merge Checklist

- [x] Pull request title is okay.
- [x] No license issues.
- [x] Milestone correctly set?
- [ ] Test coverage is ok
- [x] Assignees are selected.
- [x] Minimum number of approvals
- [x] No changes are requested

**Be nice. Be informative.**

Closes apache#5854 from Flyangz/fix-op-log-leak.

Closes apache#5832

2549928 [liuyang] fix fd leak when closing KyuubiSession but OperationLog not initialized

Authored-by: liuyang <flyang116@126.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 64ee629)
Signed-off-by: Cheng Pan <chengpan@apache.org>
beryllw pushed a commit to beryllw/incubator-kyuubi that referenced this issue Jun 7, 2024
@wForget
Merge branch 'branch-1.8' into 'branch-1.8.0'
a08447c 
[KYUUBI apache#5832] Always perform closing action in OperationLog to avoid fd leak

See merge request !49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind:bug This is a clearly a bug priority:major
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[KYUUBI #5832] Always perform closing action in OperationLog to avoid fd leak Flyangz/kyuubi
3 participants
@yaooqinn @pan3793 @Flyangz