[core] Introduce CacheStats and expose ScanStats #4678
Merged
Conversation
This file contains 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
mxdzs0612
force-pushed
the
CacheStats
branch
from
d3fb51e to
223e7bc
Compare
mxdzs0612
changed the title
mxdzs0612
force-pushed
the
CacheStats
branch
from
223e7bc to
1213bad
Compare
|
@JingsongLi Ready for review now, PTAL, thx~ |
JingsongLi
reviewed
Dec 13, 2024
paimon-core/src/main/java/org/apache/paimon/catalog/CachingCatalog.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/utils/ObjectsCache.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/operation/metrics/CatalogCacheStats.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/catalog/CachingCatalog.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/operation/metrics/CatalogCacheStats.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/operation/metrics/CatalogCacheStats.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/operation/metrics/CatalogCacheStats.java
Outdated
Show resolved
Hide resolved
| public class CatalogCacheStats { | ||
| private long databaseCacheSize; | ||
| private long tableCacheSize; | ||
| private long manifestCacheSize; |
There was a problem hiding this comment.
What it means? Memory size or something else?
There was a problem hiding this comment.
Size. I added a new long value to evaluate bytes in mem.
wwj6591812
reviewed
Dec 14, 2024
| @@ -305,6 +316,23 @@ public void invalidateTable(Identifier identifier) { | |||
| } | |||
| } | |||
|
|
|||
| public CatalogCacheStats getCatalogCacheStats() { | |||
There was a problem hiding this comment.
where call this method?
There was a problem hiding this comment.
It's for other engine to see these stats when selecting a Paimon table.
| this.databaseCacheSize = databaseCacheSize; | ||
| } | ||
|
|
||
| public long getTableCacheSize() { |
There was a problem hiding this comment.
where call these four getXXX method?
| this.missedObject = new AtomicLong(0); | ||
| } | ||
|
|
||
| public AtomicLong getHitObject() { |
paimon-core/src/main/java/org/apache/paimon/operation/metrics/ScanMetrics.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/table/source/snapshot/SnapshotReaderImpl.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/utils/ObjectsFile.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/utils/SegmentsCache.java
Outdated
Show resolved
Hide resolved
paimon-core/src/main/java/org/apache/paimon/catalog/CachingCatalog.java
Outdated
Show resolved
Hide resolved
| public static final ConfigOption<Boolean> CACHE_STATS_ENABLED = | ||
| key("cache.cache-stats-enabled") | ||
| .booleanType() | ||
| .defaultValue(false) |
There was a problem hiding this comment.
Why default value is false?
There was a problem hiding this comment.
Don't want to affect other engines who do not need this metrics. You think true is better?
mxdzs0612
force-pushed
the
CacheStats
branch
2 times, most recently
from
b180968 to
ca1cf74
Compare
JingsongLi
reviewed
Dec 17, 2024
paimon-core/src/main/java/org/apache/paimon/table/system/AuditLogTable.java
Outdated
Show resolved
Hide resolved
JingsongLi
reviewed
Dec 17, 2024
paimon-core/src/main/java/org/apache/paimon/operation/AbstractFileStoreScan.java
Outdated
Show resolved
Hide resolved
JingsongLi
reviewed
Dec 17, 2024
paimon-core/src/main/java/org/apache/paimon/operation/metrics/ObjectCacheStats.java
Outdated
Show resolved
Hide resolved
mxdzs0612
force-pushed
the
CacheStats
branch
from
4cfb98b to
64f7419
Compare
JingsongLi
reviewed
Dec 17, 2024
paimon-core/src/main/java/org/apache/paimon/operation/AbstractFileStoreScan.java
Outdated
Show resolved
Hide resolved
mxdzs0612
force-pushed
the
CacheStats
branch
from
64f7419 to
e299d86
Compare
JingsongLi
reviewed
Dec 17, 2024
| } | ||
|
|
||
| public void reportScan(ScanStats scanStats) { | ||
| latestScan = scanStats; | ||
| durationHistogram.update(scanStats.getDuration()); | ||
| } | ||
|
|
||
| public void reportCache(CacheMetrics cacheMetrics) { | ||
| this.cacheMetrics = cacheMetrics; |
There was a problem hiding this comment.
Why set cacheMetrics again? You should use this cacheMetrics...
There was a problem hiding this comment.
Just want to make the code unified....
|
+1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Purpose
Linked issue: close #xxx
Add CacheSizes for CachingCatalog and CacheMetrics for ObjectCache so that engine can get cache stats at query level.
Tests
added
API and Format
CachingCatalog.estimatedCacheSizes
ScanMetrics.getLatestScan
ScanMetrics.getCacheMetrics
Documentation
added