[server] Validate block-cache config against system memory #1508
Conversation
| rocksDBServerConfig.getRocksDBBlockCacheSizeInBytes() + (rocksDBServerConfig.isUseSeparateRMDCacheEnabled() | ||
| ? rocksDBServerConfig.getRocksDBRMDBlockCacheSizeInBytes() | ||
| : 0); | ||
| if (Runtime.getRuntime().maxMemory() * 0.8 < cacheBytesNeeded) { |
There was a problem hiding this comment.
can we make this threshold configurable?
Also can multiple rocksDB be instantiated resulting in a combined usage of more than the allowed memory?
There was a problem hiding this comment.
I don't think this will work. The Runtime will tell you information about the JVM, but the rocksdb data usage won't be billed against that?
There was a problem hiding this comment.
blockcache is shared amongst all rocksdb engines.
There was a problem hiding this comment.
Yeah, Zac is correct.
One way in Oracle JVM (getting this from stackoverflow):
long memorySize = ((com.sun.management.OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean()).getTotalPhysicalMemorySize();
Not sure whether it works with OpenJDK or not.
| @@ -338,9 +338,9 @@ public RocksDBServerConfig(VeniceProperties props) { | |||
| } | |||
|
|
|||
| this.rocksDBBlockCacheSizeInBytes = | |||
| props.getSizeInBytes(ROCKSDB_BLOCK_CACHE_SIZE_IN_BYTES, 16 * 1024 * 1024 * 1024L); // 16GB | |||
| props.getSizeInBytes(ROCKSDB_BLOCK_CACHE_SIZE_IN_BYTES, 2 * 1024 * 1024 * 1024L); // 2GB | |||
There was a problem hiding this comment.
why the jump from 16 -> 2?
There was a problem hiding this comment.
Glancing at our config repo, we're already explicitly setting these in all colos, so the only users who this 'might' impact at davinci users? We should check that. I don't think shrinking it is going to be a problem for server deployments. Though I too am curious to the logic for why we'd like to shrink these defaults in this PR
There was a problem hiding this comment.
To make all the tests pass as mac jvm allocates 4G of memory
There was a problem hiding this comment.
DVC users pick up from global-config or can override it, which is also set to 16G.
There was a problem hiding this comment.
Can we please verify how this config is getting defaulted for Davinci users? If we're covered there I think this is fine.
There was a problem hiding this comment.
Emm.. Why not just override it in VeniceServerWrapper instead of here to let test pass in Mac?
| @@ -338,9 +338,9 @@ public RocksDBServerConfig(VeniceProperties props) { | |||
| } | |||
|
|
|||
| this.rocksDBBlockCacheSizeInBytes = | |||
| props.getSizeInBytes(ROCKSDB_BLOCK_CACHE_SIZE_IN_BYTES, 16 * 1024 * 1024 * 1024L); // 16GB | |||
| props.getSizeInBytes(ROCKSDB_BLOCK_CACHE_SIZE_IN_BYTES, 2 * 1024 * 1024 * 1024L); // 2GB | |||
There was a problem hiding this comment.
Emm.. Why not just override it in VeniceServerWrapper instead of here to let test pass in Mac?
| this.rocksDBRMDBlockCacheSizeInBytes = | ||
| props.getSizeInBytes(ROCKSDB_RMD_BLOCK_CACHE_SIZE_IN_BYTES, 2 * 1024 * 1024 * 1024L); // 2GB | ||
| props.getSizeInBytes(ROCKSDB_RMD_BLOCK_CACHE_SIZE_IN_BYTES, 1 * 1024 * 1024 * 1024L); // 1GB |
| rocksDBServerConfig.getRocksDBBlockCacheSizeInBytes() + (rocksDBServerConfig.isUseSeparateRMDCacheEnabled() | ||
| ? rocksDBServerConfig.getRocksDBRMDBlockCacheSizeInBytes() | ||
| : 0); | ||
| if (Runtime.getRuntime().maxMemory() * 0.8 < cacheBytesNeeded) { |
There was a problem hiding this comment.
Yeah, Zac is correct.
One way in Oracle JVM (getting this from stackoverflow):
long memorySize = ((com.sun.management.OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean()).getTotalPhysicalMemorySize();
Not sure whether it works with OpenJDK or not.
| : 0); | ||
|
|
||
| long systemMemorySize = getOSMemorySize(); | ||
| if (systemMemorySize > 0 && (systemMemorySize * 0.8 < cacheBytesNeeded)) { |
There was a problem hiding this comment.
As @m-nagarajan mentioned, it is better to make the ratio configurable.
| OperatingSystemMXBean osBean = ManagementFactory.getOperatingSystemMXBean(); | ||
|
|
||
| if (osBean instanceof com.sun.management.OperatingSystemMXBean) { | ||
| com.sun.management.OperatingSystemMXBean extendedOsBean = (com.sun.management.OperatingSystemMXBean) osBean; |
There was a problem hiding this comment.
Have you tested this function in Mac or Linux with OpenJDK?
If it doesn't work in these envs, this feature will be useless...
There was a problem hiding this comment.
Yes, this works in mac, and linux container/helix code uses same mechanism to fetch system memory.
majisourav99
force-pushed
the
cfgCheck
branch
from
b5cde84 to
70246a3
Compare
Validate block-cache config against system memory
If operator deploys config with exceeds system resource limits, it makes the cluster into inoperable state and finally destroys the cluster. This PR fails rolling out such config for blockcache.
How was this PR tested?
GHCI
Does this PR introduce any user-facing changes?