-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(core): release disk space faster on table drop #6555
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
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWhen a table is dropped, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/io/questdb/cairo/CairoEngine.java(1 hunks)core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java(1 hunks)core/src/main/java/io/questdb/cairo/pool/PoolConstants.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java (2)
core/src/main/java/io/questdb/std/Unsafe.java (1)
Unsafe(40-542)core/src/main/java/io/questdb/cairo/pool/PoolConstants.java (1)
PoolConstants(27-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (36)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
core/src/main/java/io/questdb/cairo/CairoEngine.java (1)
1197-1198: LGTM! Pool notifications correctly trigger cleanup on table drop.The calls to
notifyDropped()on both pools appropriately notify them to release idle resources when a table is dropped, which aligns with the PR objective of freeing disk space faster.core/src/main/java/io/questdb/cairo/pool/PoolConstants.java (2)
29-29: New constantCR_DROPPEDadded for table drop operations.The addition of
CR_DROPPEDprovides a distinct close reason for dropped tables, improving observability and logging clarity. Ensure this constant is used consistently where tables are dropped (see review comment in AbstractMultiTenantPool.java).
36-44: LGTM! Switch expression refactoring is clean and complete.The refactoring to a switch expression is idiomatic Java and correctly maps all close reason constants, including the new
CR_DROPPED, to their text representations.
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java
Outdated
Show resolved
Hide resolved
|
can we have a test also? |
[PR Coverage check]😍 pass : 100 / 113 (88.50%) file detail
|
|
This reverts commit f3be96a.
This change closes WAL writers faster that helps deleting them from disk.
Before the change when table is dropped, there are a lot of logging like this:
Now when idle WAL writers are closed at the time of the drop, the WAL and table files are deleted faster.