HIVE-27636. Exception in HiveMaterializedViewsRegistry is leaving staging directories behind - #6650
HIVE-27636. Exception in HiveMaterializedViewsRegistry is leaving staging directories behind#6650anshuksi282-ksolves wants to merge 2 commits into
Conversation
…ging directories behind
rtrivedi12
left a comment
rtrivedi12
left a comment
There was a problem hiding this comment.
@anshuksi282-ksolves Thank you for the patch! Can you please check the 8 new sonar issues introduced from Test class? Thanks!
| LOG.warn("Error while cleaning up staging directories for materialized view " + | ||
| materializedViewTable.getCompleteName(), ioe); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
@anshuksi282-ksolves context.clear() is only called on exception, but I think the success path also abandons the Context without cleanup. Do you think we should have context.clear() in finally block so it is always called?
There was a problem hiding this comment.
@anshuksi282-ksolves context.clear() is only called on exception, but I think the success path also abandons the Context without cleanup. Do you think we should have context.clear() in finally block so it is always called?
Thanks for catching that, @rtrivedi12! You're right — moved context.clear() into a finally block now, so it's called on both the success and exception paths. Also fixed the checkstyle indentation issues flagged by SonarQube. Please take a look when you get a chance.
0f74d64 to
d61b1ab
Compare
|
3 participants
What changes were proposed in this pull request?
HiveMaterializedViewsRegistry.createMaterialization()creates aContextobject (used for query parsing/semantic analysis) before attempting to parse the materialized view's query. If parsing or semantic analysis fails, the method logs a warning and returnsnull, but never calledcontext.clear()to clean up any scratch/staging directories theContextmay have created during that attempt.This PR adds a call to
context.clear()inside thecatchblock, so that any such directories are removed eagerly, at the point the exception is caught, rather than being left behind.Why are the changes needed?
Directories created by
Context(e.g. for CTE materialization, or dummy-table handling for FROM-less queries) are only registered viafs.deleteOnExit(). This means they are only cleaned up when the JVM exits. For a long-running process like HiveServer2, ifcreateMaterialization()repeatedly hits parse/semantic exceptions (e.g. due to a materialized view definition referencing a table that no longer exists), these directories accumulate over time and are never removed until the server restarts. This can eventually cause HDFS directory-item-limit errors, as reported in the JIRA:Cannot create staging directory '...': The directory item limit of ... is exceeded: limit=1048576 items=1048576
Does this PR introduce any user-facing change?
No.
How was this patch tested?
TestHiveMaterializedViewsRegistry#testStagingDirIsCleanedUpOnParseException. It uses a materialized view query with a CTE referenced enough times to trigger CTE materialization (which creates a real directory tracked byContext), followed by an invalid column reference that fails during semantic analysis. The test asserts that no per-query scratch subdirectory remains oncecreateMaterialization()returns.TestContext#testClearRemovesScratchDirEagerlyWhenHDFSCleanupEnabled, a more targeted test confirming thatContext.clear()removes a scratch directory immediately (rather than relying ondeleteOnExit) whenisHDFSCleanupis enabled.mvn -pl ql test -Dtest=TestContext,TestHiveMaterializedViewsRegistrypasses.Contains content generated by Claude (Anthropic), used to help investigate the root cause, draft the fix, and design/verify the accompanying tests.