Added changes to enable full file cache stats #17538
Conversation
Signed-off-by: Mayank Sharma <[email protected]>
mayanksharma27
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
bugmakerrrrrr,
jainankitk and
linuxpi
as code owners
|
❌ Gradle check result for da0dcc9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| /** | ||
| * Statistics on full file cache. | ||
| */ | ||
| @PublicApi(since = "3.0.0") |
There was a problem hiding this comment.
Lets mark all the new classes as ExperimentalAPI as it gives us flexibility to make changes w/o breaking the compatibility in next releases.
| private final CounterMetric activeFullFileBytes = new CounterMetric(); | ||
| private final CounterMetric activeFullFileCount = new CounterMetric(); |
There was a problem hiding this comment.
lets remove this along with hitsCount , missCount for now. It is easy for accounting this on block based files as it is on per block level. For full file, accounting for every read will make the bring the counting in every read/write adding to CPU/latency overhead. We can revisit it once we feel the need for it.
| @@ -103,6 +111,7 @@ public void testWritableWarmFeatureFlagDisabled() { | |||
| } | |||
|
|
|||
| public void testWritableWarmBasic() throws Exception { | |||
|
|
|||
There was a problem hiding this comment.
nit : remove unintended changes.
| for (NodeStats stats : response.getNodes()) { | ||
| FileCacheStats fcStats = stats.getFileCacheStats(); | ||
| if (!Objects.isNull(fcStats)) { | ||
| if (!isFileCacheEmpty(fcStats)) { |
There was a problem hiding this comment.
nit : we use convention of == false to increase readability throughout our repo . Pls follow the same.
|
|
||
| FileCache fileCache = internalTestCluster.getDataNodeInstance(Node.class).fileCache(); | ||
|
|
||
| // TODO: Make these validation more robust. |
There was a problem hiding this comment.
Pls add when are we going to do that . I guess switchable index input ?
| this.theCache = SegmentedCache.<Path, CachedIndexInput>builder() | ||
| // use length in bytes as the weight of the file item | ||
| .weigher(CachedIndexInput::length) | ||
| .listener((removalNotification) -> { | ||
| RemovalReason removalReason = removalNotification.getRemovalReason(); | ||
| CachedIndexInput value = removalNotification.getValue(); | ||
| Path key = removalNotification.getKey(); | ||
|
|
||
| if (removalReason != RemovalReason.REPLACED) { | ||
|
|
||
| // check if full file and publish metric | ||
| if (value instanceof CachedFullFileIndexInput) { | ||
| fullFileCacheStatsCollector.recordUsedFileBytes(value.length(), false); | ||
| } | ||
|
|
||
| catchAsRuntimeException(value::close); | ||
| catchAsRuntimeException(() -> Files.deleteIfExists(key)); | ||
| } | ||
| }) | ||
| .capacity(capacity) | ||
| .build(); | ||
| ; | ||
|
|
There was a problem hiding this comment.
Lets de-dupe the code which is there in other cxtor as well .
Description
Related Issues
Resolves #17537
Meta Issue #13149
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.