OAK-12282 : defining a fixed bound for the AbstractDiskCache

[pat-lego]

The AbstractPersistentCache is unbound leading to OOM exceptions.

[joerghoh]

a lot of minor things:

• Comments: AI writes too many comments which are 100% valid and helpful in the context of this PR, but not really helpful in a year in the context of a different problem.
• some consistency

[joerghoh]

I would remove this for consistency reasons (or we would need to annotate every configuration parameter for the cache with the same).

[pat-lego]

I cleared these comments as part of the fix since we can simply increase the list size no need for the toggle

[nfsantos]

Writing to disk will be limited to the performance of the IO system and does not scale much with the number of threads that are writing. In fact, a single thread writing efficiently to disk will likely saturate the disk. The max queue size should be a function of the write IO of the disk and of the memory that we want to devote to buffering pending writes, it should not depend on the number of write threads. And we also don't want to cause an OOME only by increasing the number of threads. The number of writers and the size of the queue should be separate settings.

Since segments are around 256KB, a queue size of 100 would take up 25MB when full. I'd say this is enough, as writes to a local SSD should be faster than reading from the network, so a few write threads should be able to keep up with whatever we can get from Azure.

[joerghoh]

Writing to disk will be limited to the performance of the IO system and does not scale much with the number of threads that are writing

This is definitely true for the classic hard drives (aka "spinning rust"), but not anymore for NVME-SSDs. You get the full IOPS only when you execute operations at a massive scale (IIRC 256 concurrent threads). And this is applicable for both read and write. Assuming a modern operating system also the filesystem should be able to handle that.
For that I strongly recommend to keep these 10 as default.

[nfsantos]

Yes, my claim that read/write does not scale much with number of threads is wrong for modern SSDs. They do scale.
In this particular case, the maximum download rate we can achieve from Azure will likely be far below what we can write to an SSD, so a small number of writers may be enough to keep up. But I was not advocating to decrease the number of writer threads. 10 is probably more than needed but the cost of having these threads around is minimal, so it's not an issue.

[nfsantos]

Personally, I think this is unnecessary. The change is quite safe. And if for some reason having a bound on the queue causes trouble, we can increase it's size to be effectively unlimited.

[nfsantos]

LGTM, but please get a few more approvals from people who are more involved with the usage of this cache.

[nfsantos]

Yes, my claim that read/write does not scale much with number of threads is wrong for modern SSDs. They do scale.
In this particular case, the maximum download rate we can achieve from Azure will likely be far below what we can write to an SSD, so a small number of writers may be enough to keep up. But I was not advocating to decrease the number of writer threads. 10 is probably more than needed but the cost of having these threads around is minimal, so it's not an issue.

[pat-lego]

@joerghoh any additional requests for this PR or can we merge it?

[joerghoh]

lgtm