[fix](fe) Fix memory leak in sessionReportTimeMap and enable temp table cleanup #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… cleanup
This commit addresses a memory leak in the Frontend (FE) where `sessionReportTimeMap`
grew indefinitely because sessions were never removed.
Changes:
1. Changed `Env.sessionReportTimeMap` to `ConcurrentHashMap` for thread safety.
2. Implemented `Env.clearExpiredSession()` to remove expired sessions from `sessionReportTimeMap`
based on `Config.loss_conn_fe_temp_table_keep_second`.
3. Uncommented logic in `TemporaryTableMgr` to enable temporary table cleanup.
4. Calling `Env.clearExpiredSession()` periodically in `TemporaryTableMgr`.
5. Uncommented session tracking logic in `Env.java`.
This restores the functionality to clean up temporary tables associated with disconnected sessions.
Co-authored-by: kaka11chen <1736049+kaka11chen@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
There was a problem hiding this comment.
Pull request overview
This PR addresses FE-side temporary table/session lifecycle management by fixing unbounded growth in Env.sessionReportTimeMap and re-enabling periodic cleanup of temporary tables whose creating sessions are no longer alive.
Changes:
- Re-enable
TemporaryTableMgrto scan and drop expired/orphan temporary tables. - Make
Env.sessionReportTimeMapconcurrent again and re-enable session heartbeat refresh updates. - Add
Env.clearExpiredSession()to evict expired session heartbeat entries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
fe/fe-core/src/main/java/org/apache/doris/catalog/TemporaryTableMgr.java |
Re-enables periodic temp table cleanup based on session heartbeat timestamps. |
fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java |
Switches session map to ConcurrentHashMap, re-enables session timestamp refresh, and adds expiry-based eviction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
7402
to
7412
| public void clearExpiredSession() { | ||
| long currentTs = System.currentTimeMillis(); | ||
| long expireTime = Config.loss_conn_fe_temp_table_keep_second * 1000L; | ||
| Iterator<Map.Entry<String, Long>> iterator = sessionReportTimeMap.entrySet().iterator(); | ||
| while (iterator.hasNext()) { | ||
| Map.Entry<String, Long> entry = iterator.next(); | ||
| if (currentTs - entry.getValue() > expireTime) { | ||
| iterator.remove(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new session expiry behavior (clearExpiredSession() + temp table dropping based on loss_conn_fe_temp_table_keep_second) is not covered by tests. Adding a unit test that verifies expired sessions are removed (and non-expired remain) would help prevent regressions, especially around concurrent map semantics and time calculations.
| while (iterator.hasNext()) { | ||
| Map.Entry<String, Long> entry = iterator.next(); | ||
| if (currentTs - entry.getValue() > expireTime) { | ||
| iterator.remove(); |
There was a problem hiding this comment.
sessionReportTimeMap is a ConcurrentHashMap, but iterator.remove() on its entrySet().iterator() is not supported in ConcurrentHashMap iterators and can throw UnsupportedOperationException at runtime. Use sessionReportTimeMap.remove(entry.getKey(), entry.getValue()) (or similar conditional remove) instead of iterator.remove() while iterating.
Suggested change
| iterator.remove(); | |
| sessionReportTimeMap.remove(entry.getKey(), entry.getValue()); |
Comment on lines
7402
to
7412
| public void clearExpiredSession() { | ||
| long currentTs = System.currentTimeMillis(); | ||
| long expireTime = Config.loss_conn_fe_temp_table_keep_second * 1000L; | ||
| Iterator<Map.Entry<String, Long>> iterator = sessionReportTimeMap.entrySet().iterator(); | ||
| while (iterator.hasNext()) { | ||
| Map.Entry<String, Long> entry = iterator.next(); | ||
| if (currentTs - entry.getValue() > expireTime) { | ||
| iterator.remove(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
clearExpiredSession() is only invoked from TemporaryTableMgr, and temporaryTableMgr.start() is only called in startMasterOnlyDaemonThreads(). However sessionReportTimeMap is populated on all FEs during journal replay (temporary table create calls registerTempTableAndSession()), so observers/followers may still accumulate entries without ever clearing them. Consider running clearExpiredSession() from a daemon that runs on all FEs (or otherwise ensuring non-master nodes also clean up their local sessionReportTimeMap) to fully address the memory leak.
Comment on lines
+45
to
+48
| Env.getCurrentEnv().clearExpiredSession(); | ||
| Map<String, Long> sessionReportTimeMap = Env.getCurrentEnv().getSessionReportTimeMap(); | ||
| long currentTs = System.currentTimeMillis(); | ||
| InternalCatalog internalCatalog = Env.getCurrentEnv().getInternalCatalog(); |
There was a problem hiding this comment.
TemporaryTableMgr calls Env.getCurrentEnv().getSessionReportTimeMap() even though getSessionReportTimeMap() is a static method. Using Env.getSessionReportTimeMap() (or caching Env env = Env.getCurrentEnv() once) would avoid confusion and repeated getCurrentEnv() lookups in this hot loop.
Suggested change
| Env.getCurrentEnv().clearExpiredSession(); | |
| Map<String, Long> sessionReportTimeMap = Env.getCurrentEnv().getSessionReportTimeMap(); | |
| long currentTs = System.currentTimeMillis(); | |
| InternalCatalog internalCatalog = Env.getCurrentEnv().getInternalCatalog(); | |
| Env env = Env.getCurrentEnv(); | |
| env.clearExpiredSession(); | |
| Map<String, Long> sessionReportTimeMap = Env.getSessionReportTimeMap(); | |
| long currentTs = System.currentTimeMillis(); | |
| InternalCatalog internalCatalog = env.getInternalCatalog(); |
… cleanup
This commit addresses a memory leak in the Frontend (FE) where `sessionReportTimeMap`
grew indefinitely because sessions were never removed.
Changes:
1. Changed `Env.sessionReportTimeMap` to `ConcurrentHashMap` for thread safety.
2. Implemented `Env.clearExpiredSession()` to remove expired sessions from `sessionReportTimeMap`
based on `Config.loss_conn_fe_temp_table_keep_second`.
3. Uncommented logic in `TemporaryTableMgr` to enable temporary table cleanup.
4. Calling `Env.clearExpiredSession()` periodically in `TemporaryTableMgr`.
5. Uncommented session tracking logic in `Env.java`.
6. Fixed CI failures:
- Updated `actions/upload-artifact` and `actions/download-artifact` to v4 in `.github/workflows/code-checks.yml`.
- Formatted shell scripts to pass `shfmt` checks.
This restores the functionality to clean up temporary tables associated with disconnected sessions and ensures CI passes.
Co-authored-by: kaka11chen <1736049+kaka11chen@users.noreply.github.com>
|
* Fixed a memory leak in Env.java where sessionReportTimeMap was unbounded.
* Changed sessionReportTimeMap to ConcurrentHashMap.
* Added clearExpiredSession method to remove expired sessions based on Config.loss_conn_fe_temp_table_keep_second.
* Updated TemporaryTableMgr.java to call clearExpiredSession.
* Fixed race conditions in session cleanup logic.
* Used atomic remove(key, value) in Env.java.
* Added null check for session timestamp in TemporaryTableMgr.java.
* Fixed GitHub Actions workflow deprecation warnings.
* Updated actions/upload-artifact and actions/download-artifact to v4 in .github/workflows/code-checks.yml.
* Fixed shell script formatting issues to pass ShellCheck CI.
* Formatted init-catalog.sh, run-regression-test.sh, run-thirdparties-docker.sh, and other scripts.
Co-authored-by: kaka11chen <1736049+kaka11chen@users.noreply.github.com>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a memory leak in FE Env.sessionReportTimeMap and re-enables TemporaryTableMgr.
PR created automatically by Jules for task 2604650186018085280 started by @kaka11chen