Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
reschke commented on code in PR #1427:
URL: https://github.com/apache/jackrabbit-oak/pull/1427#discussion_r1609655472
##
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/package-info.java:
##
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-@Version("1.0.0")
Review Comment:
If it's for use inside Oak only, it should be marked "@Internal".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
reschke commented on code in PR #1427:
URL: https://github.com/apache/jackrabbit-oak/pull/1427#discussion_r1609608631
##
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/package-info.java:
##
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-@Version("1.0.0")
Review Comment:
Is this a public API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
smiroslav merged PR #1427: URL: https://github.com/apache/jackrabbit-oak/pull/1427 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
ahanikel commented on PR #1427: URL: https://github.com/apache/jackrabbit-oak/pull/1427#issuecomment-2122246769 @smiroslav Could you have another look at this one so we can also have it in the 1.64 release? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
ahanikel commented on code in PR #1427:
URL: https://github.com/apache/jackrabbit-oak/pull/1427#discussion_r1599674013
##
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/PersistentDiskCache.java:
##
@@ -204,7 +204,17 @@ private void cleanUpInternal() {
}
if (cacheSize.get() > maxCacheSizeBytes * 0.66) {
File segment = segmentCacheEntry.getPath().toFile();
-cacheSize.addAndGet(-segment.length());
+long length = segment.length();
+if (length == 0) {
+if (logger.isDebugEnabled()) {
+logger.debug("Avoiding removal of zero-sized
file {}", segmentCacheEntry.getPath());
+} else {
+logger.warn("Avoiding removal of zero-sized
file.");
+}
+return;
+}
+long cacheSizeAfter = cacheSize.addAndGet(-length);
+diskCacheIOMonitor.updateCacheSize(cacheSizeAfter,
directory.length());
Review Comment:
You're both right, the directory size can only be calculated by traversing
the directory, the size attribute only shows the size of the directory
structure itself on disk. I'm going to change this to measure the calculated
size and the size change instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
ahanikel commented on code in PR #1427:
URL: https://github.com/apache/jackrabbit-oak/pull/1427#discussion_r1599546203
##
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/PersistentDiskCache.java:
##
@@ -204,7 +204,17 @@ private void cleanUpInternal() {
}
if (cacheSize.get() > maxCacheSizeBytes * 0.66) {
File segment = segmentCacheEntry.getPath().toFile();
-cacheSize.addAndGet(-segment.length());
+long length = segment.length();
+if (length == 0) {
+if (logger.isDebugEnabled()) {
Review Comment:
@smiroslav The debug logs the path which could mean a lot of logging if
those zero-sized files occur often. The warnings are always the same string
without the file name, so if they occur frequently, they are suppressed and
logged as "last warning was repeated nnn times" (or something like that).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
joerghoh commented on code in PR #1427:
URL: https://github.com/apache/jackrabbit-oak/pull/1427#discussion_r1598601449
##
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/PersistentDiskCache.java:
##
@@ -204,7 +204,17 @@ private void cleanUpInternal() {
}
if (cacheSize.get() > maxCacheSizeBytes * 0.66) {
File segment = segmentCacheEntry.getPath().toFile();
-cacheSize.addAndGet(-segment.length());
+long length = segment.length();
+if (length == 0) {
+if (logger.isDebugEnabled()) {
+logger.debug("Avoiding removal of zero-sized
file {}", segmentCacheEntry.getPath());
+} else {
+logger.warn("Avoiding removal of zero-sized
file.");
+}
+return;
+}
+long cacheSizeAfter = cacheSize.addAndGet(-length);
+diskCacheIOMonitor.updateCacheSize(cacheSizeAfter,
directory.length());
Review Comment:
It probably should be this:
```suggestion
diskCacheIOMonitor.updateCacheSize(cacheSizeAfter,
segment.length());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
smiroslav commented on code in PR #1427:
URL: https://github.com/apache/jackrabbit-oak/pull/1427#discussion_r1593534438
##
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/PersistentDiskCache.java:
##
@@ -204,7 +204,17 @@ private void cleanUpInternal() {
}
if (cacheSize.get() > maxCacheSizeBytes * 0.66) {
File segment = segmentCacheEntry.getPath().toFile();
-cacheSize.addAndGet(-segment.length());
+long length = segment.length();
+if (length == 0) {
+if (logger.isDebugEnabled()) {
Review Comment:
Why both debug and warn are needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
smiroslav commented on code in PR #1427:
URL: https://github.com/apache/jackrabbit-oak/pull/1427#discussion_r1593533371
##
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/PersistentDiskCache.java:
##
@@ -204,7 +204,17 @@ private void cleanUpInternal() {
}
if (cacheSize.get() > maxCacheSizeBytes * 0.66) {
File segment = segmentCacheEntry.getPath().toFile();
-cacheSize.addAndGet(-segment.length());
+long length = segment.length();
+if (length == 0) {
+if (logger.isDebugEnabled()) {
+logger.debug("Avoiding removal of zero-sized
file {}", segmentCacheEntry.getPath());
+} else {
+logger.warn("Avoiding removal of zero-sized
file.");
+}
+return;
+}
+long cacheSizeAfter = cacheSize.addAndGet(-length);
+diskCacheIOMonitor.updateCacheSize(cacheSizeAfter,
directory.length());
Review Comment:
https://docs.oracle.com/javase/8/docs/api/java/io/File.html#length--
API docs says that File.length returns unspecified value for directories.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] OAK-10771 - Add disk cache size stats and issue warning if evicted segment has zero length [jackrabbit-oak]
ahanikel opened a new pull request, #1427: URL: https://github.com/apache/jackrabbit-oak/pull/1427 The persistent disk cache computes its size internally by adding/subtracting the size of added/purged segments. We would like to be able to see if that computation is correct, by having both the computed size and the effective size on disk in the metrics. The background for this is that in a few rare instances, the disk cache evicted too many items (down to almost zero) instead of the expected 65% of its max size. Also we want to make sure that no evictions happen on items that have a length of zero. We're not sure if that happens or not, so the change is to refuse eviction in that case and log a warning instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
