HIVE-29465:Prevent excessive query results cache usage at runtime#6376
HIVE-29465:Prevent excessive query results cache usage at runtime#6376ramitg254 wants to merge 1 commit intoapache:masterfrom
Conversation
ramitg254
changed the title
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
ramitg254
changed the title
ramitg254
changed the title
ramitg254
changed the title
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
|
| return false; | ||
| } | ||
|
|
||
| if (isSafeCacheWriteEnabled) { |
There was a problem hiding this comment.
@ramitg254 : thanks for working on this so far
I'm not sure if the approach fully addresses what has been reported: as far as I can understand, there is a safe buffer directory, where the files are placed, and this safe folder is on the same storage, but that's not the only issue, it's rather that this doesn't prevent big files actually landing on the filesystem that holds the cache
the original report showed something like this:
du -h -d 1 /efs/tmp/hive/_resultscache_/results-9d89cc59-c99d-46a5-9d93-2b550576532012.0K ./66356edb-57a6-4f0a-90cd-7d14d9e2b739
...
1.1T ./0fe343fb-6a89-4d28-b2fd-caed2f2e42f6
...
1.1T .
I missed something to double-check before creating the jira: if the "0fe343fb-6a89-4d28-b2fd-caed2f2e42f6" folder belongs to a finished query result? if so - and given that it clearly exceeded the configured 2G max cache size - query results cache should have taken care of that, so I think the original problem/usecase should be investigated thoroughly first
There was a problem hiding this comment.
@abstractdog I have written down my understanding of the problem below please validate:
Problem statement -> so when cache is enabled cache entry validation takes place at post execution causing cache entry already been written to cache directory in runtime exceeding max cache size which should not happen.
Replication of Problem Statement scenario -> I have added unit test which replicate this problem statemnt in TestCachedResults.java in which for the same set of queries testUnsafeCacheWrite passes when at runtime cache directory size at runtime increases beyond max cache size allowed which is the current behaviour we have and the other is testSafeCacheWrite which is solving this issue for the same set of queries and not exceeding at any moment on runtime beyond max cache size allowed.
Solution: we are introducing safe cache write conf which is when enabled then query files for fetch work does not directly written to cache directory but we proceed it as normal query execution and since normal query execution also stores these files somewhere during runtime (like in local scratch dir in case of these unit tests) and by this way we are not maintaining any extra storage temporary buffer and if it fails then fails as normal query and if it succeeds then we perform validation checks for those files in normal query execution and if it is valid then we just copy those files to cache dir in post execution and if while moving these files if it fails in between then I am performing cleaanup as well for it.
And I made this configurable as there is overhead in copying files from location of normal query execution to cache directory
| private static final Logger LOG = LoggerFactory.getLogger(TestCachedResults.class); | ||
|
|
||
| @ClassRule | ||
| public static HiveTestEnvSetup envSetup = new HiveTestEnvSetup(); |
There was a problem hiding this comment.
we don't need this, HiveConfForTest is sufficient in most of the cases
| public static HiveTestEnvSetup envSetup = new HiveTestEnvSetup(); | ||
|
|
||
| @Rule | ||
| public TestRule methodRule = envSetup.getMethodRule(); |
There was a problem hiding this comment.
I think don't need this either
|
|
||
| @BeforeClass | ||
| public static void setUp() throws Exception { | ||
| conf = envSetup.getTestCtx().hiveConf; |
There was a problem hiding this comment.
new HiveConfForTest(...)
| driver.run(sql); | ||
| } | ||
|
|
||
| private static long getFolderSize(File folder) { |
There was a problem hiding this comment.
this looks like reinventing the wheel, is there a chance we already have this implemented somewhere else?
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a “safe cache write” mode for Hive query results cache to prevent oversized/invalid entries from being written directly into the cache directory (reducing runtime cache overspill), with accompanying tests.
Changes:
- Introduces
hive.query.results.safe.cache.write.enabledconfiguration flag. - Updates results destination selection so safe mode writes outside the cache and later conditionally copies into the cache.
- Adds runtime copy logic in
QueryResultsCache#setEntryValidand a new test suite validating safe vs unsafe behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| ql/src/test/org/apache/hadoop/hive/ql/TestCachedResults.java | Adds tests and a cache-size monitor to verify safe vs unsafe cache write behavior. |
| ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java | Routes output to default destination in safe mode and records a “safe” source directory. |
| ql/src/java/org/apache/hadoop/hive/ql/cache/results/QueryResultsCache.java | Implements safe-mode copy into cache on entry validation and cleanup helpers. |
| common/src/java/org/apache/hadoop/hive/conf/HiveConf.java | Adds the new configuration variable and description text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @Test | ||
| public void testSafeCacheWrite() throws Exception { | ||
| HiveConf.setBoolVar(conf, HiveConf.ConfVars.HIVE_QUERY_RESULTS_SAFE_CACHE_WRITE_ENABLED, true); |
There was a problem hiding this comment.
The tests mutate a shared static conf but testUnsafeCacheWrite never explicitly sets HIVE_QUERY_RESULTS_SAFE_CACHE_WRITE_ENABLED back to false. If testSafeCacheWrite runs first (test order is not guaranteed), testUnsafeCacheWrite will execute in safe mode and assert the opposite behavior. Make each test explicitly set the flag to the intended value (or reset it in @Before / @After).
| } | ||
|
|
||
| @Test | ||
| public void testUnsafeCacheWrite() throws Exception { |
There was a problem hiding this comment.
The tests mutate a shared static conf but testUnsafeCacheWrite never explicitly sets HIVE_QUERY_RESULTS_SAFE_CACHE_WRITE_ENABLED back to false. If testSafeCacheWrite runs first (test order is not guaranteed), testUnsafeCacheWrite will execute in safe mode and assert the opposite behavior. Make each test explicitly set the flag to the intended value (or reset it in @Before / @After).
| public void testUnsafeCacheWrite() throws Exception { | |
| public void testUnsafeCacheWrite() throws Exception { | |
| HiveConf.setBoolVar(conf, HiveConf.ConfVars.HIVE_QUERY_RESULTS_SAFE_CACHE_WRITE_ENABLED, false); |
| private static String cacheDir; | ||
|
|
||
| private ScheduledExecutorService scheduler; | ||
| private long maxCacheSize = 0; |
There was a problem hiding this comment.
maxCacheSize is written from the scheduler thread and read from the test thread without any memory-visibility guarantees, which can make assertions flaky (e.g., reading a stale 0). Use an AtomicLong (or make it volatile and update carefully) to ensure cross-thread visibility.
| private long maxCacheSize = 0; | |
| private volatile long maxCacheSize = 0; |
| private void startCacheMonitor(long intervalMs) { | ||
| scheduler.scheduleAtFixedRate(() -> { | ||
| long size = getFolderSize(new File(cacheDir)); | ||
| maxCacheSize = Math.max(maxCacheSize, size); | ||
| }, 0, intervalMs, TimeUnit.MILLISECONDS); | ||
| } |
There was a problem hiding this comment.
maxCacheSize is written from the scheduler thread and read from the test thread without any memory-visibility guarantees, which can make assertions flaky (e.g., reading a stale 0). Use an AtomicLong (or make it volatile and update carefully) to ensure cross-thread visibility.
| conf = envSetup.getTestCtx().hiveConf; | ||
|
|
||
| HiveConf.setBoolVar(conf, HiveConf.ConfVars.HIVE_QUERY_RESULTS_CACHE_ENABLED, true); | ||
| HiveConf.setVar(conf, HiveConf.ConfVars.HIVE_QUERY_RESULTS_CACHE_DIRECTORY, "/tmp/hive/cache"); |
There was a problem hiding this comment.
Hard-coding /tmp/hive/cache in a test can cause collisions across concurrent test runs and may fail on environments without a writable /tmp (or on non-Unix setups). Prefer using a per-test temporary directory (e.g., test framework temp dirs / java.nio.file.Files#createTempDirectory) and set the conf to that path.
| Path destFile = new Path(resultDir, | ||
| new Path(fs.getPath().toString().substring(safeDir.length() + 1))); |
There was a problem hiding this comment.
Building relative paths via substring(safeDir.length() + 1) is fragile: it assumes the string form of each path always starts with safeDir (same scheme/authority, same qualification, no trailing slash differences). If that assumption breaks, it can produce incorrect paths or throw StringIndexOutOfBoundsException. Prefer computing a proper relative path using qualified Path/URI comparison (e.g., qualify both paths and derive the relative suffix from URI paths) and validate that the source is inside safeDir before deriving the destination.
| return false; | ||
| } | ||
| fetchWork.setFilesToFetch(cacheFilesToFetch); | ||
| fetchWork.setTblDir(new Path(resultDir, fetchWork.getTblDir().toString().substring(safeDir.length() + 1))); |
There was a problem hiding this comment.
Building relative paths via substring(safeDir.length() + 1) is fragile: it assumes the string form of each path always starts with safeDir (same scheme/authority, same qualification, no trailing slash differences). If that assumption breaks, it can produce incorrect paths or throw StringIndexOutOfBoundsException. Prefer computing a proper relative path using qualified Path/URI comparison (e.g., qualify both paths and derive the relative suffix from URI paths) and validate that the source is inside safeDir before deriving the destination.
| rwLock.writeLock().lock(); | ||
| boolean succeeded = true; | ||
| try { | ||
| for (FileStatus fs : fetchWork.getFilesToFetch()) { |
There was a problem hiding this comment.
The cache write lock is held while performing potentially slow filesystem copies. This can block unrelated cache operations for the duration of the copy and hurt concurrency under load. Consider copying files outside the cache rwLock, and only acquiring the lock for the minimal critical section where shared cache state is updated.
| Path srcFile = fs.getPath(); | ||
| Path destFile = new Path(resultDir, | ||
| new Path(fs.getPath().toString().substring(safeDir.length() + 1))); | ||
| succeeded = FileUtil.copy(srcFs, srcFile, cacheFs, destFile, false, conf); |
There was a problem hiding this comment.
The cache write lock is held while performing potentially slow filesystem copies. This can block unrelated cache operations for the duration of the copy and hurt concurrency under load. Consider copying files outside the cache rwLock, and only acquiring the lock for the minimal critical section where shared cache state is updated.
| "If the query results safe cache is enabled. This will safely write to cache directory by first evaluating " + | ||
| "the cache entry is not overspilling the the cache directory before writing it to cache directory "), |
There was a problem hiding this comment.
The new conf var description has grammatical issues (e.g., duplicated 'the', awkward phrasing) and it doesn’t clearly describe the actual behavior implemented (write results to a non-cache destination first, then copy into cache only after the entry is deemed valid/acceptable). Please reword to be precise and user-facing.
| "If the query results safe cache is enabled. This will safely write to cache directory by first evaluating " + | |
| "the cache entry is not overspilling the the cache directory before writing it to cache directory "), | |
| "If enabled, query results are written to a temporary non-cache location first and copied into the " + | |
| "cache directory only after the cache entry is accepted as valid, for example after size checks."), |
There was a problem hiding this comment.
agreed, I was about to comment the same, configuration should be clear about the tradeoff which is present under normal circumstances
| "If the query results safe cache is enabled. This will safely write to cache directory by first evaluating " + | ||
| "the cache entry is not overspilling the the cache directory before writing it to cache directory "), |
| cacheFs.mkdirs(resultDir); | ||
|
|
||
| Set<FileStatus> cacheFilesToFetch = new HashSet<>(); | ||
| rwLock.writeLock().lock(); |
There was a problem hiding this comment.
I can see that in other places in this class, this is handled via a separate variable:
Lock writeLock = rwLock.writeLock();
try{
writeLock.lock();
}
but that usage pattern is not consistent: feel free to pick one, and unify all the lock usages
| return false; | ||
| } | ||
|
|
||
| if (isSafeCacheWriteEnabled) { |
There was a problem hiding this comment.
maybe refactor this whole block to a separate method
| "If the query results cache is enabled. This will keep results of previously executed queries " + | ||
| "to be reused if the same query is executed again."), | ||
|
|
||
| HIVE_QUERY_RESULTS_SAFE_CACHE_WRITE_ENABLED("hive.query.results.safe.cache.write.enabled", false, |
There was a problem hiding this comment.
for correct alphabetical order this should rather be hive.query.results.cache.safe.write.enabled
What changes were proposed in this pull request?
Introducing safe cache writing conf which is when enabled writing to cache directory should not happen dirctly and if the entry is valid then only that entry should be copied to cache directory.
Why are the changes needed?
spilling of cache directory was happening when query as cleanup is done in the post execution.
Does this PR introduce any user-facing change?
No
How was this patch tested?
locally and ci test