CASSSIDECAR-366 Added endpoint for returning the system disk information #297
Conversation
| // System information related permissions | ||
| public static final Permission SYSTEM = new StandardPermission("SYSTEM", CLUSTER_SCOPE); |
There was a problem hiding this comment.
nit: Should it be DISK_INFO:READ? SYSTEM might be too wide.
There was a problem hiding this comment.
It can be specific to disk info. Made the changes.
| public static final String LIFECYCLE_ROUTE = API_V1 + CASSANDRA + "/lifecycle"; | ||
|
|
||
| private static final String SYSTEM_API_PREFIX = API_V1 + "/system"; | ||
| public static final String SYSTEM_DISK_INFO_ROUTE = SYSTEM_API_PREFIX + "/diskinfo"; |
There was a problem hiding this comment.
following the convention (using - to connect words in endpoint), it should be disk-info.
|
|
||
| this.diskInfoCache = Caffeine.newBuilder() | ||
| .expireAfterWrite(CACHE_TTL) | ||
| .maximumSize(1) |
There was a problem hiding this comment.
The cache has only a single value.. Why using a cache? Seemingly an overhead.
If it remains a single value cache, maybe consider just using AtomicReference and a lastUpdated timestamp.
There was a problem hiding this comment.
Used cache as it fits the cache use case. But yeah it could be overhead for this simple use case. Changed it to atomic reference with timestamp.
nvharikrishna
force-pushed
the
366-trunk-sysinfo-disk
branch
from
3ef5949 to
c195d7e
Compare
yifan-c
left a comment
yifan-c
left a comment
There was a problem hiding this comment.
LGTM.
The discussion thread on adding OSHI dep: https://lists.apache.org/thread/md5voclo1900nvysqgvlmtsc5xqy3ckd
frankgh
left a comment
frankgh
left a comment
There was a problem hiding this comment.
Looks good in general. I have a few comments
| LIVE_MIGRATION("LIVE_MIGRATION", DATA_COPY, LIST_FILES); | ||
| LIVE_MIGRATION("LIVE_MIGRATION", DATA_COPY, LIST_FILES), | ||
|
|
||
| SYSTEM("SYSTEM", DISK_INFO); |
| CacheEntry newEntry = new CacheEntry(freshData, CACHE_TTL); | ||
|
|
||
| // Try to update with CAS | ||
| if (diskInfoCache.compareAndSet(current, newEntry)) { |
There was a problem hiding this comment.
NIT: formatting
| if (diskInfoCache.compareAndSet(current, newEntry)) { | |
| if (diskInfoCache.compareAndSet(current, newEntry)) | |
| { |
| /** | ||
| * Loads disk information from the system using OSHI | ||
| */ | ||
| private List<DiskInfo> loadDiskInfo() |
There was a problem hiding this comment.
NIT: can we keep the public methods before private methods?
| if (current == null || current.isExpired()) | ||
| { | ||
| List<DiskInfo> freshData = loadDiskInfo(); | ||
| CacheEntry newEntry = new CacheEntry(freshData, CACHE_TTL); |
There was a problem hiding this comment.
can we use the TimeProvider here instead? You could inject it as a class parameter for DiskInfoHandler and use it in this constructor
| CacheEntry(List<DiskInfo> diskInfo, Duration ttl) | ||
| { | ||
| this.diskInfo = diskInfo; | ||
| this.expiryTime = System.currentTimeMillis() + ttl.toMillis(); |
There was a problem hiding this comment.
can we use time provider here instead?
nvharikrishna
left a comment
nvharikrishna
left a comment
There was a problem hiding this comment.
Addressed review comments.
| CacheEntry newEntry = new CacheEntry(freshData, CACHE_TTL); | ||
|
|
||
| // Try to update with CAS | ||
| if (diskInfoCache.compareAndSet(current, newEntry)) { |
| if (current == null || current.isExpired()) | ||
| { | ||
| List<DiskInfo> freshData = loadDiskInfo(); | ||
| CacheEntry newEntry = new CacheEntry(freshData, CACHE_TTL); |
| CacheEntry(List<DiskInfo> diskInfo, Duration ttl) | ||
| { | ||
| this.diskInfo = diskInfo; | ||
| this.expiryTime = System.currentTimeMillis() + ttl.toMillis(); |
| /** | ||
| * Loads disk information from the system using OSHI | ||
| */ | ||
| private List<DiskInfo> loadDiskInfo() |
3 participants
CASSSIDECAR-366 Added endpoint for System Disk Information to Sidecar
Endpoint information:
Sample response:
[ { "totalSpace": 1000000000000, "freeSpace": 500000000000, "usableSpace": 450000000000, "name": "data1", "mount": "/dev/sda1", "type": "ext4" } ]