use caffeine caches, adds in memory tracking [DRAFT] #86
Conversation
| @@ -46,12 +48,14 @@ public S3SeekableInputStreamFactory( | |||
| this.parquetMetadataStore = new ParquetMetadataStore(configuration.getLogicalIOConfiguration()); | |||
| this.objectMetadataStore = | |||
| new MetadataStore(objectClient, telemetry, configuration.getPhysicalIOConfiguration()); | |||
| this.memoryTracker = new MemoryTracker(); | |||
| this.objectBlobStore = | |||
| new BlobStore( | |||
There was a problem hiding this comment.
Nit: we probably want to start moving our classes to the builder pattern. These constructors are getting pretty huge
| this.blobCache = | ||
| Caffeine.newBuilder() | ||
| .maximumSize(configuration.getBlobStoreCapacity()) | ||
| .expireAfterWrite(Duration.ofMillis(configuration.getCacheEvictionTimeMillis())) |
There was a problem hiding this comment.
I think we chatted about this - should/can we use both expire after write and expire after access?
There was a problem hiding this comment.
Yeah, looked into it..so it's not recommend to use both: ben-manes/caffeine#511,
I am thinking of going with expireAfterWrite for now. I actually got a bit stuck with this, did some benchmarking a few times and it was failing. Seems like it's likely to do with cache eviction, so I will need to look into this some more. Will get back to it after my PTO or pass onto someone else from the team.
| */ | ||
| public class MemoryTracker { | ||
|
|
||
| @Getter long memoryUsed; |
There was a problem hiding this comment.
I would guess this class is supposed to be thread safe, right? You want to make this AtomicLong
| @@ -76,6 +85,8 @@ public static PhysicalIOConfiguration fromConfiguration(ConnectorConfiguration c | |||
| .readAheadBytes(configuration.getLong(READ_AHEAD_BYTES_KEY, DEFAULT_READ_AHEAD_BYTES)) | |||
| .maxRangeSizeBytes(configuration.getLong(MAX_RANGE_SIZE_BYTES_KEY, DEFAULT_MAX_RANGE_SIZE)) | |||
| .partSizeBytes(configuration.getLong(PART_SIZE_BYTES_KEY, DEFAULT_PART_SIZE)) | |||
| .cacheEvictionTimeMillis( | |||
| configuration.getInt(CACHE_EVICTION_TIME_KEY, DEFAULT_CACHE_EVICTION_TIME_MILLIS)) | |||
| .build(); | |||
There was a problem hiding this comment.
Wait. Are we using this for both Blob cache and Block cache, with the same config?
Not saying this is wrong, but should these be configured separately?
| * @param bytesAllocated bytes allocated | ||
| * @return total memory in use | ||
| */ | ||
| public long incrementMemoryUsed(long bytesAllocated) { |
There was a problem hiding this comment.
Nit: probably want to use more consistent names here, something like trackAllocate trackFree or something?
| @@ -7,6 +7,7 @@ private Constants() {} | |||
|
|
|||
| public static final int ONE_KB = 1024; | |||
| public static final int ONE_MB = 1024 * 1024; | |||
| public static final int ONE_GB = ONE_MB * ONE_MB; | |||
There was a problem hiding this comment.
Do we use this anywhere?
ahmarsuhail
force-pushed
the
memory-tracking
branch
from
dfee1ee to
836b886
Compare
Issue #, if available:
Description of changes:
DRAFT
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.